Coinbase Smart Wallet - jesjupyter's results

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