Transactions bundles signed for a future nonce cannot be cancelled

[code423n4]

Lines of code

https://github.com/AmbireTech/ambire-common/blob/5c54f8005e90ad481df8e34e85718f3d2bfa2ace/contracts/AmbireAccount.sol#L138

Vulnerability details

Impact

Transaction bundles signed for a future nonce cannot be cancelled, expect by possibly unfeasibly many calls to execute().

Proof of Concept

AmbireAccount.execute() validates a signature against a hash based on an incrementing nonce. Only a transaction bundle hash with the current nonce can be executed. A signature for the current nonce may thus be invalidated by signing a dummy transaction bundle which only causes the nonce to increment, rendering the undesirable signature forever inexecutable.
But if a transaction bundle is signed for a nonce in the future, either by mistake or in anticipation of a executeMultiple() call, the only way to cancel the signature would be to repeatedly call execute() until the nonce has passed. This might cost significant gas (the signed nonce might be arbitrarily high), and the transaction bundle might then be executed anyway by a frontrunner.

Recommended Mitigation Steps

Implement a mapping which stores cancelled transaction bundle hashes, and check this before executing.

Assessed type

Context

[Ivshti]

Intended tradeoff - there’s no way we’d implement another gas overhead for this - native transactions work the same way, so I wouldn’t say that future nonce invalidation is a significant feature

[c4-judge]

Picodes marked the issue as primary issue

[Picodes]

Seems Low severity to me as this is how classic EOAs works

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[d3e4]

How is this how classic EOAs work? With an EOA you can send a new transaction with a custom nonce to cancel it. The issue here is precisely that you cannot do this. The nonce is set exclusively by the contract. So you can only cancel your current transaction. The current transaction is what #14 reported about.
This issue (#31) is about future nonces. These cannot be cancelled by just sending another transaction bundle.

[romeroadrian]

How is this how classic EOAs work? With an EOA you can send a new transaction with a custom nonce to cancel it. The issue here is precisely that you cannot do this. The nonce is set exclusively by the contract. So you can only cancel your current transaction. The current transaction is what #14 reported about. This issue (#31) is about future nonces. These cannot be cancelled by just sending another transaction bundle.

I reported a similar issue in #14 and the conclusion (which I agree) is that a dummy transaction can serve as a cancellation method. Similarly, this can be extended to handle multiple nonces using executeMultiple().

While I do agree that a function to increment the nonce could be useful, this is at best a QA finding or even a simple gas optimization.

[d3e4]

Similarly, this can be extended to handle multiple nonces using executeMultiple().

What if the signed nonce is 100, or 10000 or more, in the future? It may be arbitrarily expensive to have the contract increment the nonce in a loop until it's overtaken. This is not a viable solution for cancelling future nonces.

[romeroadrian]

Similarly, this can be extended to handle multiple nonces using executeMultiple().

What if the signed nonce is 100, or 10000 or more, in the future? It may be arbitrarily expensive to have the contract increment the nonce in a loop until it's overtaken. This is not a viable solution for cancelling future nonces.

It's the same as if you sign a transaction for an EOA with a nonce of 10000.

[d3e4]

It's still not quite the same because here there is a lot of costly code to execute. Even an executeMultiple() where each transaction bundle is empty has to store the new nonce and validate the signature for each nonce to increment. Note that for a multisig recoverAddrImpl() even has to be called several times for each nonce. And there's no telling how much the smart wallet signature costs; what if it's an (very expensive!) on-chain implementation of some signature verification algorithm?
There should at least be a way to bypass all of this (at least signature verification for empty transaction bundles) and just directly increase the nonce.

[Ivshti]

It's still not quite the same because here there is a lot of costly code to execute. Even an executeMultiple() where each transaction bundle is empty has to store the new nonce and validate the signature for each nonce to increment. Note that for a multisig recoverAddrImpl() even has to be called several times for each nonce. And there's no telling how much the smart wallet signature costs; what if it's an (very expensive!) on-chain implementation of some signature verification algorithm? There should at least be a way to bypass all of this (at least signature verification for empty transaction bundles) and just directly increase the nonce.

I really don't understand your argument.

Say you want to cancel nonce 1000 but the current nonce is 100. In an EOA, you can pre-sign a txn with nonce 1000 that you can then use to cancel if needed. If you want to cancel preemtively, you have to execute 900 signed transactions. Absolutely the same thing applies for Ambire - you can pre-sign a future nonce, or you can do the costly option and execute 900 signed bundles. The argument that the signature verification might be more expensive isn't strong enough IMO.

[Picodes]

If it's too expensive you can also just move your holdings to another wallet

[Picodes]

In any case, the fact that it behaves like an EOA is convincing enough to keep the Low severity here in my opinion.