All Smart Wallet funds will be lost if users remove all owners

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/MultiOwnable.sol#L102

Vulnerability details

Impact

The Smart Wallet does not prevent users from removing all owners. Consequently, if the Smart Wallet holds token balances, they will be lost permanently.

Proof of Concept

In the current implementation of MultiOwnable, an owner address can remove itself either by an immediate call to removeOwnerAtIndex or by signing a userOp that contains an execution of removeOwnerAtIndex, which is then executed by SmartWallet::execute. Even if the user signs another userOp to add another owner and this operation occurs after the removal process, the second operation would not be valid when the entry point attempts to validate user operation in SmartWallet::validateUserOp, as the owner in the index from SignatureWrapper would return a zero address.

src/SmartWallet/CoinbaseSmartWallet.sol:
298:         SignatureWrapper memory sigWrapper = abi.decode(signature, (SignatureWrapper));
299:         bytes memory ownerBytes = ownerAtIndex(sigWrapper.ownerIndex);  

Consequently, all tokens in the Smart Wallet will be lost permanently as there are no owners available to sign valid operations or to immediately execute any arbitrary calls.

Tools Used

• Manual review

Recommended Mitigation Steps

The Smart Wallet contract should consider to include safeguards that prevent the removal of all owners. This could involve setting a minimum number of required owners or implementing a mechanism to prevent the last owner from being removed.

Also, provide clear documentation and guidance to users on the importance of maintaining multiple owners and the risks associated with removing all owners from the Smart Wallet.

Assessed type

Error

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #18

[raymondfam]

Owners are typically controlled by the original owner. Nonetheless, a measure to prevent such a scenario with no owners left from happening would be beneficial.

[c4-pre-sort]

raymondfam marked the issue as duplicate of #22

[c4-pre-sort]

raymondfam marked the issue as not a duplicate

[c4-pre-sort]

raymondfam marked the issue as primary issue

[wilsoncusack]

There are many ways for users to lose access to wallet funds, including add incorrect owners, or removing all owners. This is not something we intend to fix or guard against.

[c4-sponsor]

wilsoncusack marked the issue as disagree with severity

[c4-sponsor]

wilsoncusack (sponsor) acknowledged

[3docSec]

Users signing a transaction to remove the last user in their wallet would normally be a user error, and I would agree with the sponsor on not considering it of particular concern.

It is however worth mentioning that the removeOwnerAtIndex call can be replayed across chains, so it's unlikely but possible that a wallet remains without owners after a transaction is replayed against the signer's intention.

Would you @wilsoncusack reconsider your statement in light of this?

[c4-judge]

3docSec marked the issue as satisfactory

[3docSec]

(as per grouping, I am leaving under this primary issue only the findings that explicitly mention the possibility of the wallet remaining without any owners left in power)

[wilsoncusack]

Users signing a transaction to remove the last user in their wallet would normally be a user error, and I would agree with the sponsor on not considering it of particular concern.

It is however worth mentioning that the removeOwnerAtIndex call can be replayed across chains, so it's unlikely but possible that a wallet remains without owners after a transaction is replayed against the signer's intention.

Would you @wilsoncusack reconsider your statement in light of this?

IMO the key finding here would be https://github.com/code-423n4/2024-03-coinbase-findings/issues/114, then. And not to do with guards on removing all owners. Having a guard on removing all owners would just serve as a sort of accidental check on a possibly undesirable remove owner replay. cc @xenoliss

[c4-judge]

3docSec marked the issue as selected for report

[3docSec]

After consulting with a fellow judge, I'll mark this one as QA:

While it's concerning that the wallet remains without owners without it being a user mistake (replay), none of the wardens noticed the key "replay" aspect that increases severity. This association to increase severity was done in #114.

I am downgrading this one to Low because it's consistent with how it has been reported rather than how it "could have been". The sponsor is however warmly recommended to give the mitigation of this finding similar priority as the mitigation of #114, and to include such mitigation in the mitigation review scope.

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-a

[c4-judge]

3docSec marked the issue as not selected for report

[khalidfsh]

I appreciate your thorough assessment of both Issue #114 and Issue #181. After carefully reviewing the findings, I would like to respectfully suggest reconsidering the classification of Issue #181 as a duplicate of Issue #114. Below, I outline my reasoning:

• Both Issue #114 and issue #181 originate from vulnerabilities within the MultiOwnable contract, specifically concerning owner management. The root cause of both vulnerabilities appears to be the same: inadequacies in the contract logic related to owner removal.

• While Issue #114 appropriately highlights the risk of unintentional owner removal across different chains, it may not fully capture the critical impact outlined in Issue #181. Issue 181 specifically emphasizes the possibility of users removing all owners, resulting in permanent loss of access to wallet funds. This severe consequence warrants equal attention and mitigation efforts.

• Addressing the root cause identified in Issue #114, such as implementing safeguards against unintended owner removal, would likely mitigate the risk outlined in Issue #181. Therefore, treating Issue #181 as a duplicate of Issue #114 aligns with the principle of rational fixes, as outlined by Code4rena's judging criteria.

I acknowledge the importance of exercising caution and considering the judge's perspective. However, it's crucial to ensure that all critical impacts are appropriately addressed and that similar exploits with the same root cause are treated consistently. Thank you for your attention to this matter.

[3docSec]

Having this one as a separate finding than 114 is fair for the following reasons:

• the key aspect of 114 is replayability across chains, and the fact that different chains may have a different state of owners. Both considerations are entirely missing in this group
• in post-contest hindsight, now that we are aware of both this and the 114 group, we can tell how dangerous this finding is, and that's why this finding will exceptionally be included in the report despite not being the QA selected, and the sponsor is informally recommended to fix it with the very same priority as 114. If any single one of the dupes had mentioned the increased risk due to cross-chain ownership misalignment, this could've very well been a valid High. Unfortunately, no warden did, and the fact that the finding COULD have been a valid High doesn't mean it should be judged as such, and this point is key to ensuring fairness to other participants
• the mitigation of 114 is not enough to mitigate this issue: the sponsor may (and did!) mitigate 114 by adding the owner address to the removeOwner request; the mitigation of this group is keeping a count of owners in power. You can fix one without fixing the other and vice-versa, so the principle of rational fixes doesn't hold when you look closely