[Security notice] Insufficient coverage for Soroban signature payload

[dmkozh]

TL;DR recommendations:

• Avoid re-using the private keys when changing the contract admin accounts (such as SAC admin accounts)
• When writing contracts and using require_auth_for_args, include the authorizer address into payload (e.g. user.require_auth_for_args((user, foo)) instead of user.require_auth_for_args((foo,)))

Summary

Due to a design oversight the standard signature payload for Soroban authorization entries is missing the address of the signer. In majority of scenarios this is not an issue, because the signer is still included in the final signature payload via the call tree (for example, the transfer source is always a part of the transfer function payload). However, there are scenarios where the signer address is omitted from the call arguments. For example, the SAC admin functions such as mint fetch the admin address directly from storage and call require_auth for it - admin address won't end up in the final signature payload at all. In these scenarios, if multiple accounts reuse the same signer key, then there is a potential for replaying the transactions that have already been authorized by one of the accounts.

To the best of our understanding, the most obvious vulnerable scenarios have not occurred on-chain. However, there is still a risk that there are some scenarios that are hard to detect. Also, until this has been fixed at the protocol level, there is always a risk of users being vulnerable due to sharing the private keys between the accounts.

Potential impact examples

Unsafe SAC admin rotation

1. G-account A is an admin of a SAC token contract. The public key A is also its own only signer.
   2 G-account B is another account that A owns. B has two signer public keys: B (weight 1) and A (weight 1). B also has default operation thresholds, so either key A, or key B can be used to authorize any Soroban operation.
2. A mints some of their token to to address: SAC.mint(to, 1000). A uses Soroban auth payload with Address credentials and signature expiring 1 day from now.
3. Shortly after (within 1 day) A switches the SAC admin to become B: SAC.set_admin(B)
4. Anyone can replay SAC.mint(to, 1000) call by taking signature from step 3 and switching the address in credentials from A to B. This happens because nonce used in signature has been previously associated with A.

Notice, that every nuance is important in the above sequence. For example, there is no vulnerability if B uses signature threshold of 2 (instead of 1), or if admin change happens after the signatures from A have expired, or if there is no admin rotation at all etc.

Unsafe require_auth_for_args usage

1. Contract developer implements their own custom token (custom_token contract)
2. In transfer(from, to, amount) function instead of calling from.require_auth() (equivalent to from.require_auth_for_args((from, to, amount)) they call from.require_auth_for_args((to, amount))
3. User owns two G-accounts A and B that both share the same signer key K with weight 1 and medium signature threshold of 1.
4. User executes custom_token.transfer(A, to, 1000) using Soroban auth payload with Address credentials
5. Anyone can take the signature from step 4 and execute custom_token.transfer(B, to, 1000)

Mitigation

A protocol change is planned for protocol 27 that will ensure that credentials owner is included in the default signature payload and there is no way to accidentally miss it.

As protocol 27 will not be available for the next few months, the following precautions can be taken in the meantime:

For account owners:

• If possible, just avoid sharing the private keys between accounts
• If you must share the keys, make sure that you don't use these accounts for the contract admin rotations - change the new admin's keys before performing the rotation

For contract developers:

• Most of the time just using require_auth is sufficient
• If you must use require_auth_for_args include the authorizer address into payload (e.g. user.require_auth_for_args((user, foo)) instead of user.require_auth_for_args((foo,)))
• Optionally, when fetching an address to authorize from storage, explicitly add that address to the auth payload via require_auth_for_args (similarly to the example above). This might be a reasonable precaution for the new contracts, but use your judgement for updating the existing contracts (as the probability of the users to be vulnerable is rather low)