Favoring the balancing of pools over virtual impacts defeats the purpose of virtual impacts

Summary

Favoring the balancing of individual pools over global virtual impacts does not accomplish the goal of making large amounts of toxic order flow more costly

Vulnerability Detail

Impacts balancing pools are consumed before virtual impacts are considered

Impact

Imagine that this audit's incarnation of GMX had already been live during the Terra collapse, and Eth/UST had been one of the stable pairs in the virtual pool including Eth/USDC and Eth/USDT, and each one is currently balanced, and the virtual swap balance is flat. Eth/USDC and Eth/USDT each had small positive Eth impact balances, but Eth/UST had a very large Eth impact balance (meaning swapping to Eth would get a discount). Things are running smoothly, but then there are whispers that Terra is going to go bankrupt. People rush out of their Eth/USDC and Eth/USDT stable positions into Eth, but since those markets had very small reserves, the swap impacts very quickly go negative, and the virtual impacts grow an grow. In the meantime, anyone and everyone with UST uses GMX to exit their positions, and actually get discounts because of the large Eth impact balance. In the end, the LPs of the Eth/UST get stuck with UST which goes to zero. Had the virtual impacts been applied before the pool-specific ones, the LPs would have at least gotten large fees, and may have had more time to withdraw their deposits (which does not affect balance), since other exchanges would have been cheaper, given that there would have been an impact assessed on trades on GMX.

Position virtual impacts have an analogous vulnerability since they are also only considered once the market-specific imbalance is flat or negative.

Code Snippet

Virtual impacts do not get applied until there is no more positive pool-specific impact:

// File: gmx-synthetics/contracts/pricing/SwapPricingUtils.sol : SwapPricingUtils.getPriceImpactUsd()   #1

96             // the virtual price impact calculation is skipped if the price impact
97             // is positive since the action is helping to balance the pool
98             //
99             // in case two virtual pools are unbalanced in a different direction
100            // e.g. pool0 has more WNT than USDC while pool1 has less WNT
101            // than USDT
102            // not skipping the virtual price impact calculation would lead to
103            // a negative price impact for any trade on either pools and would
104            // disincentivise the balancing of pools
105:@>         if (priceImpactUsd >= 0) { return priceImpactUsd; }

https://github.com/sherlock-audit/2023-04-gmx/blob/main/gmx-synthetics/contracts/pricing/SwapPricingUtils.sol#L91-L104

Tool used

Manual Review

Recommendation

Apply virtual impacts first, and only give discounts when the virtual balance is flat.

xvi10 commented 1 year ago

in the given scenario mainly the UST pool would be affected which is expected behaviour

anyone and everyone with UST uses GMX to exit their positions

if the calculated USD value of UST starts increasing due to inflows vs the calculated USD value of ETH in the ETH/UST, then negative price impact would apply

IllIllI000 commented 1 year ago

@xvi10 the thrust of this submission is that the UST pool's impacts remain isolated until its pool is drained, at which point there is a sudden step function due to it being in sync with the other virtual impacts. Why is this behavior favored over only giving impacts when this really are skewed? What is the advantage of the current way?

xvi10 commented 1 year ago

the current logic is preferred to avoid entering a state where there is no incentive to balance pools

    // in case two virtual pools are unbalanced in a different direction
    // e.g. pool0 has more WNT than USDC while pool1 has less WNT
    // than USDT
    // not skipping the virtual price impact calculation would lead to
    // a negative price impact for any trade on either pools and would
    // disincentivise the balancing of pools