c4-bot-8 commented 5 months ago

Lines of code

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

Vulnerability details

Impact

The initial owner of CoinbaseSmartWallet can use MultiOwnable.sol addOwnerAddress() to add a new owner address. onlyOwner modifier use _checkOwner to ensure the caller is an authorized owner

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

        revert Unauthorized();
    }

isOwnerAddress() checks if the given account address is registered as owner.

    /// @notice Checks if the given `account` address is registered as owner.
    ///
    /// @param account The account address to check.
    ///
    /// @return `true` if the account is an owner, else `false`.
    function isOwnerAddress(address account) public view virtual returns (bool) {
        return _getMultiOwnableStorage().isOwner[abi.encode(account)];
    }

Consider this senario：

A user addOwnerAddress() add B as a new owner address.
B becomes malicious and A now wants to use removeOwnerAtIndex() to remove B.
B noticed the tx and front runs it. B can use addOwnerAddress(), add another address C as a new owner, or directly use removeOwnerAtIndex() remove initial owner A.
If B remove initial owner A, now B can take over this smart wallet.

similer issue: https://github.com/code-423n4/2023-01-drips-findings/issues/163

Proof of Concept

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

Tools Used

Recommended Mitigation Steps

only initial owner of smart wallet can add/remove new address as owner

Assessed type

MEV