Malicious owner can remove other owners and make themselves sole owner of the wallet.

[c4-bot-10]

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

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #18

[raymondfam]

See #18.

[c4-pre-sort]

raymondfam marked the issue as duplicate of #22

[c4-pre-sort]

raymondfam marked the issue as duplicate of #181

[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-b