Balance check during MagicSpend validation cannot ensure that MagicSpend has enough balance to cover the requested fund.

[c4-bot-2]

Lines of code

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

Vulnerability details

Impact

Balance check during MagicSpend validation cannot ensure that MagicSpend has enough balance to cover the requested fund.

        // 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) {
            revert InsufficientBalance(withdrawAmount, address(this).balance);
        }

Proof of Concept

1. EntryPoint executes userOps by two loops: validation loop and execution loop.
2. Suppose we has two userOps using MagicSpend
   • userOp1 with withdrawAmount 100
   • userOp2 with withdrawAmount 150
3. userOp1 and userOp2 are packed together as UserOperation[] ops and ops will be executed by entryPoint through the handleOps method
4. Suppose the balance of MagicSpend is 200.
5. During the validation loop
   • validatePaymasterUserOp will be called during userOp1 validation and pass for address(this).balance (=200) is larger than withdrawAmount (=100)
   • validatePaymasterUserOp will be called during userOp1 validation and pass for address(this).balance (=200) is larger than withdrawAmount (=150)

     // https://github.com/eth-infinitism/account-abstraction/blob/abff2aca61a8f0934e533d0d352978055fddbd96/contracts/core/EntryPoint.sol#L98C1-L102C10
     for (uint256 i = 0; i < opslen; i++) {
         UserOpInfo memory opInfo = opInfos[i];
         (uint256 validationData, uint256 pmValidationData) = _validatePrepayment(i, ops[i], opInfo);
         _validateAccountAndPaymasterValidationData(i, validationData, pmValidationData, address(0));
     }

6. During the execution loop
   • The balance of MagicSpend is 200
   • The request fund of userOp1 and userOp2 is 100+150=250 > 200

     // https://github.com/eth-infinitism/account-abstraction/blob/abff2aca61a8f0934e533d0d352978055fddbd96/contracts/core/EntryPoint.sol#L107-L109
     for (uint256 i = 0; i < opslen; i++) {
         collected += _executeUserOp(i, ops[i], opInfos[i]);
     }

   • After executing postOp of userOp1, the balance of MagicSpend is 100.

     // https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/MagicSpend/MagicSpend.sol#L160C1-L162C10
     if (withdrawable > 0) {
         SafeTransferLib.forceSafeTransferETH(account, withdrawable, SafeTransferLib.GAS_STIPEND_NO_STORAGE_WRITES);
     }

   • When executing postOp of userOp2, MagicSpend does not has enough balance to cover the requested fund (=150) of userOp2 and the execution will fail.

     // https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/MagicSpend/MagicSpend.sol#L160C1-L162C10
     if (withdrawable > 0) {
         SafeTransferLib.forceSafeTransferETH(account, withdrawable, SafeTransferLib.GAS_STIPEND_NO_STORAGE_WRITES);
     }

   • So, Balance check during MagicSpend validation cannot ensure that MagicSpend has enough balance to cover the requested fund.

     // https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/MagicSpend/MagicSpend.sol#L130-L135
     // 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) {
         revert InsufficientBalance(withdrawAmount, address(this).balance);
     }

Recommended Mitigation Steps

Use a state instead of address(this).balance to record the remain balance.

        if (remain < withdrawAmount) {
            revert InsufficientBalance(withdrawAmount, address(this).balance);
        }
        remain -= withdrawAmount;

Assessed type

Access Control

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

This would happen only when the entrypoint batches the transactions. Additionally, this is kind of related to the known issue from readme: When acting as a paymaster, EntryPoint will debit MagicSpend slightly more than actualGasCost, meaning what is withheld on a gas-paying withdraw will not cover 100% of MagicSpend's balance decrease in the EntryPoint.

[wilsoncusack]

Ah right I feel like we discussed this @xenoliss and then perhaps forgot in later conversation. The only way to avoid would be to have validation keep some tally of all the expected coming withdraws.

[c4-sponsor]

wilsoncusack (sponsor) confirmed

[3docSec]

To be fair, one could argue that the bundlers should simulate the execution of their bundles before submission to avoid reverts; this is however a valid way of grieving the reputation of the MagicSpend contract

[c4-judge]

3docSec marked the issue as satisfactory

[wilsoncusack]

To be fair, one could argue that the bundlers should simulate the execution of their bundles before submission to avoid reverts; this is however a valid way of grieving the reputation of the MagicSpend contract

Re, it is correct that the bundler would be expected to see this and revert the whole bundle. However the point is valid that the guard is not fully satisfactory as it is written. It should probably be removed or fixed.

[c4-judge]

3docSec marked the issue as selected for report

[SovaSlava]

Bundler always check such cases - https://github.com/etherspot/skandha/blob/master/packages/executor/src/services/BundlingService/service.ts#L263-L268

Also, there is eip - https://github.com/eth-infinitism/ERCs/blob/0a3011040045ea36b551dd287d895a1187204b96/ERCS/erc-7562.md#entity-specific-rules

So, such bundle will never be even created.

[3docSec]

While I agree (and already stated before) that bundlers should be defensive wrt bundles reverting as a whole, it is true that this finding proves that the MagicSpend can transactions it won't later pay for, which is against the promise that is implicit in a successful validatePaymasterUserOp

[SovaSlava]

Bundlers make validatios offline, so the paymaster(magicSpender) should not write anything to storage - this makes no sense

[3docSec]

@SovaSlava comments in PJQA must come with factual evidence, so please refrain from making any statement about what makes sense for you.

The factual evidence is that SSTORE and SLOAD are not among the opcodes a paymaster can't use in the validation phase.

[SovaSlava]

step 3 in POC mentions that userop1 and userop2 are packed together in a bundle. Such scenario is impossible as long as the bundler follows the rules of EIP-4337. For EIP-4337 validation rules pls refer to the ERC-7562. Sources:

1. https://docs.stackup.sh/docs/validation-rules
2. https://ethereum-magicians.org/t/erc-7562-account-abstraction-validation-scope-rules/16683
3. https://github.com/eth-infinitism/account-abstraction/blob/develop/erc/ERCS/erc-7562.md

The rule EREP-010 specifically mentions the case described in POC: https://docs.stackup.sh/docs/entity-specific-rules#erep-010-paymaster-gas-limits

And you can check yourself how bundlers implement this rule: Etherspot's Skandha: https://github.com/etherspot/skandha/blob/master/packages/executor/src/services/BundlingService/service.ts#L255-L277

Alchemy's Rundler: https://github.com/alchemyplatform/rundler/blob/main/crates/pool/src/mempool/uo_pool.rs#L371-L375

Stackup's bundler: https://github.com/stackup-wallet/stackup-bundler/blob/main/pkg/modules/checks/standalone.go#L176-L209

---

What I'm trying to say is that the current implementation of the paymaster is perfectly in line with EIP-4337 and is not a bug. Paymaster can't and shouldn't validate the whole bundle.

You can argue that some bundler may not follow EREP-010 and submit a bundle that will revert, but we can make this case for any arbitrary validation rule.

[3docSec]

step 3 in POC mentions that userop1 and userop2 are packed together in a bundle. Such scenario is impossible as long as the bundler follows the rules of EIP-4337

I have no proof of this statement, no part of EIP-4337 requires a paymaster native balance check.

Such precautionary check is therefore arbitrary, and to confirm this, you can observe that the checks you linked are on the paymaster's deposit on the EntryPoint, while this finding reports insufficient native token balance to cover postOp:

          paymasterDeposit[paymaster] = await entryPointContract.balanceOf(
            paymaster
          );