Open c4-bot-7 opened 5 months ago
raymondfam marked the issue as sufficient quality report
raymondfam marked the issue as duplicate of #46
See #46.
raymondfam marked the issue as duplicate of #68
3docSec marked the issue as not a duplicate
3docSec changed the severity to QA (Quality Assurance)
This previously downgraded issue has been upgraded by 3docSec
3docSec marked the issue as duplicate of #88
3docSec marked the issue as not a duplicate
3docSec marked the issue as primary issue
Root cause: removeOwnerAtIndex
can be replayed cross-chain despite the same index may point to a different owner. The issue was confirmed by the sponsor in the #57 thread and addressed in this PR.
3docSec marked the issue as satisfactory
3docSec marked the issue as selected for report
After consulting with a fellow judge, I'm upgrading this one as high, as there is a well-defined attack path that causes the user to lose ownership of their wallet.
3docSec changed the severity to 3 (High Risk)
3docSec changed the severity to QA (Quality Assurance)
@3docSec was this downgraded mistakenly when de-duping #170 or for another reason?
Totally, I will report the issue to the C4 team, this is a technical issue, thanks for flagging @McCoady
This previously downgraded issue has been upgraded by 3docSec
Note: The sponsor requested to have the title of this finding updated to appropriately reflect the issue. The judge (3docSec) has reviewed and agreed to the change.
Final report title:
Remove owner calls can be replayed to remove a different owner at the same index, leading to severe issues when combined with lack of last owner guard
Lines of code
https://github.com/code-423n4/2024-03-coinbase/blob/main/src/SmartWallet/MultiOwnable.sol#L102
Vulnerability details
Impact
Users are able to upgrade their account's owners via either directly onto the contract with a regular transaction or via an ERC-4337 EntryPoint transaction calling
executeWithoutChainIdValidation
. If a user chooses to use a combination of these methods it's very likely that the addresses at a particular ownership index differ across chain. Therefore if a user later callsremoveOwnerAtIndex
on another chain will end up removing different addresses on different chains. It is unlikely this would be the users intention. The severity of this ranges from minimal (the user can just add the mistakenly removed owner back) or criticial (the user mistakenly removes their only accessible owner on a specific chain, permanently locking the account).Proof Of Concept
Scenario A: Alice permanently bricks her account on an unused chain:
executeWithoutChainIdValidation
Scenario A: Alice adds owners using both methods and ends up with undesired results
execute
.executeWithoutChainIdValidation
executeWithoutChainIdValidation
to callremoveOwnerAtInde
she will be removing different owners on different chains, which is likely not her intention.While more complex scenarios than this might sound bizarre it's important to remember that Alice could be using this smart account for the next N years, only making changes sporadically, and as her ownership mappings across different chains become more out of sync the likelihood of a signifanct error occuring increases.
Recommended Mitigation As
MultiOwnableStorage
uses a mapping to track owner information rather than a conventional array, it might be simpler to do away with the indexes entirely and have aremoveOwner(bytes calldata _ownerToRemove)
function. This would avoid the sitations outlined above where when callingremoveOwnerAtIndex
removes different owners on different chains. To ensure replayability and avoid having a stuck nonce on chains where_ownerToRemove
is not an owner the function should not revert in the case the owner is not there, but instead return a boolremoved
to indicate whether an owner was removed or not.This would make it significantly less likely that users run into the issues stated above, without having to limit their freedom to make ownership changes manually or via ERC-4337 EntryPoint transactions.
Assessed type
Other