The anti-bricking mechanism only applies to the normal mode, but not the recovery mode

[code423n4]

Lines of code

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

Vulnerability details

Impact

recoveryInfoHash is critical for recovery but is not protected in the same way a signer's own privilege is protected.
It is therefore possible for a recovery key to remove its own ability to recover the account, bricking the account.

Proof of Concept

The anti-bricking mechanism is supposed to prevent a signer from signing away their own privilege. This is checked by require(privileges[signerKey] != bytes32(0), 'PRIVILEGE_NOT_DOWNGRADED'); at the end of AmbireAccount.execute(). But this presupposes that it was signerKey that signed the transaction just executed. This is not the case in the case of a recovery. Then signerKey is signerKeyToRecover such that [privileges[signerKeyToRecover] == recoveryInfoHash](https://github.com/AmbireTech/ambire-common/blob/5c54f8005e90ad481df8e34e85718f3d2bfa2ace/contracts/AmbireAccount.sol#L153). signerKeyToRecover is supposedly the key we are trying to recover, meaning it is lost, so it doesn't matter what happens to its privileges. Probably we are about to give privilege to a new address.
It seems plausible that this new address might be incorrectly entered, and in this case it is critical that the recovery key (which signed the transaction about to be executed) cannot revoke its own ability to recover, leaving the account with no authorised signer.
Recovery keys do not operate based on privileges but on there being an appropriate recoveryInfoHash set, as noted above. Therefore this value must not be allowed to change, which would be the analogous anti-bricking mechanism for recoveries.

Recommended Mitigation Steps

At a minimum simply check that privileges[signerKeyToRecover] == recoveryInfoHash still.

But note that we might actually want to be able to set privileges[signerKeyToRecover] = 0 for fear that the lost key might become compromised. Currently this is not possible. It would be acceptable to move recoveryInfoHash somewhere else, i.e. privileges[someOtherSignerKeyToRecover] = recoveryInfoHash. Note that someOtherSignerKeyToRecover may actually be an arbitrary address; it is only used to retrieve recoveryInfoHash. So it is not critical that someOtherSignerKeyToRecover is a valid address that we can sign with; we can still use it to recover with.
A alternative could then be to allow privileges[signerKeyToRecover] == 0 for recoveries being finalised and instead require that privileges[someOtherSignerKeyToRecover] == recoveryInfoHash for someOtherSignerKeyToRecover. This can be seen as having two separate but analogous anti-bricking mechanisms, one for normal execution and one for recovery.

Assessed type

Invalid Validation

[Picodes]

Seems Low severity to me as:

• the anti-bricking mechanism is a safety check but there is no real attack path that wouldn't require an user mistake here
• there is already a timelock to protect the wallet from malicious recovery payloads
• as the reports concede, "we might actually want to be able to set privileges[signerKeyToRecover] = 0 in fear that the lost key might become compromised." so it's not clear what the best procedure would be. Enforcing privileges[someOtherSignerKeyToRecover] == recoveryInfoHash for someOtherSignerKeyToRecover doesn't look like a good solution to me as you're enforcing the recovery procedure to give signing rights to another address, which could be dangerous.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)