Coinbase Smart Wallet - Jorgect's results

Lines of code

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

Vulnerability details

The paymaster is a extension of the eip-4337, normally the paymaster is welling to pay a user transaction if the account can return the amount of gas at the final of the transaction.

In the context of the coinbase smart wallet the paymaster is the contract call magicSpend.sol, This contract expose the normal function need it to be a paymaster:

function validatePaymasterUserOp
    (PackedUserOperation calldata userOp, bytes32 userOpHash, uint256 maxCost)
    external returns (bytes memory context, uint256 validationData);

function postOp
    (PostOpMode mode, bytes calldata context, uint256 actualGasCost, uint256 actualUserOpFeePerGas)
    external;

The magic spend is also implementing the entry point deposit, unlock and withdraw functions as required.

Addionally of this the magicSpend is implementing a withdraw functions for users:

file:https://src/MagicSpend/MagicSpend.sol#L181
 function withdraw(WithdrawRequest memory withdrawRequest) external {
        _validateRequest(msg.sender, withdrawRequest);

        if (!isValidWithdrawSignature(msg.sender, withdrawRequest)) {
            revert InvalidSignature();
        }

        if (block.timestamp > withdrawRequest.expiry) {
            revert Expired();
        }

        // reserve funds for gas, will credit user with difference in post op
        _withdraw(withdrawRequest.asset, msg.sender, withdrawRequest.amount);
    }

[Link]

The problem is that validatePaymasterUserOp is consuming the same signature of the withdraw function, so user can request a transaction through the paymaster, then front runt this transaction calling the withdraw function in the magicSpend (as you notice this transaction is not being processed through the bundler so user can get this withdraw transaction first if he send the correct amount of gas to be included first) making the validatePaymasterUserOp revert because the nonce was already consumed.

This is behavior is so problematic see the impact.

Impact

Are there any griefing attacks that could cause this paymaster to be banned by bundlers?

1. Paymaster can be banned by bundlers because user can trigger revert transactions which is one of the reason because bundlers can banned paymasters.

I consider that this have to be another vulnerability but i decided to putted here because the main problem is the same.

2. Paymaster can be drained if user front run the signature gave to pay a operation, withdrawing directly this funds in the withdraw function, user can do this repeated times until the Paymaster is completed drained.

Proof of Concept

Run this test in file:/test/MagicSpend/Withdraw.t.sol :

    function test_frontRuning() public {  //@audit POC HIGH 1
        MagicSpend.WithdrawRequest memory WithdrawRequest = MagicSpend.WithdrawRequest({
            asset: asset,
            amount: amount, 
            nonce: nonce,
            expiry: expiry,
            signature: _getSignature()
        });

        UserOperation memory userOp = UserOperation({
            sender: withdrawer,
            nonce: 0,
            initCode: "",
            callData: "",
            callGasLimit: 49152,
            verificationGasLimit: 378989,
            preVerificationGas: 273196043,
            maxFeePerGas: 1000304,
            maxPriorityFeePerGas: 1000000,
            paymasterAndData: abi.encodePacked(address(magic), abi.encode(WithdrawRequest)),
            signature: ""
        });

        magic.withdraw(WithdrawRequest); //Front runing the validatePaymasterUserOp operation

        bytes32 userOpHash = sha256("hi");
        uint256 maxCost = amount - 10;

        vm.expectRevert();
        magic.validatePaymasterUserOp(userOp, userOpHash, maxCost);
    }

Tools Used

Manual,Foundry

Recommended Mitigation Steps

Consider add another signer for the withdraw function different from the validatePaymasterUserOp signer.

Assessed type

Other

Lines of code

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

Vulnerability details

When user wants create a wallet he has to call the createAccount function in the factory, this functions is taking the owner by a salt:

 function createAccount(bytes[] calldata owners, uint256 nonce)
        public
        payable
        virtual
        returns (CoinbaseSmartWallet account)
    {
        if (owners.length == 0) {
            revert OwnerRequired();
        }

        (bool alreadyDeployed, address accountAddress) =
            LibClone.createDeterministicERC1967(msg.value, implementation, _getSalt(owners, nonce));  <-----

        account = CoinbaseSmartWallet(payable(accountAddress));

        if (alreadyDeployed == false) {
            account.initialize(owners);
        }
    }

[Link]

Note that user have to deploy the wallet with the same owner and nonces if they want the same address for the wallet in others chain.

The problem is that if a user changed some owners (addOwnerAddress,removeOwnerAtIndex) whether the signature have chain id or not, the user need to pass the first owners to deploy the wallet in other chains.

Consider the next scenario:

1. user create a Coinbase wallet in all the chain availables with two owners.
2. One of this owners wallet get compromised, so the compromised owner get removed from the Coinbase wallet.
3. A new L2 get added in the protocol.
4. User want to deploy his wallet in the new chain but he notice that he need to pass the compromised owner wallet to create the wallet with the same address.

At this point the compromised owner wallet can do bad thing due he is an owner in other chain of the wallet, bad thing like stole money from the wallet, delete owners, etc

Impact

User may not be capable to create another wallet in other chain because he need the all owner of the wallet, one of this owners can be a compromised wallet who can steal funds.

Proof of Concept

As you can see below, the salt for a new wallet has to be the first owners of the wallet to get the same address:

 function createAccount(bytes[] calldata owners, uint256 nonce)
        public
        payable
        virtual
        returns (CoinbaseSmartWallet account)
    {
        if (owners.length == 0) {
            revert OwnerRequired();
        }

        (bool alreadyDeployed, address accountAddress) =
            LibClone.createDeterministicERC1967(msg.value, implementation, _getSalt(owners, nonce));  <-----

        account = CoinbaseSmartWallet(payable(accountAddress));

        if (alreadyDeployed == false) {
            account.initialize(owners);
        }
    }

[Link]

This can be problematic if the owners of the wallet was already changed.

Tools Used

Manual.

Recommended Mitigation Steps

The most straightforward solution if change the the way that the salt is got it instead of owner this can be some other string passed by the user.

Assessed type

Other