IllIllI - Limit orders are unnecessarily delayed by a block

[sherlock-admin]

IllIllI

medium

Limit orders are unnecessarily delayed by a block

Summary

Limit orders are delayed by a block due to oracle block number restrictions, in combination with requiring ascending/descending prices. Limit orders have special logic to ensure that they're only triggered when the price approaches the trigger price. To ensure the two prices (the primary price and secondary price) used for determining the price direction are not prices from before the order is placed, the oracle code requires that oracle price block numbers all come after the block where the order was placed.

Vulnerability Detail

Rather than requiring that only the secondary (later endpoint) price comes after the block in which the order was placed, both the secondary and the primary price are required to come after the order block.

Impact

Consider the case where a user wants to set a stoploss at a price of 100, and they submit an order at the indicated block/price:

block range : primary/secondary prices
  [1,2]     :       110
  [2,3]     :       100   <--
  [4,5]     :        60

An order submitted at the indicated block would be unable to be filled using the prices at block range [2,3], and instead would be forced to use [4,5] block range. This on its own is not an issue, but if the order is combined with a swap whose price impact penalty in [2,3] is very small, but is very large in [4,5], the user will be understandably unhappy about the order execution.

Code Snippet

All oracle prices are required to come after the order timestamp, including both the position and the swap prices:

// File: gmx-synthetics/contracts/order/DecreaseOrderUtils.sol : DecreaseOrderUtils.validateOracleBlockNumbers()   #1

139            if (
140                orderType == Order.OrderType.LimitDecrease ||
141                orderType == Order.OrderType.StopLossDecrease
142            ) {
143                uint256 latestUpdatedAtBlock = orderUpdatedAtBlock > positionIncreasedAtBlock ? orderUpdatedAtBlock : positionIncreasedAtBlock;
144                if (!minOracleBlockNumbers.areGreaterThan(latestUpdatedAtBlock)) {
145                    OracleUtils.revertOracleBlockNumbersAreSmallerThanRequired(minOracleBlockNumbers, latestUpdatedAtBlock);
146                }
147                return;
148:           }

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/order/DecreaseOrderUtils.sol#L134-L156

// File: gmx-synthetics/contracts/order/IncreaseOrderUtils.sol : IncreaseOrderUtils.validateOracleBlockNumbers()   #2

105 @>         if (orderType == Order.OrderType.LimitIncrease) {
106                console.log("orderUpdatedAtBlock", orderUpdatedAtBlock);
107                console.log("positionIncreasedAtBlock", positionIncreasedAtBlock);
108                console.log("minOracleBlockNumbers", minOracleBlockNumbers[0]);
109                uint256 laterBlock = orderUpdatedAtBlock > positionIncreasedAtBlock ? orderUpdatedAtBlock : positionIncreasedAtBlock;
110                if (!minOracleBlockNumbers.areGreaterThan(laterBlock)) {
111 @>                 OracleUtils.revertOracleBlockNumbersAreSmallerThanRequired(minOracleBlockNumbers, laterBlock);
112                }
113                return;
114            }
115    
116            BaseOrderUtils.revertUnsupportedOrderType();
117:       }

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/order/IncreaseOrderUtils.sol#L98-L117

The same is true for liquidations and swaps.

Tool used

Manual Review

Recommendation

Allow a price that is separate from the primary and secondary prices, to come before the order block, and allow the primary/secondary prices to equal the order block

[xvi10]

Fix in https://github.com/gmx-io/gmx-synthetics/pull/111