A User may accidentally loses control of the wallet due to reorg and lack of restriction

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

The wallet is expected to be deployed on Ethereum, Base, Optimism, Arbitrum, Polygon, BNB, Avalanche, Gnosis. However, in L2, there might be reorg attack which could lead to a user accidentally loses control of the wallet. Thus the funds stored in the contracts are permanently lost.

Proof of Concept

There's no restriction on the situation that a owner shouldn't remove himself or the owner list can not be empty in the function removeOwnerAtIndex . Thus the wallet is vulnerable to reorg attack.

    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);
    }

Consider the following situation:

1. address A is the only user of the wallet.
2. User calls addOwnerAddress with address A to add address B as the owner.
3. Later, User calls removeOwnerAtIndex with address A to remove address A as the owner.
4. Unluckily, reorg occurs and user first clears the ownership of address A and later wants to add address B. The later transaction would revert and the user lost control of the wallet forever.

Tools Used

Manual

Recommended Mitigation Steps

Add restriction that a user can not remove himself or the owner list should not be empty after removal (use a counter to count owner numbers).

Assessed type

Governance

[raymondfam]

Reorg doesn't work that way. But "the owner list should not be empty after removal" makes this a duplicate of #18.

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #18

[c4-pre-sort]

raymondfam marked the issue as duplicate of #22

[c4-pre-sort]

raymondfam marked the issue as duplicate of #181

[c4-judge]

3docSec marked the issue as satisfactory

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-b