Denial of Service Due to Underflow Error in MagicSpend

[c4-bot-1]

Lines of code

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

Vulnerability details

Impact

Denial of Service Due to Underflow Error in postOp(...) function in MagicSpend contract

Proof of Concept

   function postOp(IPaymaster.PostOpMode mode, bytes calldata context, uint256 actualGasCost)
        external
        onlyEntryPoint
    {
        // `PostOpMode.postOpReverted` should be impossible.
        // Only possible cause would be if this contract does not own enough ETH to transfer
        // but this is checked at the validation step.
        assert(mode != PostOpMode.postOpReverted);

>>>        (uint256 maxGasCost, address account) = abi.decode(context, (uint256, address));

        // Compute the total remaining funds available for the user accout.
        // NOTE: Take into account the user operation gas that was not consummed.
>>>        uint256 withdrawable = _withdrawableETH[account] + (maxGasCost - actualGasCost);

        // Send the all remaining funds to the user accout.
        delete _withdrawableETH[account];
        if (withdrawable > 0) {
            SafeTransferLib.forceSafeTransferETH(account, withdrawable, SafeTransferLib.GAS_STIPEND_NO_STORAGE_WRITES);
        }
    }

The function above shows how postOp(...) function is implemented in the MagicSpend contract. As noted from the pointer a subtraction operation is carried out between maxGasCost and actualGasCost, depending on the external contract calling this function, whenever actualGasCost is greater than maxGasCost which is derived from context decode, there would be denial of service due to underflow error, there by reverting.

Tools Used

Manual Review

Recommended Mitigation Steps

Protocol should adjust postOp(...) function such that when actualGasCost is greater than maxGasCost, it is reassigned the maxGasCost value to prevent Denial of Service to any external contract calling this function, as provided below.

   function postOp(IPaymaster.PostOpMode mode, bytes calldata context, uint256 actualGasCost)
        external
        onlyEntryPoint
    {
        // `PostOpMode.postOpReverted` should be impossible.
        // Only possible cause would be if this contract does not own enough ETH to transfer
        // but this is checked at the validation step.
        assert(mode != PostOpMode.postOpReverted);

        (uint256 maxGasCost, address account) = abi.decode(context, (uint256, address));
+++     if ( actualGasCost > maxGasCost ) {
+++       actualGasCost = maxGasCost;
+++     }
        // Compute the total remaining funds available for the user accout.
        // NOTE: Take into account the user operation gas that was not consummed.
        uint256 withdrawable = _withdrawableETH[account] + (maxGasCost - actualGasCost);

        // Send the all remaining funds to the user accout.
        delete _withdrawableETH[account];
        if (withdrawable > 0) {
            SafeTransferLib.forceSafeTransferETH(account, withdrawable, SafeTransferLib.GAS_STIPEND_NO_STORAGE_WRITES);
        }
    }

Assessed type

Under/Overflow

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #99

[raymondfam]

See #99.

[c4-judge]

3docSec marked the issue as unsatisfactory: Invalid