Exceeding Maximum Owner Limit in Smart Wallet Initialization

[c4-bot-7]

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

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

No intended cap in place indeed.

[c4-sponsor]

wilsoncusack marked the issue as disagree with severity

[wilsoncusack]

This is just a case of NatSpec being out of date. MultiOwnable is no longer constrained to 256 owners. Will fix

[c4-sponsor]

wilsoncusack (sponsor) confirmed

[3docSec]

The 256 owners limit check is indeed nowhere to be found. However, since there is no clear impact, we should treat this finding as QA (protocol not behaving as of spec).

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-a