IllIllI - Liquidation shouldn't be used to close positions that were fully-collateralized prior to collateral requirement changes

[sherlock-admin]

IllIllI

medium

Liquidation shouldn't be used to close positions that were fully-collateralized prior to collateral requirement changes

Summary

There are various factors associated with minimum collateral requirements, and if a position falls below them, the position is liquidated.

Vulnerability Detail

If the position was over-collateralized and in profit prior to the change in the minimums, and the minimum is increased, the position is liquidated.

Impact

Liquidation gives all funds to the pool, giving nothing back to the user

Code Snippet

A position becomes liquidatable once it falls below the changeable collateral requirements:

// File: gmx-synthetics/contracts/position/PositionUtils.sol : PositionUtils.isPositionLiquidatable()   #1

377            if (shouldValidateMinCollateralUsd) {
378 @>             cache.minCollateralUsd = dataStore.getUint(Keys.MIN_COLLATERAL_USD).toInt256();
379                if (cache.remainingCollateralUsd < cache.minCollateralUsd) {
380                    return true;
381                }
382            }
383    
384            if (cache.remainingCollateralUsd <= 0) {
385                return true;
386            }
387    
388:           // validate if (remaining collateral) / position.size is less than the min collateral factor (max leverage exceeded)

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/position/PositionUtils.sol#L368-L388

Liquidations give everything to the pool, and nothing to the position's account

Tool used

Manual Review

Recommendation

Close the position with a market order, rather than liquidating it, if the user was previously above the minimum with the old factor

[xvi10]

the min collateral requirements should not be changed unless it is an emergency requirement

this setting should eventually be removed from the Config contract

[IllIllI000]

@xvi10, while it may be for emergency use, if it does end up being used, isn’t the current behavior incorrect? Should this be a no-fix?

[xvi10]

yes, updated to confirmed