Platform: Code4rena
Start Date: 14/03/2024
Pot Size: $49,000 USDC
Total HM: 3
Participants: 51
Period: 7 days
Judge: 3docSec
Id: 350
League: ETH
Rank: 19/51
Findings: 1
Award: $36.34
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: 0xmystery
Also found by: 0xbrett8571, 0xhacksmithh, 7ashraf, Bigsam, Circolors, IceBear, Jorgect, Koala, Limbooo, SBSecurity, Tigerfrake, ZanyBonzy, aycozynfada, cheatc0d3, cryptphi, d3e4, doublespending, foxb868, gpersoon, imare, jesjupyter, lsaudit, robriks, shealtielanz, y4y
36.3397 USDC - $36.34
The Smart Wallet does not prevent users from removing all owners. Consequently, if the Smart Wallet holds token balances, they will be lost permanently.
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.
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.
Error
#0 - c4-pre-sort
2024-03-21T22:31:33Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-03-21T22:31:39Z
raymondfam marked the issue as duplicate of #18
#2 - raymondfam
2024-03-21T22:32:11Z
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.
#3 - c4-pre-sort
2024-03-22T22:32:24Z
raymondfam marked the issue as duplicate of #22
#4 - c4-pre-sort
2024-03-24T14:46:02Z
raymondfam marked the issue as not a duplicate
#5 - c4-pre-sort
2024-03-24T14:46:06Z
raymondfam marked the issue as primary issue
#6 - wilsoncusack
2024-03-26T14:05:21Z
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.
#7 - c4-sponsor
2024-03-26T14:05:24Z
wilsoncusack marked the issue as disagree with severity
#8 - c4-sponsor
2024-03-26T14:05:26Z
wilsoncusack (sponsor) acknowledged
#9 - 3docSec
2024-03-27T08:49:43Z
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?
#10 - c4-judge
2024-03-27T08:50:02Z
3docSec marked the issue as satisfactory
#11 - 3docSec
2024-03-27T08:51:39Z
(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)
#12 - wilsoncusack
2024-03-27T13:08:40Z
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
removeOwnerAtIndexcall 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
#13 - c4-judge
2024-03-27T13:44:28Z
3docSec marked the issue as selected for report
#14 - 3docSec
2024-03-27T18:00:47Z
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.
#15 - c4-judge
2024-03-27T18:01:29Z
3docSec changed the severity to QA (Quality Assurance)
#16 - c4-judge
2024-03-27T18:01:33Z
3docSec marked the issue as grade-a
#17 - c4-judge
2024-03-27T21:05:31Z
3docSec marked the issue as not selected for report
#18 - khalidfsh
2024-03-28T18:11:23Z
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.
#19 - 3docSec
2024-03-29T07:55:20Z
Having this one as a separate finding than 114 is fair for the following reasons:
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