Coinbase Smart Wallet - 0xhacksmithh's results

Lines of code

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

Vulnerability details

Impact

At the end wallet remain with no owners or with malicious owner.

Proof of Concept

In MultiOwnable.sol there is a function removeOwnerAtIndex() which allow a Owner to remove other owners

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

Here is onlyOwner() modifier which implemented as follows

    modifier onlyOwner() virtual {
        _checkOwner();
        _;
    }

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

        revert Unauthorized();
    }

    function isOwnerAddress(address account) public view virtual returns (bool) {
        return _getMultiOwnableStorage().isOwner[abi.encode(account)];
    }

Here you clearly see that OnlyOwner check boil down to isOwnerAddress() which ensure that if a user is Owner then he can remove other owners from index via deleting. And same he can do with himselfe as well.

If one of the Owner goes malicious then he can kick out all other owners.

Tools Used

Recommended Mitigation Steps

There should be Master Owner so he can revert Malicious Owner act

Assessed type

Access Control