Skip to content
This repository has been archived by the owner on Sep 24, 2023. It is now read-only.

IllIllI - Gas spikes after outages may prevent order execution #173

Open
sherlock-admin opened this issue Mar 25, 2023 · 1 comment
Open
Labels
Has Duplicates Medium Reward A payout will be made for this issue Sponsor Confirmed Won't Fix The sponsor confirmed this issue will not be fixed

Comments

@sherlock-admin
Copy link
Contributor

IllIllI

medium

Gas spikes after outages may prevent order execution

Summary

Users are required to specify execution fee at order creation. This fee is to reimburse the keeper for executing their order, and is based on a formula that includes the tx.price of the keeper when it executes the order.

Vulnerability Detail

If the user submits an order to exit the position, and specifies a very generous execution fee, there may be an outage in the keeper or oracle network that delays the execution of the order. When the outage is resolved, the transaction gas fees may spike because of all of the queued orders that were waiting for the network to come back (similar to a long-on storm), and or due to other protocols trying to service their own liquidations.

In such a scenario, the user's oracle price is protected for a certain amount of time, but after that window passes, the order won't be executable. The issue is that there is no way for the user to update the execution fee so that it still gets executed during the gas spike, without altering their execution price.

Impact

It's entirely possible that the provided execution fee would generally be described as excessive, at the time of order creation, but due to the outage it became insufficient. During the time window where the order isn't executed, the user's position may change from a profit, to a loss, or even become liquidated.

Code Snippet

Updating the execution fee always touches the order, which changes the update timestamp, which is used to decide which oracle prices are valid:

// File: gmx-synthetics/contracts/exchange/OrderHandler.sol : OrderHandler.   #1

88             // allow topping up of executionFee as partially filled or frozen orders
89             // will have their executionFee reduced
90             address wnt = TokenUtils.wnt(dataStore);
91             uint256 receivedWnt = orderVault.recordTransferIn(wnt);
92 @>          order.setExecutionFee(order.executionFee() + receivedWnt);
93     
94             uint256 estimatedGasLimit = GasUtils.estimateExecuteOrderGasLimit(dataStore, order);
95             GasUtils.validateExecutionFee(dataStore, estimatedGasLimit, order.executionFee());
96     
97 @>          order.touch();
98             OrderStoreUtils.set(dataStore, key, order);
99     
100            OrderEventUtils.emitOrderUpdated(eventEmitter, key, sizeDeltaUsd, triggerPrice, acceptablePrice);
101:       }

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/exchange/OrderHandler.sol#L68-L101

Tool used

Manual Review

Recommendation

Allow the user to update the execution fee without changing the order timestamp if the tx.gasprice of the update transaction is above some threshold/execution fee factor

@xvi10
Copy link

xvi10 commented Mar 29, 2023

this is a valid concern but we do not think the contracts should be changed to add this feature, instead keepers may be reimbursed through a keeper fund or the protocol may run keepers that are willing to execute the transactions even if the execution fee will not be fully reimbursed

@sherlock-admin sherlock-admin added the Reward A payout will be made for this issue label Apr 3, 2023
@xvi10 xvi10 added the Won't Fix The sponsor confirmed this issue will not be fixed label Apr 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Has Duplicates Medium Reward A payout will be made for this issue Sponsor Confirmed Won't Fix The sponsor confirmed this issue will not be fixed
Projects
None yet
Development

No branches or pull requests

2 participants