Coinbase Smart Wallet - aycozynfada's results

Lines of code

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

Vulnerability details

Impact

Malicious owner through the removeOwnerAtIndex() can remove other owners and make themselves the sole owner of the wallet. This is because the function's access modifiers only needs one owner executing this call to remove other owners.
Peradventure we have a wallet with 20 owners, a malicious owner on a rogue move can remove all other owners and gain sole access to the contract and gain access to all the funds in it.

Proof of Concept

The only owner access control modifier

   /// @notice Access control modifier ensuring the caller is an authorized owner
    modifier onlyOwner() virtual {
        _checkOwner();
        _;
    }

The check _onlyOwner() function which only needs one owner to valid to execute removeOwnerAtIndex()

    function _checkOwner() internal view virtual {
        if (isOwnerAddress(msg.sender) || (msg.sender == address(this))) {
            return;
        }

        revert Unauthorized();
    }

// @notice Removes an owner from the given `index`.
    ///
    /// @dev Reverts if the owner is not registered at `index`.
    ///
    /// @param index The index to remove from.
    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);
    }

Tools Used

Manual review, vs code

Recommended Mitigation Steps

removeOwnerAtIndex() should only be executed by address.this following a consensus decision from all wallet owners.

Assessed type

Access Control