Users can front run the signature of the paymaster operation leading to some problems.

[c4-bot-6]

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

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[raymondfam]

Will let the sponsor verify the runnable POC.

[wilsoncusack]

To be clear, the same signature cannot be used twice. The front run is interesting: requires a user to submit a userOp with the withdraw signature in paymasterAndData, and then call from their SCW (via another user op or direct call from an EOA owner) and directly call withdraw. There's a race condition here, but it could indeed hurt the paymaster's reputation if pulled off.

[c4-sponsor]

wilsoncusack marked the issue as disagree with severity

[c4-sponsor]

wilsoncusack (sponsor) acknowledged

[3docSec]

Because the front-run signature would DoS the second one, tokens would be spent only once (invalidating point 2. of the impact), so it looks more like a grieving attack on the MS reputation with no tokens at risk => medium seems more appropriate.

[c4-judge]

3docSec changed the severity to 2 (Med Risk)

[c4-judge]

3docSec marked the issue as satisfactory

[c4-judge]

3docSec marked the issue as selected for report