Owner/s can remove all wallet owners

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

The current implementation of removing an owner allows a single owner to remove all users even himself from the wallet ownership

Proof of Concept

The following function inherited by the wallet contract allows any owner to remove all (even himself) as a owner

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

The POC is added to the RemoveOwnerAtIndex.t.sol tests file :

    function testRemoveAllOwners() public {
        uint256 numOfOwners = mock.nextOwnerIndex();
        // console.log("owners", numOfOwners);

        // @ this call be batched with `CoinbaseSmartWallet#executeBatch`

        // @!! the current calling account must be the last index as a owner to call the following function
        vm.startPrank(owner1Address);
        for (uint256 i = numOfOwners - 1; i> 0; i--) { 
            mock.removeOwnerAtIndex(i);
        }
        vm.stopPrank();
    }

Main points from the POC are:

1. To remove all the owners the calling (malicious) owner must remove itself as the last one
2. The entire call can be constructed offline (with ownerAtIndex returning some value) and batched with CoinbaseSmartWallet#executeBatch

Tools Used

Manual review

Recommended Mitigation Steps

Allow the owner to only remove himself from the wallet.

With the current removeOwnerAtIndex removing a address cannot be simply done becouse some owners register them self as an EC point.

The solution would be to create a new function wich uses a signature to verify (like with CoinbaseSmartWallet_validateSignature) that the msg.sender is indeed the right user to remove.

Assessed type

Error

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