Issue when using tx.gasprice to estimate the execution fee

Summary

Inaccurate estimation of the gas fee in L2 arbitrium network

Vulnerability Detail

The user needs to pay the execution fee for the keeper to execution order and the execution is based on the current gas price of the network

gmx-synthetics\contracts\deposit\DepositUtils.sol:
  127          uint256 estimatedGasLimit = GasUtils.estimateExecuteDepositGasLimit(dataStore, deposit);
  128:         GasUtils.validateExecutionFee(dataStore, estimatedGasLimit, params.executionFee);
  129  

gmx-synthetics\contracts\exchange\OrderHandler.sol:
  94          uint256 estimatedGasLimit = GasUtils.estimateExecuteOrderGasLimit(dataStore, order);
  95:         GasUtils.validateExecutionFee(dataStore, estimatedGasLimit, order.executionFee());
  96  

gmx-synthetics\contracts\order\OrderUtils.sol:
  138          uint256 estimatedGasLimit = GasUtils.estimateExecuteOrderGasLimit(dataStore, order);
  139:         GasUtils.validateExecutionFee(dataStore, estimatedGasLimit, order.executionFee());
  140  

gmx-synthetics\contracts\withdrawal\WithdrawalUtils.sol:
  163          uint256 estimatedGasLimit = GasUtils.estimateExecuteWithdrawalGasLimit(dataStore, withdrawal);
  164:         GasUtils.validateExecutionFee(dataStore, estimatedGasLimit, params.executionFee);
  165

which calls:

    // @dev adjust the estimated gas limit to help ensure the execution fee is sufficient during
    // the actual execution
    // @param dataStore DataStore
    // @param estimatedGasLimit the estimated gas limit
    function adjustGasLimitForEstimate(DataStore dataStore, uint256 estimatedGasLimit) internal view returns (uint256) {
        uint256 baseGasLimit = dataStore.getUint(Keys.ESTIMATED_GAS_FEE_BASE_AMOUNT);
        uint256 multiplierFactor = dataStore.getUint(Keys.ESTIMATED_GAS_FEE_MULTIPLIER_FACTOR);
        uint256 gasLimit = baseGasLimit + Precision.applyFactor(estimatedGasLimit, multiplierFactor);
        return gasLimit;
    }

    // @dev the estimated gas limit for deposits
    // @param dataStore DataStore
    // @param deposit the deposit to estimate the gas limit for
    function estimateExecuteDepositGasLimit(DataStore dataStore, Deposit.Props memory deposit) internal view returns (uint256) {
        uint256 gasPerSwap = dataStore.getUint(Keys.singleSwapGasLimitKey());
        uint256 swapCount = deposit.longTokenSwapPath().length + deposit.shortTokenSwapPath().length;
        uint256 gasForSwaps = swapCount * gasPerSwap;

        if (deposit.initialLongTokenAmount() == 0 || deposit.initialShortTokenAmount() == 0) {
            return dataStore.getUint(Keys.depositGasLimitKey(true)) + deposit.callbackGasLimit() + gasForSwaps;
        }

        return dataStore.getUint(Keys.depositGasLimitKey(false)) + deposit.callbackGasLimit() + gasForSwaps;
    }

eventually the cost is determined by tx.gasprice.

Given the context that the protocol will deploy the smart contract to both arbtrium and avalanche, using tx.gasprice is not a fair way.

According to https://support.avax.network/en/articles/6169826-how-are-gas-fees-calculated

With the introduction of dynamic fees, legacy-style transactions that only have a single gas price parameter can lead to both delayed transactions and overpaying for transactions. Dynamic fee transactions are the solution! For more info, read this.

For the dynamic fee algorithm, when a block is produced or verified, we look over the past 10s to see how much gas has been consumed within that window (with an added charge for each block produced in that window) to determine the current network utilization. This window has a target utilization, which is currently set to 15M gas units. Feel like the user can steal choose to pay the low execution fee, which is bad for GMX Errr I cannot find any solid explanation of how the tx.gasprice determined in arbtrium network.

some user pays very high execution fee and some user pays low execution fee because of the usage of the tx.gasprice

The tx.gasprice can fluctuate a lot based on the metric of the business of the network.

It is possible that the user underpays the execution fee in a less busy time but the keeper has to pay additional gas fee to execute the order.

In arbtrium, the gas estimation has a different mechanism

https://developer.arbitrum.io/arbos/gas#estimating-gas

The L2 component consists of the traditional fees geth would pay to stakers in a vanilla L1 chain, such as the computation and storage charges applying the state transition function entails. ArbOS charges additional fees for executing its L2-specific precompiles, whose fees are dynamically priced according to the specific resources used while executing the call.

https://developer.arbitrum.io/arbos/gas#gas-price-floor

The L2 gas price on a given Arbitrum chain has a set floor, which can be queried via ArbGasInfo.getMinimumGasPrice (currently 0.1 gwei on Arbitrum One and 0.01 gwei on Nova).

The code does not use ArbGasinfo to check the updated L2 Gas price

https://arbiscan.io/address/0x000000000000000000000000000000000000006c#readContract

Resources:

https://github.com/OffchainLabs/nitro/blob/master/precompiles/ArbGasInfo.go

and

https://developer.arbitrum.io/arbos/common-precompiles

see ArbGasInfo

Impact

Users can heavily underpay the gas fee because of the lack of gas estimation check, especially in the L2 network, the protocol has to stipend the gas out of his own pocket to execute the order.

Code Snippet

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/gas/GasUtils.sol#L109-L145

Tool used

Manual Review

Recommendation

We recommend the protocol integrate the ArbGasInfo gas check in L2 network and just over-charge the execution fee and refund the additional fee after execution.

Duplicate of #173

sherlock-audit / 2023-02-gmx-judging

ShadowForce - Issue when using tx.gasprice to estimate the execution fee #209