IllIllI - Virtual swap balances don't take into account token prices

[sherlock-admin]

IllIllI

medium

Virtual swap balances don't take into account token prices

Summary

Virtual swap balances don't take into the fact that an exchange between the collateral token and the virtual token is taking place, necessitating an exchange rate.

Vulnerability Detail

Virtual position impacts are all based on the virtual token being the same token as the market's index token. With virtual swap impacts, they're not the same token - the market token is a market collateral token, and the virtual token is some other token, which likely has a different price. For example, the README mentions ETH/USDC as ETH/USDT, where USDC is paired with USDT. USDC and USDT sound like they should be equivalent, but looking at the monthly chart of the exchange rate between the two, it has been between 0.8601 and 1.2000 - a 20% difference at the margins. Further, if one of them were to de-peg, the difference may be even larger and for a much longer period of time.

This applies to swaps, as well as to position changes, since those also track virtual swap inventory, since the balance is changing.

Impact

Orders on some markets will get larger/smaller virtual discounts/penalties than they should, as compared to other markets using the same virtual impact pool. In addition to the basic accounting/fee issues associated with the difference, if the price difference is large enough, someone can swap through the market where the impact is smaller due to the exchange rate in order to push the impact more negative, and then simultaneously swap through the other market, where the same amount of funds would result in a larger positive impact than was incurred negatively in the other market, unfairly draining any impact discounts available to legitimate traders.

Code Snippet

The delta being applied is a token amount:

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

283            MarketUtils.applyDeltaToPoolAmount(
284                params.dataStore,
285                params.eventEmitter,
286                _params.market.marketToken,
287                _params.tokenIn,
288 @>             (cache.amountIn + fees.feeAmountForPool).toInt256()
289            );
290    
291            // the poolAmountOut excludes the positive price impact amount
292            // as that is deducted from the swap impact pool instead
293:           MarketUtils.applyDeltaToPoolAmount(

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

and is stored unaltered as the virtual amount:

// File: gmx-synthetics/contracts/market/MarketUtils.sol : MarketUtils.applyDeltaToVirtualInventoryForSwaps()   #2

1471        function applyDeltaToVirtualInventoryForSwaps(
1472            DataStore dataStore,
1473            EventEmitter eventEmitter,
1474            address market,
1475            address token,
1476            int256 delta
1477        ) internal returns (bool, uint256) {
1478            bytes32 marketId = dataStore.getBytes32(Keys.virtualMarketIdKey(market));
1479            if (marketId == bytes32(0)) {
1480                return (false, 0);
1481            }
1482    
1483            uint256 nextValue = dataStore.applyBoundedDeltaToUint(
1484                Keys.virtualInventoryForSwapsKey(marketId, token),
1485 @>             delta
1486            );
1487    
1488            MarketEventUtils.emitVirtualSwapInventoryUpdated(eventEmitter, market, token, marketId, delta, nextValue);
1489    
1490            return (true, nextValue);
1491:       }

https://github.com/sherlock-audit/2023-04-gmx/blob/main/gmx-synthetics/contracts/market/MarketUtils.sol#L1471-L1491

The same is true for deposits, decreases, increases, and withdrawals.

Tool used

Manual Review

Recommendation

Use oracle prices and convert the collateral token to the specific virtual token

[xvi10]

added a note in gmx-io/gmx-synthetics@3f17dd5

[IllIllI000]

gmx-io/gmx-synthetics@3f17dd5
The commit does not fix the issue, and instead acknowledges it via code comments, explaining the issue and its potential negative effects, and says that a patch may be applied at a later time if the issue ever manifests and needs to be addressed.
done