IllIllI - Positions cannot be liquidated once the oracle prices are zero
Opened this issue · 11 comments
IllIllI
medium
Positions cannot be liquidated once the oracle prices are zero
Summary
In the unlikely event that the price of an asset reaches zero, there is no way to liquidate the position, because both usages of oracles will revert.
Vulnerability Detail
Zero is treated as an invalid value, for both index oracle prices, as well as position prices.
Impact
Positions won't be liquidatable, at an extremely critical moment that they should be liquidatable. Losses and fees will grow and the exchange will become insolvent.
Code Snippet
The Chainlink oracle rejects prices of zero:
// File: gmx-synthetics/contracts/oracle/Oracle.sol : Oracle._setPricesFromPriceFeeds() #1
571 (
572 /* uint80 roundID */,
573 int256 _price,
574 /* uint256 startedAt */,
575 /* uint256 timestamp */,
576 /* uint80 answeredInRound */
577 ) = priceFeed.latestRoundData();
578
579 uint256 price = SafeCast.toUint256(_price);
580 uint256 precision = getPriceFeedMultiplier(dataStore, token);
581
582 price = price * precision / Precision.FLOAT_PRECISION;
583
584 if (price == 0) {
585 @> revert EmptyFeedPrice(token);
586 }
587: As does the usage of off-chain oracle prices:
// File: gmx-synthetics/contracts/oracle/Oracle.sol : Oracle.getLatestPrice() #2
346 if (!secondaryPrice.isEmpty()) {
347 return secondaryPrice;
348 }
349
350 Price.Props memory primaryPrice = primaryPrices[token];
351 if (!primaryPrice.isEmpty()) {
352 return primaryPrice;
353 }
354
355 @> revert OracleUtils.EmptyLatestPrice(token);
356: }Tool used
Manual Review
Recommendation
Provide a mechanism for positions to be liquidated even if the price reaches zero
oracles should report the minimum possible price for this case
@xvi10 if the price drops from $100 down to zero, isn’t that going to cause problems? If you don’t want to work with prices of zero, shouldn’t there be, e.g. 1 wei of value so that wildly incorrect prices aren’t used?
i think if we allow zero an issue could be raised that allowing zero increases the risk that oracle malfunctions leading to reports of zero prices could cause losses to the pool
in that case, perhaps the recommendation in the previous comment is not correct, the oracle should report zero as the price but the market would need some manual intervention to check and settle
the readme states that Oracle signers are expected to accurately report the price of tokens, so any misreporting would be a separate bug, and reporting a price of 100 seems much more dangerous than an unidentified bug. As is described in #155, using a sentinel value would be safer
agree on that, i'm not sure if it is worth the complexity to handle a zero price case vs manually settling the market, will mark this as confirmed and may not fix
some calculations such as position.sizeInTokens would not make sense if the price is zero or negative so I believe the contracts in the current state cannot support these values and the markets would need to be manually settled
Escalate for 10 USDC
This issue should be invalid, because of how unlikely this is to be happen. A same argument (with similiar unlikely odds) can be made that positions cannot be liquidated if ethereum goes down as a network.
Escalate for 10 USDC
This issue should be invalid, because of how unlikely this is to be happen. A same argument (with similiar unlikely odds) can be made that positions cannot be liquidated if ethereum goes down as a network.
You've created a valid escalation for 10 USDC!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
There is a viable scenario (even if unlikely) https://docs.sherlock.xyz/audits/judging/judging . This is a valid risk and is easily protected against. CEX securities go to zero all the time, and as crypto grows and ages, the same will become true for DAO tokens
Escalation rejected
Although unlikely, a zero price should not break the liquidations.
Considering this a valid medium
Escalation rejected
Although unlikely, a zero price should not break the liquidations.
Considering this a valid medium
This issue's escalations have been rejected!
Watsons who escalated this issue will have their escalation amount deducted from their next payout.
this is a valid issue, but as mentioned the market needs to be manually settled as some calculations may not make sense for zero or negative values