IllIllI - Positions can still be liquidated even if orders to prevent it can't execute

[sherlock-admin]

IllIllI

medium

Positions can still be liquidated even if orders to prevent it can't execute

Summary

Positions can still be liquidated even if orders to close positions or add collateral can't execute, because liquidation does not transfer tokens

Vulnerability Detail

Liquidation orders do not transfer tokens - they just use the increment*()/applyDelta*() functions to update the portions allotted to the various parties. Orders to close positions, on the other hand, actually transfer the tokens so if the transfer reverts, the position can't be closed. If the collateral token is paused (e.g. USDC), a user won't be able to close their position, or add collateral to it, in order to prevent it from being liquidated, but the liquidation keeper will be able to liquidate without any issue.

Impact

Users will be liquidated without being able to prevent it

Code Snippet

Liquidation doesn't actually transfer any funds - it just updates who got what:

// File: gmx-synthetics/contracts/position/DecreasePositionCollateralUtils.sol : DecreasePositionCollateralUtils.getLiquidationValues()   #1

346            } else {
347 @>             values.pnlAmountForPool = (params.position.collateralAmount() - fees.funding.fundingFeeAmount).toInt256();
348            }
349    
350            PositionPricingUtils.PositionFees memory _fees;
351    
352            PositionUtils.DecreasePositionCollateralValues memory _values = PositionUtils.DecreasePositionCollateralValues(
353                values.pnlTokenForPool,
354 @>             values.executionPrice, // executionPrice
355                0, // remainingCollateralAmount
356                values.positionPnlUsd, // positionPnlUsd
357                values.pnlAmountForPool, // pnlAmountForPool
358                0, // pnlAmountForUser
359                values.sizeDeltaInTokens, // sizeDeltaInTokens
360                values.priceImpactAmount, // priceImpactAmount
361                0, // priceImpactDiffUsd
362                0, // priceImpactDiffAmount
363                PositionUtils.DecreasePositionCollateralValuesOutput(
364:                   address(0),

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/position/DecreasePositionCollateralUtils.sol#L344-L364

And then increments/applies delta to the accounting.

Tool used

Manual Review

Recommendation

Keep user collateral at a separate address from the pool address, so that liquidations have to do an actual transfer which may revert, rather than just updating internal accounting

[xvi10]

This should be a very rare case, we believe that it may not make sense to force doing a transfer to handle this case, liquidations should instead be manually disabled if this were to happen, it may also be possible to do this automatically with a contract function that anyone can invoke and if transfers are verified to not be working then the contract has access to disable liquidations for that market, for the current scope this feature will not be added