`withdrawGasExcess()` does not check against `withdrawRequest.expiry`

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-03-coinbase/blob/main/src/MagicSpend/MagicSpend.sol#L109 https://github.com/code-423n4/2024-03-coinbase/blob/main/src/MagicSpend/MagicSpend.sol#L169

Vulnerability details

Impact

The WithdrawRequest.expiry parameter is not being enforced in the withdrawGasExcess() function of the Magic Spend smart contract, leading to a violation of an invariant.

Proof of Concept

When the withdrawing account is an ERC-4337 compliant smart contract (like Smart Wallet), there are three different ways the Magic Spend smart contract can be used:

1. Pay gas only
2. Transfer funds during execution only
3. Pay gas and transfer funds during execution

Users can leverage assets from CoinBase.com as if they are on-chain, allowing them to cover gas costs and withdraw funds for various operations.

This report focuses specifically on the scenario "Transfer funds during execution only" and "Pay gas and transfer funds during execution", where user calls withdrawGasExcess() in execution.

1. The user submits an ERC-4337 UserOperation to the bundler.
2. The bundler simulates validation of the UserOperation to ensure it can pay for its execution and includes it in a transaction.
3. The EntryPoint calls validateUserOp() with Smart Contract Wallet (SCW), then decreases PayMaster deposits.
4. EntryPoint invokes validatePaymasterUserOp() on MagicSpend/PayMaster. In this function, _validateRequest() is triggered, flipping the corresponding _nonceUsed mapping to true. It also checks for isValidWithdrawSignature().

    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);

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

        _withdrawableETH[userOp.sender] += withdrawAmount - maxCost;
        context = abi.encode(maxCost, userOp.sender);
    }

EntryPoint then executes UserOperation.callData on SCW, which invokes withdrawGasExcess() on MagicSpend.sol. No parameter is passed in here. Two scenarios here:

1. User receives ETH during execution, and entry point proceed to call postOp().
2. User receives ETH then perform other arbitrary operations. postOp() and rest of steps are triggered to complete the transaction.

    function withdrawGasExcess() external {
        uint256 amount = _withdrawableETH[msg.sender];
        // we could allow 0 value transfers, but prefer to be explicit
        if (amount == 0) revert NoExcess();
        delete _withdrawableETH[msg.sender];
        _withdraw(address(0), msg.sender, amount);
    }

However, throughout this transaction, there is no enforcement of the WithdrawRequest.expiry. In validatePaymasterUserOp(), only a valid nonce and signature are checked. Consequently, withdrawGasExcess() transfers ETH without additional checks, violating the invariant:

A WithdrawRequest cannot be used past WithdrawRequest.expiry.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider passing WithdrawRequest into withdrawGasExcess() as parameter.

-   function withdrawGasExcess() external {
+   function withdrawGasExcess(WithdrawRequest memory withdrawRequest) external {
+       if(block.timestamp > withdrawRequest.expiry) revert();
        uint256 amount = _withdrawableETH[msg.sender];
        if (amount == 0) revert NoExcess();
        delete _withdrawableETH[msg.sender];
        _withdraw(address(0), msg.sender, amount);
    }

Assessed type

Context

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #41

[raymondfam]

No runnable proof was given as in #41. Additionally, expiry is supposed to be imposed during validatePaymasterUserOp(), and not on withdrawGasExcess().

[c4-pre-sort]

raymondfam marked the issue as duplicate of #167

[c4-judge]

3docSec marked the issue as not a duplicate

[c4-judge]

3docSec marked the issue as primary issue

[3docSec]

The MagiSpend does not indeed check for signature expiry, but returns the info to the EntryPoint that performs the check:

File: MagicSpend.sol
109:     function validatePaymasterUserOp(UserOperation calldata userOp, bytes32, uint256 maxCost)
110:         external
111:         onlyEntryPoint
112:         returns (bytes memory context, uint256 validationData)
---
128:         validationData = (sigFailed ? 1 : 0) | (uint256(withdrawRequest.expiry) << 160);
129: 

File: EntryPoint.sol
481:         if (outOfTimeRange) {
482:             revert FailedOp(opIndex, "AA32 paymaster expired or not due");
483:         }
484:     }
486:     function _getValidationData(uint256 validationData) internal view returns (address aggregator, bool outOfTimeRange) {
487:         if (validationData == 0) {
488:             return (address(0), false);
489:         }
490:         ValidationData memory data = _parseValidationData(validationData);
491:         // solhint-disable-next-line not-rely-on-time
492:         outOfTimeRange = block.timestamp > data.validUntil || block.timestamp < data.validAfter;
493:         aggregator = data.aggregator;
494:     }

It's worth noting that this very verification is proven to happen as expected by the expectRevert in the PoC of #167 .

[c4-judge]

3docSec marked the issue as unsatisfactory: Invalid