0xumarkhatab - Protocol Implementation accepts 0 price from Pyth network which can cause Disruption in core functionalities

[sherlock-admin3]

0xumarkhatab

medium

Protocol Implementation accepts 0 price from Pyth network which can cause Disruption in core functionalities

Summary

Currently, core functions of protocol like quoteOptoin , liquidate , transferMarginToFund,Order execution depends on the fetched price of the asset from Pyth network. However, in the current implementation, the protocol does not prevent zero price and these functionalities can be badly affected.

Protocol team member bchen3 has confirmed that even though they trust the Pyth network , this scenario is considerable to mitigate.

Vulnerability Detail

The getPrice function gets the price from Pyth network which is a source of reliable data feeds.

function getPrice(bytes32 priceFeedId) public view returns (uint256, uint256) {
        // We don't use pyth.getPrice(), so we can control when to revert with _maxPriceAge,
        // reverted with StalePrice if price.publishTime exceeds _maxPriceAge
        try _pyth.getPriceNoOlderThan(priceFeedId, _maxPriceAge) returns (PythStructs.Price memory price) {
            // Assumes base price is against quote
            return (_convertToUint256(price), price.publishTime);
        } catch (bytes memory reason) {
            revert LibError.OracleDataRequired(priceFeedId, reason);
        }
    }

    function _convertToUint256(PythStructs.Price memory pythPrice) internal pure returns (uint256) {
        // Remember to update the conversion formula below accordingly if you have changed the conditions.
        // Note both calculations below rely on the conditions here to prevent from overflow.
        // Be sure to double check if you change the conditions.
        if (pythPrice.price < 0 || pythPrice.expo > 0 || pythPrice.expo < -int8(INTERNAL_DECIMALS))
            revert IllegalPrice(pythPrice);

        // .price = 181803
        // .expo = -2
        // decimal price = 181803 * 10^(-2) =  1818.03
        // converted price = 181803 * 10^(18 - 2) = 1.81803e21

        uint256 baseConversion = 10 ** uint256(int256(int8(INTERNAL_DECIMALS)) + pythPrice.expo);

        return uint256(int256(pythPrice.price)) * baseConversion;
    }

This function handles things elegantly however , current implementation does not prevent 0 price. According to Pythnet - How to use Price Feeds :

Sometimes, Pyth will not be able to provide a current price for a product. 
This situation can happen for various reasons. 

For example, US equity markets only trade during certain hours, and outside those hours,
 it's not clear what an equity's price is. Alternatively, a network outage (at the internet level, blockchain level, 
or at multiple data providers) may prevent the protocol from producing new price updates.
(Such outages are unlikely, but integrators should still be prepared for the possibility.) 

In such cases, Pyth may return a stale price for the product.

Integrators should be careful to avoid accidentally using a stale price

Pyth network acknowledges that stale prices can be delivered for an asset in some scenarios. The likelihood is low but the impact on liquidation , quoteOption ,transferMarginToFund, Order execution can be drastic . It can either make loss of funds for users or just bricking the functionality of the system based on different scenarios

Impact

• Non-determined behaviour of functions like liquidation , quoteOption ,transferMarginToFund, Order execution
• Protocol or Users funds can be lost (i.e if getPrice is used to decide if some person is liquidatable , non-liquidatable users might be liquidated because their collateral will appear to be far less than it actually is )

  Code Snippet

  https://github.com/sherlock-audit/2024-02-perpetual/blob/main/perp-contract-v3/src/oracle/pythOracleAdapter/PythOracleAdapter.sol#L80-L89

  Tool used

Manual Review

Recommendation

Add non-zero price check in getPrice

 function getPrice(bytes32 priceFeedId) public view returns (uint256, uint256) {
        // We don't use pyth.getPrice(), so we can control when to revert with _maxPriceAge,
        // reverted with StalePrice if price.publishTime exceeds _maxPriceAge
        try _pyth.getPriceNoOlderThan(priceFeedId, _maxPriceAge) returns (PythStructs.Price memory price) {
            // Assumes base price is against quote
            require(price.price!=0,"Pyth provided Stale zero price ")
            return (_convertToUint256(price), price.publishTime);
        } catch (bytes memory reason) {
            revert LibError.OracleDataRequired(priceFeedId, reason);
        }
    }

You can also check this in _convertToUint256 as

 if ( pythPrice.price <= 0 || pythPrice.expo > 0 || pythPrice.expo < -int8(INTERNAL_DECIMALS))
            revert IllegalPrice(pythPrice);

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

its trusted to provide accurate data.

[nevillehuang]

Invalid, this falls under the following known issue:

2. Oracle (Pyth) is expected to accurately report the price of market

Even if a 0 price is returned, it is assume that it is the accurate market price for the asset at that point of time. It also means a depeg has occured, but this issue fails to provide an adequate explanation.

[IllIllI000]

The sponsor acknowledges that zero is a valid price, and the change may cause liquidations to be impossible in such cases, but points out that crypto prices are unlikely to be zero.