wrong check for ETH balance is done inside the MagicSpend#validatePaymasterUserOp which can lead to reverts inside MagicSpend#postOp

[c4-bot-1]

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

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #110

[raymondfam]

See #110. With added test.

[c4-judge]

3docSec marked the issue as satisfactory