Lack of validation of `latestRoundData` could lead to stale prices

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

The PriceFeed contract could be returning outdated prices, leading to inaccurate calculations wherever the contract’s functionality is used. This could result in a loss of money for both the protocol and the users.

Proof of Concept

In order for the protocol to access the square root price of the base token in terms of the quote token, it uses PriceFeed’s function getSqrtPrice. Within it, a call to latestRoundData from AggregatorV3Interface is made in order to get the price for the specified price feed. The only validation done for the returned data is for the quoteAnswer (in other words, the answer parameter), where the check confirms if quoteAnswer is greater than 0.

getSqrtPrice gets called in the function getSqrtIndexPrice if there is a priceFeed address set for the pair. With no check implemented, the returned stale price is then used in PredyPool, GammaTradeMarket, PositionCalculator, and PerpMarketV1. In order of the most impact:

1. GammaTradeMarket - the function is called in 3 separate functions: autoHedge, autoClose, and checkAutoHedgeAndClose, where a fresh price would be needed at all times for a position to be automatically hedged at the right time or closed in order for the protocol to lose the least amount of funds.
2. PositionCalculator - the stale price would make the calculation of the minimum margin inaccurate in function calculateMinMargin.
3. PerpMarketV1 - here it is used in the internal function _calculateInitialMargin, which is called in the contract's callback function following a trade. A stale price would result in an invalid cost for the position to take place.
4. PredyPool - only calls the function in order to read the square root of the index price of the specified pairId; as a result, there isn't much of a risk, but the wrong price is still returned.

As it can be seen in the official documentation of Chainlink Data Feeds:

Your application should track the latestTimestamp variable or use the updatedAt value from the latestRoundData() function to make sure that the latest answer is recent enough for your application to use it.

If the updatedAt parameter isn’t properly validated, the protocol could continue to operate with stale prices as it wouldn’t be aware that there’s an issue.

Tools Used

Manual Review

Recommended Mitigation Steps

For price freshness, the function getSqrtPrice should be updated as follows:

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

+       require(answeredInRound >= roundID, "Stale price");

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

Assessed type

Oracle

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Insufficient proof

[cholakovvv]

hey @alex-ppg! With all due respect, I believe this report provides sufficient evidence of the vulnerability's validity. Here's why:

1. The Vulnerability Details section thoroughly explains the bug itself.
2. The Proof of Concept section provides a detailed explanation of where the bug resides and its potential impact across various use cases of the function. It also includes a reference from the official Chainlink documentation.

Furthermore, the report outlines a proper Recommended Mitigation Steps with a diff demonstrating how to prevent the bug.

Please check this again.

[alex-ppg]

Hey @cholakovvv, thanks for your contribution! Chainlink has directly stated that the round ID values do not need to be sanitized and the answeredInRoundId value is a legacy variable that has been deprecated.