M-01 MitigationConfirmed

[c4-bot-7]

Comments

MagicSpend is a contract that allows on-chain accounts to present valid Withdraw Requests and receive funds.

The withdraw signature are obtained off-chain by providing fiat money or other tokens that the user can exchange for this prepaid ETH amount.

This withdraw request signature can be presented to the contract in two ways as :

1. a direct account/SWC call to MagicSpend#withdraw method. In this way the user pays gas fees to get the already prepaid ETH amount

2. MagicSpend contract can be also used as a erc-4337 EntryPoint v0.6 compliant paymaster. As a paymaster it allows the user/SWC account to get ETH without the need to already have the necessary native gas to spend for the transaction.

Vulnerability details

The guard that is supposed to protect the MagicSpend when used as a paymaster from accounts withdrawing when there is not enough native token available is not working correctly.

It can happen that the validatePaymasterUserOp says it can pay for a valid withdraw request but next fail on postOp where the payment to the account is finally made.

When the bundler adds UserOperation for MagicSend by successfully simulate the necessary transaction off-chain a revert of insufficient native token balance can still happen on-chain and can damage the reputation of MagicSend as a paymaster.

This revert can be a consequence of allowing in the same transaction to have other ways to deplete the MagicSend contract funds which are impossible to predict on time when the bundler bundles chosen UserOperation.

Mitigation

The MagicSend contract can deplete its native funds in the same transaction with valid withdraw request signatures send trough the EntryPoint as a paymaster and/or the call to withdraw method. Making the first mitigation PR16 as an end solution unfeasible.

The second mitigation PR17 tries to fix this problem by providing the owner of MagicSpend a tool to probabilistically limit the possible amount to withdraw within the same transactions. The MagicSpend owner must carefully monitor transactions to set an appropriate value for the maxWithdrawDenominator which regulates the amount of native funds moved out of the contract.

By using this kind of budgeting it lowers the probability of having too little funds available when withdraws happen in the same transaction but as mentioned in the fix comment : this doesn't entirely solve the issue, and the efficacy depends the value chosen and usage.

Suggestions

Here are 4 options I can provide to hopefully ease this issue: (the first two are the most promising)

1. The following idea has two goals. To minimize non reverting postOp operations and to facilitate the work imposed on the MagicSpend owner.

I would suggest a merge of both mentioned PRs and a reiteration of the denominator idea in a way that allows the owner to chose how much budget a direct method withdrawing has and leaving the rest of the balance to be used as a in paymaster mode only.

This could be achieved by having a more precise budgeting without using a denominator calculation but a fixed budget value so when the account choose to direct withdraw we can have something like this:

+   public maxDirectWithdrawBudget;
+   public currentDirectWithdrawBudget;

    function withdraw(WithdrawRequest memory withdrawRequest) external {
        _validateRequest(msg.sender, withdrawRequest);


        if (!isValidWithdrawSignature(msg.sender, withdrawRequest)) {
            revert InvalidSignature();
        }


        if (block.timestamp > withdrawRequest.expiry) {
            revert Expired();
        }

+       if (withdrawRequest.amount > currentDirectWithdrawBudget) {
+           revert OufOfWithdrawBudget();
+       }
+
+       currentDirectWithdrawBudget -= withdrawRequest.amount;
+
+       if (currentDirectWithdrawBudget <= LOW_MAXWITHDRAW_BUDGET) {
+           emit LowOnMaxWithdrawBudget(block.timestamp, maxDirectWithdrawBudget);
+       }

        // reserve funds for gas, will credit user with difference in post op
        _withdraw(withdrawRequest.asset, msg.sender, withdrawRequest.amount);
    }

When first set the currentDirectWithdrawBudget is equal to maxDirectWithdrawBudget.

Now inside validatePaymasterUserOp we can take in account this maxDirectWithdrawBudget which is solely used as a concrete budget for direct withdrawing and reconsider PR16 when checking if we have enough token balance.

The MagicSpend owner gets a bonus of a easier work of monitoring for low token balance. And the bundler should now work fine as a isolated process of funds depletion.

2. Another interesting idea/option is to add an un/locking type of functionality as an extra step for user/account wanting to directly withdraw:

I saw this 1, 2 as a reference implementation for the next idea

For example: a account decides to use the direct withdrawing functionality and first calls unlockDirectWithdrawing method which "sets" this account to direct withdraw mode only and for the same account to be used with a paymaster it (the SWC) must first call lockDirectWithdrawing.

An important aspect of this option is that withdrawing cannot be called in the same block as unlockDirectWithdrawing, but for withdrawing to work unlock must be called a block prior.

In paymaster mode we can check that an account is not locked as withdrawing only by only checking a mapping (!not by using block.number! which is banned) and that the contract has enough funds by reconsider PR16 then we can proceed sending the funds.

Aa a note: default account/SWC status is paymaster mode (or locked withdrawals) so accounts don't need to speed gas on calling lockDirectWithdrawing.

3. Instead of using signatures use a token balance mapping for accounts/SWC but in this case there must be more work done when an allowed balance expires. Plus in this mode the contract should get/have necessary funds deposited in the (almost) same time a account balance is created off-chain. But the signature solution is obviously a more elegant one to pursue.

4. There is another option to just remove the direct withdraw method but I don't think is the right idea of MagicSpend contract

Notes

I marked this issue as mitigated becouse the provided solutions was good enough. But if a extra call step is not a problem I would chose the second suggested option of locking direct withdraws as a possible alternative.

Conclusions

Partial Mitigation (MagicSpend owner must pay extra attention to control funds depletion)

[PaperParachute]

Edited on wardens behalf due to a confirmation issue.

[c4-judge]

3docSec marked the issue as satisfactory

[c4-judge]

3docSec marked the issue as confirmed for report