`PriceFeed` calculation may overflow due to performing a direct multiplication with `Q96`, causing DoS

[c4-bot-6]

Lines of code

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

Vulnerability details

Impact

PriceFeed::getSqrtPrice provides the square root price of the base token in terms of the quote token.

There is a problem where direct multiplication is performed with Constants.Q96 (2^96) twice. This can cause overflow in some cases resulting in DoS.

Proof of Concept

PriceFeed.sol#L45-L58

   function getSqrtPrice() external view returns (uint256 sqrtPrice) {
        (, int256 quoteAnswer,,,) = AggregatorV3Interface(_quotePriceFeed).latestRoundData();

        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; //@audit this can overflow

        sqrtPrice = FixedPointMathLib.sqrt(price);
    }

Here price is multiplied by Constants.Q96 directly, which is defined as the following:

Constants.sol#L19

uint256 internal constant Q96 = 0x1000000000000000000000000; //@audit 2^96

This is a case that may cause DoS due to overflow, and it is also handled by Uniswap's Oracle Library

Here's an example of how easily overflow can occur due to direct multiplication with Q96 using chisel:

uint256 q96 = 0x1000000000000000000000000; //2^96
uint256 price = 20000 * 10 ** 18; // 20000 token with 18 decimals
uint256 price2 = price * q96; // initial multiplication
q96
Type: uint
├ Hex: 0x0000000000000000000000000000000000000001000000000000000000000000
└ Decimal: 79228162514264337593543950336
price
Type: uint
├ Hex: 0x00000000000000000000000000000000000000000000043c33c1937564800000
└ Decimal: 20000000000000000000000
price2
Type: uint
├ Hex: 0x00000000000000000000043c33c1937564800000000000000000000000000000
└ Decimal: 1584563250285286751870879006720000000000000000000000
uint256 price3 = price2 * q96; // next multiplication, causing overflow
Traces:
  [740] 0xBd770416a3345F91E4B34576cb804a576fa48EB1::run()
    └─ ← panic: arithmetic underflow or overflow (0x11)

⚒️ Chisel Error: Failed to execute REPL contract!

Tools Used

Manual Review.

Recommended Mitigation Steps

Utilize Uniswap's FullMath library to perform this calculation

   function getSqrtPrice() external view returns (uint256 sqrtPrice) {
        (, int256 quoteAnswer,,,) = AggregatorV3Interface(_quotePriceFeed).latestRoundData();

        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;
+       price = FullMath.mulDiv(price * Constants.Q96 / _decimalsDiff);

        sqrtPrice = FixedPointMathLib.sqrt(price);
    }

Assessed type

Under/Overflow

[crypticdefense]

This report describes how the protocol's PriceFeed contract performs dangerous multiplication with Q96 = 2^96 twice.

First instance: uint256 price = uint256(int256(basePrice.price)) * Constants.Q96 / uint256(quoteAnswer);. Next instance: price = price * Constants.Q96 / _decimalsDiff;

First, the price is calculated with multiplication by Constants.Q96, which will result in price to have units in 2^96. There is another direct multiplcation with Constants.Q96 in the second line, which can overflow, as displayed in the report using chisel.

There are also similar findings on solodit: #1, #2, #3

Therefore, I believe this should be a valid medium severity finding.