c4-bot-4 commented 4 months ago

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/vendors/IPyth.sol#L5-L14 https://github.com/code-423n4/2024-05-predy/blob/main/src/libraries/PositionCalculator.sol#L142-L144

Vulnerability details

Impact

The protocol utilizes getPriceNoOlderThan from the Pyth oracle to determine the price of the base token, which is subsequently used to calculate the sqrtPrice.

The Pyth oracle returns a Price struct that contains the price and exponent.

The protocol does not take into account the exponent returned and proceeds to utilize the price without the exponent for the sqrt price calculation, therefore the sqrt price will be incorrect.

This will impact any calls to PriceFeed::getSqrtPrice(). An example is to check if positions are liquidatable if they are unsafe, if the sqrt price is incorrect, then safe positions may be liquidated or unsafe positions may be unable to be liquidated.

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;

        sqrtPrice = FixedPointMathLib.sqrt(price);
    }

The IPyth.Price struct contains the following parameters:

IPyth.sol#L5-L14

    struct Price {
        // Price
        int64 price;
        // Confidence interval around the price
        uint64 conf;
        // Price exponent
        int32 expo;
        // Unix timestamp describing when the price was published
        uint256 publishTime;
    }

As mentioned in the Pyth Network Docs:

"The returned price and confidence are decimal numbers written in the form a * 10^e, where e is an exponent included in the result. For example, a price of 1234 with an exponent of -2 represents the number 12.34. The result also includes a publishTime which is the unix timestamp for the price update."

This means that the exponent must be applied to the basePrice.price to get the actual price. Most Pyth Oracles return a negative exponent, but it's possible that it may be positive, so both cases need to be handled.

Currently, the protocol uses the returned basePrice.price for the sqrtPrice calculation, which will return an incorrect sqrtPrice.

An example of where this price feed is used is in PositionCalculator::getSqrtIndexPrice

PositionCalculator.sol#L142-L144

        if (pairStatus.priceFeed != address(0)) {
            return PriceFeed(pairStatus.priceFeed).getSqrtPrice();
        }

The flow of the call in this example is LiquidationLogic::liquidate => LiquidationLogic::checkVaultIsDanger => PositionCalculator::isLiquidatable => PositionCalculator::calculateMinMargin => PositionCalculator::getSqrtIndexPrice

In that case, this would not correctly be able to determine if a position is unsafe and should be forced liquidated.

Tools Used

Manual Review

Recommended Mitigation Steps

Apply the returned expo of the Price struct to the price. The expo may be positive or negative, so both cases must be handled. If exponent is negative, ensure to negate it when dividing.

   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);

+       int256 basePriceFinal = basePrice.expo >= 0 ? (basePrice.price * 10 ** basePrice.expo) : (basePrice.price / 10**(-basePrice.expo));

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

        sqrtPrice = FixedPointMathLib.sqrt(price);
    }

Assessed type

Oracle

crypticdefense commented 4 months ago

This report describes how the PriceFeed contract does not account for the exponent of the base price retrieved from the Pyth oracle.

As mentioned in the Pyth Network Docs:

"The returned price and confidence are decimal numbers written in the form a * 10^e, where e is an exponent included in the result. For example, a price of 1234 with an exponent of -2 represents the number 12.34. The result also includes a publishTime which is the unix timestamp for the price update."

The base price simply uses the price but does not account for the exponent. The recommended mitigation suggests to apply the exponent and account for both positive and negative exponents, since the oracle can return either one. As mentioned in the report, this will lead to an incorrect sqrtPrice calculation, affecting many different parts of the protocol, such as the ability for bad positions to be liquidated.

Here's a similar finding on solodit, where the protocol correctly applied the exponents to the price returned from the Pyth oracle, but were not account for negative exponents: #1