PriceFeed#getSqrtPrice() incorrectly integrates with the Pyth oracle due to only considering expo == -8 as valid prices

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/PriceFeed.sol#L45-L58

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/PriceFeed.sol#L45-L58

    function getSqrtPrice() external view returns (uint256 sqrtPrice) {
        (, int256 quoteAnswer,,,) = AggregatorV3Interface(_quotePriceFeed).latestRoundData();  //@audit
        IPyth.Price memory basePrice = IPyth(_pyth).getPriceNoOlderThan(_priceId, VALID_TIME_PERIOD);

        require(basePrice.expo == -8, "INVALID_EXP");

        require(quoteAnswer > 0 && basePrice.price > 0);

        uint256 price = uint256(int256(basePrice.price)) * Constants.Q96 / uint256(quoteAnswer);
        price = price * Constants.Q96 / _decimalsDiff;

        sqrtPrice = FixedPointMathLib.sqrt(price);
    }

This function returns the square root of the baseToken price quoted in quoteToken, and this data is queried when checking if the vault is in danger in order to liquidate it, with needing a confirmation via PositionCalculator.isLiquidatable().

Problem however is that when getting the basePrice, there is an enforcal that the expo must be -8 due to the require(basePrice.expo == -8, "INVALID_EXP");check, this check however is incorrect and would lead to the protocol to not function properly even when valid prices are available on Pyth, since the check would cause the attempt on getSqrtPrice() to revert.

The above happens because Pyth stores its price in a structure with price itself (int64) and price exponent (int32). Exponent can be any value and must not be -8. For example, price 123.4567 will be stored as:

price = 1234567
expo = -4
The converted price should be calculated as 1234567 * 10^-4 = 123.4567

However, using the current implementation of Predy querying the prices it would assume this valid price to be invalid because the expo is not -8.

This would lead to a serious problem if the quote price has a different expo value or if, for example, the pyth price feed is switched for the same market from 8 decimals precision to 6:

Since most Pyth price feeds use negative expo, this is quite a feasible scenario: i.e if some token price falls significantly, it might need better precision and thus the oracle price feed will be switched to a price with a higher precision (expo), leading to all attempts of querying the price from Pyth to revert.

Keep in mind that the PriceFeed#getSqrtPrice() function is heavily used within the protocol in multiple core logics from getting the vault's TVL in the Liquidation logic to see if the vault is safe or not.

Impact

Valid prices from Pyth would be assumed as invalid, and as such the core logic of getting the square root of the baseToken price quoted in quoteToken via PriceFeed#getSqrtPrice() would be broken, which translates to even DOSing valid liquidations from the Liquidation logic and any functionality that requires to get the prices

Recommended Mitigation Steps

Consider not hardcoding the -8 value as the expo that must be attached to the price returned by Pyth.

Assessed type

Oracle

[alex-ppg]

The Warden states that the system incorrectly mandates that the Pyth oracles configured have an exponent of -8 whilst the Pyth Network system supports a different range for exponents.

While it is true that the Pyth Network supports exponents in the [-12,12] range inclusive of positive values, the tokens that are explicitly supported by the Predy system per the contest's README all have an exponent of -8 in the Pyth Network. As such, this submission cannot be considered a valid vulnerability given that it would not cause any compatibility issues with the tokens the Predy system wishes to support at this stage.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid

[Bauchibred]

Hi @alex-ppg, thanks for judging, kindly reassess the validity of this finding in respect to the excerpts quoted below from the report:

However, using the current implementation of Predy querying the prices it would assume this valid price to be invalid because the expo is not -8.

This would lead to a serious problem if the quote price has a different expo value or if, for example, the pyth price feed is switched for the same market from 8 decimals precision to 6:

Since most Pyth price feeds use negative expo, this is quite a feasible scenario: i.e if some token price falls significantly, it might need better precision and thus the oracle price feed will be switched to a price with a higher precision (expo), leading to all attempts of querying the price from Pyth to revert.

The excerpts above showcase how this bug case is still applicable to Predy, now, since the PriceFeed#getSqrtPrice() function is heavily used within the protocol in multiple core logics from getting the vault's TVL in the Liquidation logic to see if the vault is safe or not this would essentially mean a complete DOS to these functionalities, allowing integrators to even accumulate way more bad debt, considering liquidations would revert, etc.

This window fits directly into 2-med risk per Code4rena's docs on severity categorization, since core functionalities of the protocol and their availability would be impacted.

[alex-ppg]

Hey @Bauchibred, thank you for your PJQA contribution! The oracles in use by the project have historically utilized the same level of accuracy, and a scenario whereby more than 8 decimals are required is significantly low. As such, the original ruling of the submission will remain. It is still advisable for the Sponsor to support a wider range of decimals, and they have confirmed the submission (i.e. will rectify this issue) to ensure new tokens are adequately supported.