removeOwnerAtIndex() can be front-run

[c4-bot-8]

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：

1. A user addOwnerAddress() add B as a new owner address.
2. B becomes malicious and A now wants to use removeOwnerAtIndex() to remove B.
3. 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.
4. If B remove initial owner A, now B can take over this smart wallet.

• similer issue: unauthorize() can be front-run so that the malicious authorized user would get their authority back 2023-01-drips-findings#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

[raymondfam]

See #61.

[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 not a duplicate

[c4-pre-sort]

raymondfam marked the issue as duplicate of #57

[c4-judge]

3docSec marked the issue as not a duplicate

[c4-judge]

3docSec marked the issue as duplicate of #18

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-a