Coinbase Smart Wallet - imare's results

Lines of code

https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/MagicSpend/MagicSpend.sol#L133

Vulnerability details

Impact

If the MagicSpend paymaster is called by the EntryPoint with an aggregation of user/s operations the check done inside MagicSpend#validatePaymasterUserOp for the ETH balance is not enough and can produce a revert when MagicSpend@postOp is executed putting this paymaster on a bad reputation by wrongly pass the validation and working only with a bundle of size 1 for the user operation.

If a MagicSpend paymaster contract is used with handleAggregatedOps we get the same revert.

Proof of Concept

The revert happen becouse 'MagicSpend#validatePaymasterUserOp' and MagicSpend#postOp are coded to be called sequentially one after the other. But in the EntryPoint contract both function are called separately in a for loop which hinders the 'MagicSpend#validatePaymasterUserOp' ETH balance validation on this line :

https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/MagicSpend/MagicSpend.sol#L133

The validation passes and when looping on postOp function the revert can happen on this line becouse the function SafeTransferLib.forceSafeTransferETH checks the balance and if its less then amount to send it reverts with ETHTransferFailed()

https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/MagicSpend/MagicSpend.sol#L160-L162

    function forceSafeTransferETH(address to, uint256 amount, uint256 gasStipend) internal {
        /// @solidity memory-safe-assembly
        assembly {
            if lt(selfbalance(), amount) {
                mstore(0x00, 0xb12d13eb) // `ETHTransferFailed()`. @<- here correctly reverts
                revert(0x1c, 0x04)
            }
        ...

The following POC is a new test inside the Integration.t.sol file which demonstrates that if we construct an array of UserOps operations that is bigger then one the test will fail with a revert of ETHTransferFailed() when executing MagicSpend::postOp.

    function test_MoreThanOneUserOpsPaymasterPays() public {
        uint256 NUMBER_OF_USER_OPS = 2; // @-> to see test succeed change this to 1

        UserOperation[] memory ops = new UserOperation[](NUMBER_OF_USER_OPS);

        for (uint256 i; i<NUMBER_OF_USER_OPS; i++) {
            nonce = i; // change nonce for paymasterAndData field generation !

            UserOperation memory op = _getUserOp();
            op.nonce = i; // change use nonce

            bytes32 hash = entryPoint.getUserOpHash(op);
            console.logBytes32(hash);
            (uint8 v, bytes32 r, bytes32 s) = vm.sign(accountOwnerPk, hash);
            bytes memory userOpSig =
                abi.encode(CoinbaseSmartWallet.SignatureWrapper({ownerIndex: 0, signatureData: abi.encodePacked(r, s, v)}));
            op.signature = userOpSig;
            
            ops[i] = op;
        }

        // assertEq(op.sender.balance, 0);
        entryPoint.handleOps(ops, payable(address(1))); // will revert with `ETHTransferFailed()`
    }

Tools Used

Manual review

Recommended Mitigation Steps

Check the ETH balance by taking in account the already promised withdraw amounts by introducing a global variable :

contract MagicSpend is Ownable, IPaymaster {

+   uint256 allWithdraws;

    function validatePaymasterUserOp(UserOperation calldata userOp, bytes32, uint256 maxCost)
        external
        onlyEntryPoint
        returns (bytes memory context, uint256 validationData)
    {
        WithdrawRequest memory withdrawRequest = abi.decode(userOp.paymasterAndData[20:], (WithdrawRequest));
        uint256 withdrawAmount = withdrawRequest.amount;


        if (withdrawAmount < maxCost) {
            revert RequestLessThanGasMaxCost(withdrawAmount, maxCost);
        }


        if (withdrawRequest.asset != address(0)) {
            revert UnsupportedPaymasterAsset(withdrawRequest.asset);
        }


        _validateRequest(userOp.sender, withdrawRequest);


        bool sigFailed = !isValidWithdrawSignature(userOp.sender, withdrawRequest);
        validationData = (sigFailed ? 1 : 0) | (uint256(withdrawRequest.expiry) << 160);


        // Ensure at validation that the contract has enough balance to cover the requested funds.
        // NOTE: This check is necessary to enforce that the contract will be able to transfer the remaining funds
        //       when `postOp()` is called back after the `UserOperation` has been executed.
-       if (address(this).balance < withdrawAmount) {
+       if (address(this).balance - allWithdraws < withdrawAmount) {      
            revert InsufficientBalance(withdrawAmount, address(this).balance);
        }


        // NOTE: Do not include the gas part in withdrawable funds as it will be handled in `postOp()`.
        _withdrawableETH[userOp.sender] += withdrawAmount - maxCost;
+       allWithdraws += withdrawAmount - maxCost;
        context = abi.encode(maxCost, userOp.sender);
    }

    ...

Don't forget to decrease the allWithdraws variable when the amount is withdrawn to the user !

Assessed type

Error

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