IllIllI - Trades in blocks where the bid or ask drops to zero will be priced using the previous block's price

[sherlock-admin]

IllIllI

medium

Trades in blocks where the bid or ask drops to zero will be priced using the previous block's price

Summary

The oracle prices used for traces allow multiple oracles and their last prices to be provided. The oldest block's price becomes the primary price, and the newer price becomes the secondary price. Trades in blocks where the primary price is non-zero, but the secondary price is zero, will be priced incorrectly

Vulnerability Detail

For position increase/decrease orders, the price used is either the primary or the secondary price, but a value of zero for the secondary price is considered to be a sentinel value indicating 'empty', or 'no price has been set'. In such cases, the secondary price is ignored, and the primary price is used instead.

Impact

Users exiting their positions in the first block where the price touches zero, are able to exit their positions at the primary (older) price rather than the secondary (newer) price of zero. This is pricing difference is at the expense of the pool and the other side of the trade.

Code Snippet

The secondary price is only used when it's non-zero:

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

341        function getLatestPrice(address token) external view returns (Price.Props memory) {
342            if (token == address(0)) { return Price.Props(0, 0); }
343    
344            Price.Props memory secondaryPrice = secondaryPrices[token];
345    
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:       }

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

Note that even if just the bid touches zero, that's enough to disqualify the secondary price.

Tool used

Manual Review

Recommendation

Use an actual sentinel flag rather than overloading the meaning of a 'zero' price.

[xvi10]

oracles should report the minimum possible price instead of zero

[IllIllI000]

@xvi10 same question as for #156

[xvi10]

replied in 156

[IllIllI000]

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 above, using a sentinel value would be safer

[xvi10]

replied in #156

[hack3r-0m]

Escalate for 10 USDC

should be dup of #156 instead of seperate issue

[sherlock-admin]

Escalate for 10 USDC

should be dup of #156 instead of seperate issue

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.

[IllIllI000]

looks like I missed this one. The fix for this one does not resolve the other. One is about using the wrong price, and the other is about an unhandled revert case

[hrishibhat]

Escalation rejected

Although related to the similar topic but Not a duplicate of #156

[sherlock-admin]

Escalation rejected

Although related to the similar topic but Not a duplicate of #156

This issue's escalations have been rejected!

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

[xvi10]

no code changed, similar reason to #156