This repository has been archived by the owner on Dec 10, 2023. It is now read-only.
chaduke - PositionUtils.validatePosition() uses isIncrease instead of false when calling isPositionLiquidatable(), making it not work properly for the case of isIncrease = true.
#180
Labels
Medium
A valid Medium severity issue
Reward
A payout will be made for this issue
Sponsor Confirmed
The sponsor acknowledged this issue is valid
Will Fix
The sponsor confirmed this issue will be fixed
Comments
xvi10
added
Sponsor Confirmed
|
fixed in gmx-io/gmx-synthetics@ac2b1dc the code was updated to check for whether a position is liquidatable after state variables were updated |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
chaduke
medium
PositionUtils.validatePosition() uses
isIncreaseinstead offalsewhen calling isPositionLiquidatable(), making it not work properly for the case ofisIncrease = true.Summary
PositionUtils.validatePosition()usesisIncreaseinstead offalsewhen calling isPositionLiquidatable(), making it not work properly for the case ofisIncrease = true. The main problem is that when callingisPositionLiquidatable(), we should always consider decreasing the position since we are proposing a liquidation trade (which is a decrease in position). Therefore, it should not useisIncreasefor the input parameter forisPositionLiquidatable(). We should always usefalseinstead.Vulnerability Detail
PositionUtils.validatePosition()is called to validate whether a position is valid in both collateral size and position size, and in addition, to check if the position is liquidable:https://github.com/sherlock-audit/2023-04-gmx/blob/main/gmx-synthetics/contracts/position/PositionUtils.sol#L261-L296
It calls function
isPositionLiquidatable()to check if a position is liquidable. However, it passes theisIncreaseto functionisPositionLiquidatable()as an argument. Actually, thefalsevalue should always be used for calling functionisPositionLiquidatable()since a liquidation is always a decrease position operation. A position is liquidable or not has nothing to do with exiting trade operations and only depend on the parameters of the position per se.Current implementation has a problem for an increase order: Given a Increase order, for example, increase a position by $200, when
PositionUtils.validatePosition()is called, which is after the position has been increased, we should not consider another $200 increase inisPositionLiquidatable()again as part of the price impact calculation. This is double-accouting for price impact calculation, one during the position increasing process, and another in the position validation process. On the other hand, if we usefalsehere, then we are considering a decrease order (since a liquidation is a decrease order) and evaluate the hypothetical price impact if the position will be liquidated.https://github.com/sherlock-audit/2023-04-gmx/blob/main/gmx-synthetics/contracts/position/PositionUtils.sol#L304-L412
Our POC code confirms my finding: intially, we don't have any positions, after executing a LimitIncrease order, the priceImpactUsd is evaluaed as follows (notice initialDiffUsd = 0):
Then, during validation, when
PositionUtils.validatePosition()is called, the double accouting occurs, notice thenextDiffUsdis doubled, as if the limitOrder was executed for another time!The POC code is as follows, pay attention to the
testLimit()and the execution ofcreateLimitIncreaseOrder(). Please comment out the checks for signature, timestamp and block number for oracle price in the source code to run the testing smoothly without revert.Impact
PositionUtils.validatePosition() uses
isIncreaseinstead offalsewhen calling isPositionLiquidatable(), making it not work properly for the case ofisIncrease = true. A liquidation should always be considered as a decrease order in terms of evaluating price impact.Code Snippet
Tool used
VSCode
Manual Review
Recommendation
Pass false always to isPositionLiquidatable():
function validatePosition( DataStore dataStore, IReferralStorage referralStorage, Position.Props memory position, Market.Props memory market, MarketUtils.MarketPrices memory prices, bool isIncrease, bool shouldValidateMinPositionSize, bool shouldValidateMinCollateralUsd ) public view { if (position.sizeInUsd() == 0 || position.sizeInTokens() == 0) { revert Errors.InvalidPositionSizeValues(position.sizeInUsd(), position.sizeInTokens()); } MarketUtils.validateEnabledMarket(dataStore, market.marketToken); MarketUtils.validateMarketCollateralToken(market, position.collateralToken()); if (shouldValidateMinPositionSize) { uint256 minPositionSizeUsd = dataStore.getUint(Keys.MIN_POSITION_SIZE_USD); if (position.sizeInUsd() < minPositionSizeUsd) { revert Errors.MinPositionSize(position.sizeInUsd(), minPositionSizeUsd); } } if (isPositionLiquidatable( dataStore, referralStorage, position, market, prices, - isIncrease, + false, shouldValidateMinCollateralUsd )) { revert Errors.LiquidatablePosition(); } }The text was updated successfully, but these errors were encountered: