Smart wallet owners can avoid being removed by frontrun-removing other owners.

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

The removeOwnerAtIndex function allows owners to be removed. The function can only be called by the owners or the contract itself. The issue is that owners can evade this removal by frontrunning the calls to the function and removing the owner that wants to remove them, thereby evading and consequently griefing other owners in the process.

The removeOwnerAtIndex holds the onlyOwner modifier which checks if the caller is an owner. Hence, only the owners can call.

    function removeOwnerAtIndex(uint256 index) public virtual onlyOwner {
        bytes memory owner = ownerAtIndex(index);
        if (owner.length == 0) revert NoOwnerAtIndex(index);

        delete _getMultiOwnableStorage().isOwner[owner];
        delete _getMultiOwnableStorage().ownerAtIndex[index];

        emit RemoveOwner(index, owner);
    }

By frontrunning the call to the function, and passing other owner's address, the to-be-removed owner can grief and potentially avoid being removed as an owner.

Proof of Concept

• User A and User B are users holding the indexes 1 and 2 in the storage.
• For some reason, User B being malicious for instance, User A calls the removeOwnerAtIndex function passing in index 2 to remove User B from owner's list.
• User B notices the transaction, frontruns it, passing in index 1 which is User A's index.
• Transaction executes successfully and User A is no longer an owner.
• User A's call fails and User B is still not removed.
• On a sidenote, User B can also backrun the call to _addOwnerAtIndex to remove User A if user A gets readded.

Tools Used

Manual code review

Recommended Mitigation Steps

A potential solution is limiting the calls to the removeOwnerAtIndex function to only the contract.
Another potential solution is removing the function completely, as it introduces griefing vectors.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #18

[raymondfam]

Mimicking the exploit path of #57.

[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 duplicate of #57

[c4-judge]

3docSec marked the issue as not a duplicate

[c4-judge]

3docSec marked the issue as duplicate of #18

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-a

[3docSec]

Hi @ZanyBonzy ,

I see your points, but all attacks require the attacker to have been previously added as a trusted owner, so even the most sophisticated attack can only come out of a serious mistake of the user who leaked the keys to their wallet. It can't be more than QA under this consideration.

[PaperParachute]

Closing while discussing internally

[3docSec]

Hi @ZanyBonzy sorry for the confusion. The issue was reopened accidentally while I was confirming with the C4 team whether QA findings like this one need to be open to be eligible for awards.

The answer is no: as soon as there is a grade-a or grade-b label, the findings will receive a share of the QA pot. So the issue can stay closed without impacting awarding