Coinbase Smart Wallet - Tigerfrake's results

Lines of code

https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/MultiOwnable.sol#L162-L174

Vulnerability details

Summary

According to Coinbase Docs:

Our smart wallet supports up to 256 concurrent owners. Each owner can transact independently, without sign off from any other owner. 

However, the _initializeOwners() function lack this check for the maximum number of owners allowed by the smart wallet, which is stated to be 256 concurrent owners.

Proof of Concept

• https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/MultiOwnable.sol#L162-L174

    function _initializeOwners(bytes[] memory owners) internal virtual {
        for (uint256 i; i < owners.length; i++) {
            if (owners[i].length != 32 && owners[i].length != 64) {
                revert InvalidOwnerBytesLength(owners[i]);
            }


            if (owners[i].length == 32 && uint256(bytes32(owners[i])) > type(uint160).max) {
                revert InvalidEthereumAddressOwner(owners[i]);
            }


            _addOwnerAtIndex(owners[i], _getMultiOwnableStorage().nextOwnerIndex++);
        }
    }

The function iterates over the owners array and adds each owner without verifying if the total number of owners exceeds this limit.

Impact

This could potentially allow more than 256 owners to be added, which contradicts the stated contract documentation and could lead to unexpected behavior or security issues.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a check at the beginning of the function to ensure that the number of owners being added does not exceed the maximum limit.

Here's how you could modify the function to include this check:

function _initializeOwners(bytes[] memory owners) internal virtual {
    require(owners.length <= 256, "Too many owners"); // @audit Check for max limit
    for (uint256 i; i < owners.length; i++) {
        if (owners[i].length != 32 && owners[i].length != 64) {
            revert InvalidOwnerBytesLength(owners[i]);
        }

        if (owners[i].length == 32 && uint256(bytes32(owners[i])) > type(uint160).max) {
            revert InvalidEthereumAddressOwner(owners[i]);
        }

        _addOwnerAtIndex(owners[i], _getMultiOwnableStorage().nextOwnerIndex++);
    }
}

Assessed type

Other