Chainlink's latestRoundData might return stale or incorrect results

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

In the PriceFeed contract, the protocol uses a ChainLink aggregator to fetch the latestRoundData(), but there is no check if the return value indicates stale data. The only check present is for the quoteAnswer to be > 0, however, this alone is not sufficient.

function getSqrtPrice() external view returns (uint256 sqrtPrice) {
@>        (, int256 quoteAnswer,,,) = AggregatorV3Interface(_quotePriceFeed).latestRoundData(); // missing additional checks

        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 protocol mentions that:

Attacks that stem from the TWAP being extremely stale compared to the market price within its period (currently 30 minutes) are a known risk. As a general rule, only price manipulation issues that can be triggered by manipulating the price atomically from a normal pool or oracle state are valid.

However, this stale period check is only currently applied to the Pyth integration, where the ChainLink feed is not considered for stale data.

This could lead to stale prices according to the Chainlink documentation:

https://docs.chain.link/docs/historical-price-data/#historical-rounds
https://docs.chain.link/docs/faq/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round

This discrepancy could have the protocol produce incorrect values for very important functions in different places across the system, such as GammaTradeMarket, PositionCalculator, LiquidationLogic, etc.

Proof of Concept

https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/PriceFeed.sol#L46
https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/libraries/PositionCalculator.sol#L141

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding missing checks for stale data.

@@ -43,7 +43,10 @@ contract PriceFeed {
 
     /// @notice This function returns the square root of the baseToken price quoted in quoteToken.
     function getSqrtPrice() external view returns (uint256 sqrtPrice) {
-        (, int256 quoteAnswer,,,) = AggregatorV3Interface(_quotePriceFeed).latestRoundData();
+        (uint80 quoteRoundID, int256 quoteAnswer,, uint256 quoteTimestamp, uint80 quoteAnsweredInRound) =
+            AggregatorV3Interface(_quotePriceFeed).latestRoundData();
+        require(quoteAnsweredInRound >= quoteRoundID, "Stale price!");
+        require(quoteTimestamp != 0, "Round not complete!");
+        require(block.timestamp - quoteTimestamp <= VALID_TIME_PERIOD);

Assessed type

Oracle

[alex-ppg]

The Warden has demonstrated how the Chainlink oracle employed by the system does not impose any staleness check, permitting misbehavior in the Chainlink system to not be detected by the system and the system to continue utilizing a stale price, similar to the Luna flash crash.

I believe a medium-risk rating is appropriate given the low likelihood of such an event but the devastating consequences it could result in.

A subset of the duplicates have been penalized for not properly justifying why the staleness check should be applied, for advising an incorrect alleviation (i.e. round ID based), and / or for being of lower quality than acceptable.

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked the issue as selected for report