Keepers can steal additional execution fee from users
The implementation of `payExecutionFee()` didn't take EIP-150 into consideration, a malicious keeper can exploit it to drain out all execution fee users have paid, regardless of the actual execution cost.\\nThe issue arises on `L55` of `payExecutionFee()`, as it's an `external` function, callingpayExecutionFee() is subject to EIP-150. Only `63/64` gas is passed to the `GasUtils` sub-contract(external library), and the remaing `1/64` gas is reserved in the caller contract which will be refunded to keeper(msg.sender) after the execution of the whole transaction. But calculation of `gasUsed` includes this portion of the cost as well.\\n```\\nFile: contracts\\gas\\GasUtils.sol\\n    function payExecutionFee(\\n        DataStore dataStore,\\n        EventEmitter eventEmitter,\\n        StrictBank bank,\\n        uint256 executionFee,\\n        uint256 startingGas,\\n        address keeper,\\n        address user\\n    ) external { // @audit external call is subject to EIP// Remove the line below\\n150\\n// Remove the line below\\n        uint256 gasUsed = startingGas // Remove the line below\\n gasleft();\\n// Add the line below\\n            uint256 gasUsed = startingGas // Remove the line below\\n gasleft() * 64 / 63; // @audit the correct formula\\n        uint256 executionFeeForKeeper = adjustGasUsage(dataStore, gasUsed) * tx.gasprice;\\n\\n        if (executionFeeForKeeper > executionFee) {\\n            executionFeeForKeeper = executionFee;\\n        }\\n\\n        bank.transferOutNativeToken(\\n            keeper,\\n            executionFeeForKeeper\\n        );\\n\\n        emitKeeperExecutionFee(eventEmitter, keeper, executionFeeForKeeper);\\n\\n        uint256 refundFeeAmount = executionFee // Remove the line below\\n executionFeeForKeeper;\\n        if (refundFeeAmount == 0) {\\n            return;\\n        }\\n\\n        bank.transferOutNativeToken(\\n            user,\\n            refundFeeAmount\\n        );\\n\\n        emitExecutionFeeRefund(eventEmitter, user, refundFeeAmount);\\n    }\\n```\\n\\nA malicious keeper can exploit this issue to drain out all execution fee, regardless of the actual execution cost. Let's take `executeDeposit()` operation as an example to show how it works:\\n```\\nFile: contracts\\exchange\\DepositHandler.sol\\n    function executeDeposit(\\n        bytes32 key,\\n        OracleUtils.SetPricesParams calldata oracleParams\\n    ) external\\n        globalNonReentrant\\n        onlyOrderKeeper\\n        withOraclePrices(oracle, dataStore, eventEmitter, oracleParams)\\n    {\\n        uint256 startingGas = gasleft();\\n\\n        try this._executeDeposit(\\n            key,\\n            oracleParams,\\n            msg.sender\\n        ) {\\n        } catch (bytes memory reasonBytes) {\\n// rest of code\\n        }\\n    }\\n\\nFile: contracts\\exchange\\DepositHandler.sol\\n    function _executeDeposit(\\n        bytes32 key,\\n        OracleUtils.SetPricesParams memory oracleParams,\\n        address keeper\\n    ) external onlySelf {\\n        uint256 startingGas = gasleft();\\n// rest of code\\n\\n        ExecuteDepositUtils.executeDeposit(params);\\n    }\\n\\n\\nFile: contracts\\deposit\\ExecuteDepositUtils.sol\\n    function executeDeposit(ExecuteDepositParams memory params) external {\\n// rest of code\\n\\n        GasUtils.payExecutionFee(\\n            params.dataStore,\\n            params.eventEmitter,\\n            params.depositVault,\\n            deposit.executionFee(),\\n            params.startingGas,\\n            params.keeper,\\n            deposit.account()\\n        );\\n    }\\n\\nFile: contracts\\gas\\GasUtils.sol\\n    function payExecutionFee(\\n        DataStore dataStore,\\n        EventEmitter eventEmitter,\\n        StrictBank bank,\\n        uint256 executionFee,\\n        uint256 startingGas,\\n        address keeper,\\n        address user\\n    ) external {\\n        uint256 gasUsed = startingGas - gasleft();\\n        uint256 executionFeeForKeeper = adjustGasUsage(dataStore, gasUsed) * tx.gasprice;\\n\\n        if (executionFeeForKeeper > executionFee) {\\n            executionFeeForKeeper = executionFee;\\n        }\\n\\n        bank.transferOutNativeToken(\\n            keeper,\\n            executionFeeForKeeper\\n        );\\n\\n        emitKeeperExecutionFee(eventEmitter, keeper, executionFeeForKeeper);\\n\\n        uint256 refundFeeAmount = executionFee - executionFeeForKeeper;\\n        if (refundFeeAmount == 0) {\\n            return;\\n        }\\n\\n        bank.transferOutNativeToken(\\n            user,\\n            refundFeeAmount\\n        );\\n\\n        emitExecutionFeeRefund(eventEmitter, user, refundFeeAmount);\\n    }\\n\\nFile: contracts\\gas\\GasUtils.sol\\n    function adjustGasUsage(DataStore dataStore, uint256 gasUsed) internal view returns (uint256) {\\n// rest of code\\n        uint256 baseGasLimit = dataStore.getUint(Keys.EXECUTION_GAS_FEE_BASE_AMOUNT);\\n// rest of code\\n        uint256 multiplierFactor = dataStore.getUint(Keys.EXECUTION_GAS_FEE_MULTIPLIER_FACTOR);\\n        uint256 gasLimit = baseGasLimit + Precision.applyFactor(gasUsed, multiplierFactor);\\n        return gasLimit;\\n    }\\n```\\n\\nTo simplify the problem, given\\n```\\nEXECUTION_GAS_FEE_BASE_AMOUNT = 0\\nEXECUTION_GAS_FEE_MULTIPLIER_FACTOR = 1\\nexecutionFeeUserHasPaid = 200K Gwei\\ntx.gasprice = 1 Gwei\\nactualUsedGas = 100K\\n```\\n\\n`actualUsedGas` is the gas cost since startingGas(L146 of DepositHandler.sol) but before calling payExecutionFee()(L221 of ExecuteDepositUtils.sol)\\nLet's say, the keeper sets `tx.gaslimit` to make\\n```\\nstartingGas = 164K\\n```\\n\\nThen the calculation of `gasUsed`, L55 of `GasUtils.sol`, would be\\n```\\nuint256 gasUsed = startingGas - gasleft() = 164K - (164K - 100K) * 63 / 64 = 101K\\n```\\n\\nand\\n```\\nexecutionFeeForKeeper = 101K * tx.gasprice = 101K * 1 Gwei = 101K Gwei\\nrefundFeeForUser = 200K - 101K = 99K Gwei\\n```\\n\\nAs setting of `tx.gaslimit` doesn't affect the actual gas cost of the whole transaction, the excess gas will be refunded to `msg.sender`. Now, the keeper increases `tx.gaslimit` to make `startingGas = 6500K`, the calculation of `gasUsed` would be\\n```\\nuint256 gasUsed = startingGas - gasleft() = 6500K - (6500K - 100K) * 63 / 64 = 200K\\n```\\n\\nand\\n```\\nexecutionFeeForKeeper = 200K * tx.gasprice = 200K * 1 Gwei = 200K Gwei\\nrefundFeeForUser = 200K - 200K = 0 Gwei\\n```\\n\\nWe can see the keeper successfully drain out all execution fee, the user gets nothing refunded.
The description in `Vulnerability Detail` section has been simplified. In fact, `gasleft` value should be adjusted after each external call during the whole call stack, not just in `payExecutionFee()`.
Keepers can steal additional execution fee from users.
```\\nFile: contracts\\gas\\GasUtils.sol\\n    function payExecutionFee(\\n        DataStore dataStore,\\n        EventEmitter eventEmitter,\\n        StrictBank bank,\\n        uint256 executionFee,\\n        uint256 startingGas,\\n        address keeper,\\n        address user\\n    ) external { // @audit external call is subject to EIP// Remove the line below\\n150\\n// Remove the line below\\n        uint256 gasUsed = startingGas // Remove the line below\\n gasleft();\\n// Add the line below\\n            uint256 gasUsed = startingGas // Remove the line below\\n gasleft() * 64 / 63; // @audit the correct formula\\n        uint256 executionFeeForKeeper = adjustGasUsage(dataStore, gasUsed) * tx.gasprice;\\n\\n        if (executionFeeForKeeper > executionFee) {\\n            executionFeeForKeeper = executionFee;\\n        }\\n\\n        bank.transferOutNativeToken(\\n            keeper,\\n            executionFeeForKeeper\\n        );\\n\\n        emitKeeperExecutionFee(eventEmitter, keeper, executionFeeForKeeper);\\n\\n        uint256 refundFeeAmount = executionFee // Remove the line below\\n executionFeeForKeeper;\\n        if (refundFeeAmount == 0) {\\n            return;\\n        }\\n\\n        bank.transferOutNativeToken(\\n            user,\\n            refundFeeAmount\\n        );\\n\\n        emitExecutionFeeRefund(eventEmitter, user, refundFeeAmount);\\n    }\\n```\\n