IllIllI - Missing checks for whether a position is still an ADL candidate

[sherlock-admin]

IllIllI

medium

Missing checks for whether a position is still an ADL candidate

Summary

There are no checks for whether a position is still an ADL candidate or not

Vulnerability Detail

All checks during AdlHandler.executeAdl() are global checks about exchange-wide PnL levels, and there are no checks for whether the specific position is a valid ADL candidate or not.

Impact

Between the time when the ADL keeper chooses its list of ADL candidates and when the transaction gets confirmed (e.g. if the keeper has a sudden CPU spike, or the connection to the sequencer has to be retried), the user may have already reduced their position, and it would be incorrect to further reduce their position.

Code Snippet

All checks are global:

// File: gmx-synthetics/contracts/exchange/AdlHandler.sol : AdlHandler.executeAdl()   #1

119 @>         (cache.shouldAllowAdl, cache.pnlToPoolFactor, cache.maxPnlFactorForAdl) = MarketUtils.isPnlFactorExceeded(
120                dataStore,
121                oracle,
122                market,
123                isLong,
124                Keys.MAX_PNL_FACTOR_FOR_ADL
125            );
126    
127            if (!cache.shouldAllowAdl) {
128                revert AdlNotRequired(cache.pnlToPoolFactor, cache.maxPnlFactorForAdl);
129:           }

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/exchange/AdlHandler.sol#L119-L129

// File: gmx-synthetics/contracts/exchange/AdlHandler.sol : AdlHandler.executeAdl()   #2

150            // validate that the ratio of pending pnl to pool value was decreased
151 @>         cache.nextPnlToPoolFactor = MarketUtils.getPnlToPoolFactor(dataStore, oracle, market, isLong, true);
152            if (cache.nextPnlToPoolFactor >= cache.pnlToPoolFactor) {
153                revert InvalidAdl(cache.nextPnlToPoolFactor, cache.pnlToPoolFactor);
154            }
155    
156            cache.minPnlFactorForAdl = MarketUtils.getMinPnlFactorAfterAdl(dataStore, market, isLong);
157    
158            if (cache.nextPnlToPoolFactor < cache.minPnlFactorForAdl.toInt256()) {
159                revert PnlOvercorrected(cache.nextPnlToPoolFactor, cache.minPnlFactorForAdl);
160            }
161:       }

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/exchange/AdlHandler.sol#L150-161

The order only gets canceled if the order would cause the position size to go negative

Tool used

Manual Review

Recommendation

Create a view function for whether the position is eligible for ADL, and revert if the user is not eligible when the order executes

[xvi10]

the ADL transaction should revert if it does not reduce the pnl value to pool value ratio

[IllIllI000]

@xvi10 if there are 100 ADL-eligible accounts and you pick the largest one, if that largest one becomes the smallest by the time the keeper executes, shouldn’t one of the larger other 99 be chosen instead?

[xvi10]

preferably it should be the largest ADL candidate but we don't enforce this on-chain because I think it is difficult to maintain an ordering or orders by PnL

this behaviour could be incentivised off-chain through rewarding keepers proportional to the PnL of the position ADLed, could be done on the contract level as well, but we do not think this feature is worth the complexity to add for now

[IllIllI000]

I'm Keeping this open since the argument is complexity rather than optimal behavior