IllIllI - Swaps associated with position orders will use the wrong price

[sherlock-admin]

IllIllI

high

Swaps associated with position orders will use the wrong price

Summary

Swaps of tokens gained via position orders will use the wrong price as the latest price

Vulnerability Detail

In the previous iteration of the code, the getLatestPrice() function returned the secondary price, and fell back to the primary price if the secondary price didn't exist. In the current version of the code, getLatestPrice() returns the custom price if one exists, which may be the trigger price or the maximized price. This price is used not only for the execution of the order, but also now for the swaps of the tokens after the order executes.

Impact

If, for example, the order is a market increase order, the custom price is set to the maximized price for the execution which means the liquidity taker got fewer shares than the maker gave. When those shares are swapped, that maximized price is still used, whereas if the swap had been done as a separate order, no custom price would be consulted. The two methods of doing swaps get different prices, which leads to arbitrage opportunities.

Code Snippet

Normal swap orders never have an exact price set:

// File: gmx-synthetics/contracts/order/BaseOrderUtils.sol : BaseOrderUtils.setExactOrderPrice()   #1

203        function setExactOrderPrice(
204            Oracle oracle,
205            address indexToken,
206            Order.OrderType orderType,
207            uint256 triggerPrice,
208            bool isLong
209        ) internal {
210            if (isSwapOrder(orderType)) {
211 @>             return;
212:           }

https://github.com/sherlock-audit/2023-04-gmx/blob/main/gmx-synthetics/contracts/order/BaseOrderUtils.sol#L203-L212

but swaps after the order will use the price set during the position-altering portion of the order:

// File: gmx-synthetics/contracts/swap/SwapUtils.sol : SwapUtils._swap()   #2

178        function _swap(SwapParams memory params, _SwapParams memory _params) internal returns (address, uint256) {
179            SwapCache memory cache;
180    
181            if (_params.tokenIn != _params.market.longToken && _params.tokenIn != _params.market.shortToken) {
182                revert Errors.InvalidTokenIn(_params.tokenIn, _params.market.marketToken);
183            }
184    
185            MarketUtils.validateSwapMarket(_params.market);
186    
187            cache.tokenOut = MarketUtils.getOppositeToken(_params.tokenIn, _params.market);
188 @>         cache.tokenInPrice = params.oracle.getLatestPrice(_params.tokenIn);
189:@>         cache.tokenOutPrice = params.oracle.getLatestPrice(cache.tokenOut);

https://github.com/sherlock-audit/2023-04-gmx/blob/main/gmx-synthetics/contracts/swap/SwapUtils.sol#L178-L189

Tool used

Manual Review

Recommendation

Introduce a flag to the getLatestPrice() function, indicating whether to use the custom price if it exists

[xvi10]

fixed in gmx-io/gmx-synthetics@3243138

the previous logic of allowing multiple token types led to logical issues and unexpected pricing differences

the code was updated to use a single (min, max) price value per token instead for each transaction