Open sherlock-admin opened 1 year ago
fixed in https://github.com/gmx-io/gmx-synthetics/pull/155/commits/ac2b1dc0fce1fc73d65859c95eb0ce72d20ff30d
the code was updated to check for whether a position is liquidatable after state variables were updated
chaduke
medium
PositionUtils.validatePosition() uses
isIncrease
instead offalse
when calling isPositionLiquidatable(), making it not work properly for the case ofisIncrease = true
.Summary
PositionUtils.validatePosition()
usesisIncrease
instead offalse
when 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 useisIncrease
for the input parameter forisPositionLiquidatable()
. We should always usefalse
instead.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 theisIncrease
to functionisPositionLiquidatable()
as an argument. Actually, thefalse
value 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 usefalse
here, 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 thenextDiffUsd
is 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
isIncrease
instead offalse
when 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():