`withdraw()` Non-ETH token is not supported by bundler + entry point

[c4-bot-1]

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#L181 https://github.com/code-423n4/2024-03-coinbase/blob/main/src/MagicSpend/MagicSpend.sol#L334

Vulnerability details

Impact

Non-ETH tokens are not supported in withdraw() when called through bundler + entry point.

Proof of Concept

Based on sponsor, withdraw() can be used in two ways.

1. Direct usage by the SCW owner through the execute() function, enabling a call to withdraw() with a valid withdrawRequest.
2. Submitting an ERC-4337 UserOperation to the bundler, where withdraw() is executed within the transaction flow.

Current UI on Coinbase.com ONLY supports withdraw() through bundler + entry point per sponsor.

This report particularly focuses on scenario 2, where withdraw() is processed through the bundler + entry point.

It's important to highlight that the withdraw() function is designed to support both ETH and other valid tokens, as evident from the _withdraw() implementation.

    function _withdraw(address asset, address to, uint256 amount) internal {
        if (asset == address(0)) {
            SafeTransferLib.safeTransferETH(to, amount);
        } else {
            SafeTransferLib.safeTransfer(asset, to, amount);
        }
    }

When utilizing the bundler + entry point, the process unfolds as follows:

1. The user submits an ERC-4337 UserOperation(withdraw()) 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 triggers validatePaymasterUserOp() on MagicSpend/PayMaster. Within this function, it examines whether withdrawRequest.asset != address(0). Given that address(0) denotes ETH, any non-ETH token will trigger a revert condition.

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

Tools Used

Manual Review

Recommended Mitigation Steps

Consider refactoring validatePaymasterUserOp() to 2 scenarios. Scenario 1 handles ETH Scenario 2 handles withdraw of any Non-ETH token.

Assessed type

Error

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #102

[raymondfam]

On top of the comment on #102, the contract has already been so designed to handle non-ETH assets via withdraw().

[3docSec]

This report describes a missing feature without a clear argument on what is the impact of not having it.

[c4-judge]

3docSec marked the issue as unsatisfactory: Insufficient proof

[Henrychang26]

Thanks for judging @3docSec ,

I confirmed that withdraw() is meant to be called in execution through bundler + entry point, by the documentation bot and information provided by @raymondfam (who was responsible for answering questions throughout the contest).

The sponsor also verified two methods for utilizing the withdraw() function:

1. Users can directly trigger execute() on SCW to initiate withdraw().
2. withdraw() is invoked during execution through bundler + entry point.

Method 1: Owners have the option to directly call withdraw() without relying on bundler + entry point. However, this necessitates the owner to request a withdrawal signature from Coinbase.com (userOp(withdrawRequest) to bundler), then scan the mem-pool for the required withdrawRequest input parameter in withdraw(). Subsequently, the owner can use execute() to invoke withdraw() on SCW. Although functional, this approach is suboptimal and undermines the purpose of SCW.

Method 2: Attempting to use withdraw() with bundler + entry point will result in a revert, as it exclusively supports ETH in validatePaymasterUserOp(). It's worth mentioning that Coinbase.com permits users to purchase various tokens beyond ETH and it's unreasonable to assume Non-ETH tokens are not supported.

Based on the current implementation, withdraw() is NOT compatible with bundler + entry point at all. withdraw() ETH will revert (see #102 for the reason why it reverts), and non-ETH tokens are not supported.

I would argue this is a design flaw rather than missing feature. Maybe the sponsor can take a look?

Side note: this is not a dupe of #102 which talks about a different issue.

[3docSec]

Hi @Henrychang26, I see your point, it makes sense, but still the limitation is on use cases that don’t have to be supported for the wallet’s core functionality that is to function as an ERC-4337 compatible account abstraction tool.

[Henrychang26]

@3docSec

I agree this is limitation on use cases and does not impact wallet's core functionality. This report was submitted based on the grounds that, withdraw() is designed to be used with bundler + entry point according multiple sources including README and it does not work as intended.

I have no further information to add. I believe this deserves at least QA if not valid medium severity. I respect whatever your decision is.

Thank you!