Recovery spells don't allow the addition or removal of owners, potentially leading to the compromisation of the wallet

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-10-kleidi/blob/main/src/RecoverySpell.sol#L165-L317

Vulnerability details

Description

When recovery spells are created, users preset the new owners of the safe that will be swapped. This is set at the wallet creation time, as the owners used will be part of the creation salt that is used to compute the recovery spell address. Recovery spells don't allow users to remove or add owners to that list.

In case 1 of the "new owners" is compromised and the recovery spell is used, that compromised owner will be injected as an owner in the safe's owners list, knowing that the previous owners are aware of this compromisation but had no solution to solve it.

This leads to the full compromisation of a wallet.

Proof of Concept

1. User A predicts the safe address using AddressCalculation::calculateAddress.
2. User A uses the safe's predicted address to predict a recovery spell whose owners are users C and D.
3. User A creates the safe while passing the owners as users A and B and passing the recovery spell predicted address in step 2 as the recovery spell.
4. User A deploys the recovery spell.
   At this point we assume both users B and D are compromised
5. User A initiates the wallet recovery by calling executeRecovery on the created spell, however, the issue here is that the new owners that will be replaced contain a compromised owner which is D.
6. At this point, user C can't directly remove D from the owners list in the save, as it'll require a time-locked transaction through the timelock, which is the same time user D will have to wait to execute a malicious transaction.

This results in a messed-up wallet, where a compromised owner is forcefully injected into the safe. As users before knew it was compromised but couldn't remove it.

Recommended Mitigation Steps

Allow users to add/remove recovery spell owners, which can be done by other owners signing that transaction.

Assessed type

Error

[ElliotFriedman]

Invalid issue, if recovery spell signers are compromised, the cold signers can remove the recovery spell through the timelock.

This is covered in known issues.

[GalloDaSballo]

Agree with the Sponsor

[c4-judge]

GalloDaSballo marked the issue as nullified