Schedule recovery DOS by front-running with original schedule recovery transaction if no other transaction is executed

[code423n4]

Lines of code

https://github.com/AmbireTech/ambire-common/blob/455a8057e1e6edae48903fc9d116591b22fbf1c2/contracts/AmbireAccount.sol#L171-L175

Vulnerability details

Description

If after scheduling a recovery no transaction is executed, anyone can DOS the execution of this scheduled recovery by a signature replay attack given that the nonce is not increased

Impact

DOS of scheduled recovery execution if after a recovery is scheduled no other transaction is executed

POC

Consider next commented code

    // @audit previously storage variable nonce was never increased
    } else {
        // @audit Here it is calculated when a scheduled recovery can be executed
        scheduledRecoveries[hash] = block.timestamp + recoveryInfo.timelock;
        emit LogRecoveryScheduled(hash, recoveryInfoHash, recoveryKey, currentNonce, block.timestamp, calls);
    }
    // @audit we finish the function execution but storage variable nonce was never increased
    return;

Now we can imagine next possible scenario:

1. With nonce = 10 and current_time = 10_000, Alice schedule a recovery which can be executed at current_time = 13_000 (3000 secs more than the time when the transaction was sent). The nonce hold on 10
2. Alice does no execute any other transaction until current_time = 13_000
3. Alice sent the recovery transaction execution to the mempool, however Bob sees this and decide to front run her by sending the same signed transaction that alice sent in step 1, this will increase the scheduled recovery time to current_time = 16_000.
4. Alice transcation reverts given next line: require(block.timestamp > scheduled, 'RECOVERY_NOT_READY');

Mitigation steps

    if (isCancellation) {
        delete scheduledRecoveries[hash];
        emit LogRecoveryCancelled(hash, recoveryInfoHash, recoveryKey, block.timestamp);
    } else {
        scheduledRecoveries[hash] = block.timestamp + recoveryInfo.timelock;
        emit LogRecoveryScheduled(hash, recoveryInfoHash, recoveryKey, currentNonce, block.timestamp, calls);
    }
+   ++nonce;
    return;

It is important to notice that this recommendation also solves M-03

Assessed type

DoS

[Ivshti]

This will break things as you can't execute the recovery successfully: once the nonce has been incremented, the same hash cannot be matched.

As for this DOS, it cannot be performed because of this line - note scheduled != 0.

if (scheduled != 0 && !isCancellation) {

if on the other hand isCancellation is true we enter this code

          delete scheduledRecoveries[hash];
          nonce = currentNonce + 1;
          emit LogRecoveryCancelled(hash, recoveryInfoHash, recoveryKey, block.timestamp);

[carlitox477]

You are right, this one is invalid, I missed the first check, sorry

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid