0xdeadbeef - Insufficiant gas calculations when paying execution cost to keeper

[sherlock-admin]

0xdeadbeef

high

Insufficiant gas calculations when paying execution cost to keeper

Summary

The protocol calculates the execution fees it should send the the keeper and then returns any additional funds to the user.

However, the calculations do not include the operation of sending the funds themselves.
The sending operation is currently not deterministic and can result in large gas consumption which will be paid by the keeper without compensation

Vulnerability Detail

After executions of deposits/withdrawals/orders, the protocol pays the keeper the execution cost through payExecutionFee

    function payExecutionFee(
        DataStore dataStore,
        EventEmitter eventEmitter,
        StrictBank bank,
        uint256 executionFee,
        uint256 startingGas,
        address keeper,
        address user
    ) external {
        uint256 gasUsed = startingGas - gasleft();
        uint256 executionFeeForKeeper = adjustGasUsage(dataStore, gasUsed) * tx.gasprice;
        if (executionFeeForKeeper > executionFee) {
            executionFeeForKeeper = executionFee;
        }
        bank.transferOutNativeToken(
            keeper,
            executionFeeForKeeper
        );
        emitKeeperExecutionFee(eventEmitter, keeper, executionFeeForKeeper);
        uint256 refundFeeAmount = executionFee - executionFeeForKeeper;
        if (refundFeeAmount == 0) {
            return;
        }
        bank.transferOutNativeToken(
            user,
            refundFeeAmount
        );
        emitExecutionFeeRefund(eventEmitter, user, refundFeeAmount);
    }

The executionFeeForKeeper is calculated by the gas used and additional factors defined in adjustGasUsage:

    function adjustGasUsage(DataStore dataStore, uint256 gasUsed) internal view returns (uint256) {
        // gas measurements are done after the call to withOraclePrices
        // withOraclePrices may consume a significant amount of gas
        // the baseGasLimit used to calculate the execution cost
        // should be adjusted to account for this
        // additionally, a transaction could fail midway through an execution transaction
        // before being cancelled, the possibility of this additional gas cost should
        // be considered when setting the baseGasLimit
        uint256 baseGasLimit = dataStore.getUint(Keys.EXECUTION_GAS_FEE_BASE_AMOUNT);
        // the gas cost is estimated based on the gasprice of the request txn
        // the actual cost may be higher if the gasprice is higher in the execution txn
        // the multiplierFactor should be adjusted to account for this
        uint256 multiplierFactor = dataStore.getUint(Keys.EXECUTION_GAS_FEE_MULTIPLIER_FACTOR);
        uint256 gasLimit = baseGasLimit + Precision.applyFactor(gasUsed, multiplierFactor);
        return gasLimit;
    }

As can be seen, the gas price is adjusted to account for withOraclePrices gas consumption but is not adjusted to include the execution of the bank.transferOutNativeToken of the fees and refund.

bank.transferOutNativeToken will call withdrawAndSendNativeToken and can consume a lot of gas for native transfers and for non-native transfers (when failed)
It first attempts to transfer native token unwrapped and caps the gas consumption to NATIVE_TOKEN_TRANSFER_GAS_LIMIT (currently 200_000 gas) and if it fails it will wrap the native token and transform it using ERC20 transfer function.

    function withdrawAndSendNativeToken(
        DataStore dataStore,
        address _wnt,
        address receiver,
        uint256 amount
    ) internal {
        if (amount == 0) { return; }
        AccountUtils.validateReceiver(receiver);
        IWNT(_wnt).withdraw(amount);
        uint256 gasLimit = dataStore.getUint(Keys.NATIVE_TOKEN_TRANSFER_GAS_LIMIT);
        bool success;
        // use an assembly call to avoid loading large data into memory
        // input mem[in…(in+insize)]
        // output area mem[out…(out+outsize))]
        assembly {
            success := call(
                gasLimit, // gas limit
                receiver, // receiver
                amount, // value
                0, // in
                0, // insize
                0, // out
                0 // outsize
            )
        }
        if (success) { return; }
        // if the transfer failed, re-wrap the token and send it to the receiver
        depositAndSendWrappedNativeToken(
            dataStore,
            receiver,
            amount
        );
    }

Consider a user that consumes more then 200_000 gas when receiving the refund. This will make the keeper pay a large amount of gas for the attempted transfer and for the wrapping and transfer of ERC20.

Impact

Loss of funds for keeper

Code Snippet

https://github.com/sherlock-audit/2023-04-gmx/blob/main/gmx-synthetics/contracts/gas/GasUtils.sol#L97

Tool used

Manual Review

Recommendation

The main issue is that it is not deterministic how much gas withdrawAndSendNativeToken will consume
To make it deterministic I suggest:

1. If target is not EOA (contract code exists), transfer natively by creating a self-destructing contract. This will not triggered any receiving logic which will not waste un-calculated gas.

contract TransferFunds {
    constructor(address target) payable {
        selfdestruct(payable(target));
    }
}

2. if target is EOA, use the CALL opcode to transfer funds.

With the above recommendations, the gas can be calculated in a deterministic way and included in adjustGasUsage adjustments

[0xdeadbeef0x]

Escalate for 10 USDC

Not sure why this is excluded. This issue shows how keepers will not be paid enough for their job.
In previous contest, these types of issues were valid:
sherlock-audit/2023-02-gmx-judging#195
sherlock-audit/2023-02-gmx-judging#212

Happy to answer any questions

[sherlock-admin]

Escalate for 10 USDC

Not sure why this is excluded. This issue shows how keepers will not be paid enough for their job.
In previous contest, these types of issues were valid:
sherlock-audit/2023-02-gmx-judging#195
sherlock-audit/2023-02-gmx-judging#212

Happy to answer any questions

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[IllIllI000]

This is part of the base fee of the order. The code block above even states this:

        // additionally, a transaction could fail midway through an execution transaction
        // before being cancelled, the possibility of this additional gas cost should
        // be considered when setting the baseGasLimit

Invalid

[hrishibhat]

Result:
Invalid
Unique
This is a non-issue as pointed out in the above comment by Lead Watson.

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• 0xdeadbeef0x: rejected