c4-bot-10 commented 8 months ago

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.