IllIllI - Negative prices will cause old orders to be canceled

[sherlock-admin]

IllIllI

medium

Negative prices will cause old orders to be canceled

Summary

In most cases where orders are submitted using invalid oracle prices, the check for isEmptyPriceError() returns true, and the order execution is allowed to revert, rather than canceling the order.

Vulnerability Detail

Negative Chainlink oracle prices (think negative interest rates in Europe) result in a plain revert(<string>), which isn't counted as one of these errors, and so if the price becomes negative, any outstanding order will be canceled, even if the order was submitted prior to the price going negative.

Impact

Orders to close positions will be canceled, leading to losses.

Code Snippet

Chainlink prices are converted to positive numbers:

// File: gmx-synthetics/contracts/oracle/Oracle.sol : Oracle._setPricesFromPriceFeeds()   #1

577                ) = priceFeed.latestRoundData();
578    
579:@>             uint256 price = SafeCast.toUint256(_price);

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/oracle/Oracle.sol#L577-L579

And if they're negative, the code reverts:

// File: gmx-synthetics/node_modules/@openzeppelin/contracts/utils/math/SafeCast.sol : SafeCast.toUint256()   #2

558        function toUint256(int256 value) internal pure returns (uint256) {
559            require(value >= 0, "SafeCast: value must be positive");
560            return uint256(value);
561:       }

Orders that revert get frozen or canceled

Tool used

Manual Review

Recommendation

Create a new error type, and include it in the list of OracleUtils.isEmptyPriceError() errors

[hack3r-0m]

Escalate for 10 USDC

This finding should be invalid, in "Impact" section, author mentions:

Orders to close positions will be canceled, leading to losses.

If price is negative, cancelling of order should be desired outcome.

author mentions:

Negative Chainlink oracle prices (think negative interest rates in Europe)

this is not 1:1 comparison, here sign of interest rate is compared with sign of asset and both are completly different things

[sherlock-admin]

Escalate for 10 USDC

This finding should be invalid, in "Impact" section, author mentions:

Orders to close positions will be canceled, leading to losses.

If price is negative, cancelling of order should be desired outcome.

author mentions:

Negative Chainlink oracle prices (think negative interest rates in Europe)

this is not 1:1 comparison, here sign of interest rate is compared with sign of asset and both are completly different things

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.

[sherlock-admin]

Escalate for 10 USDC

This finding should be invalid, because keepers are trusted entity as mentioned in REAMDE and signature of keeper on longest chain cannot be used on forks because of check that only keeper can update price.

You've deleted an escalation for this issue.

[IllIllI000]

Asset prices can in fact go negative due to carying costs, and the sponsor confirmed the issue

[hrishibhat]

Escalation rejected

Although unlikely the possibility of this must be addressed instead of canceling it.

[sherlock-admin]

Escalation rejected

Although unlikely the possibility of this must be addressed instead of canceling it.

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.

[xvi10]

Fix in https://github.com/gmx-io/gmx-synthetics/pull/113