code-423n4 / 2022-08-olympus-findings

5 stars 4 forks source link

Check additional fields returned by `latestRoundData` to ensure price data is not stale/incorrect #468

Closed code423n4 closed 1 year ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/PRICE.sol#L161 https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/PRICE.sol#L170

Vulnerability details

Impact

Not checking additional fields returned by Chainlink might cause incorrect prices being processed.

Proof of Concept

The only values being check from latestRoundData are price and updatedAt.

File: src/modules/PRICE.sol
161: (, int256 ohmEthPriceInt, , uint256 updatedAt, ) = _ohmEthPriceFeed.latestRoundData();
170: (, reserveEthPriceInt, , updatedAt, ) = _reserveEthPriceFeed.latestRoundData();

Recommened Mitigation Steps

Add additional checks for roundId and answeredInRound. Also, add a check to ensure updatedAt is not zero.

$ git diff --no-index PRICE.sol.orig PRICE.sol
diff --git a/PRICE.sol.orig b/PRICE.sol
index 55d85d3..b0792ea 100644
--- a/PRICE.sol.orig
+++ b/PRICE.sol
@@ -158,18 +158,30 @@ contract OlympusPrice is Module {
         uint256 ohmEthPrice;
         uint256 reserveEthPrice;
         {
-            (, int256 ohmEthPriceInt, , uint256 updatedAt, ) = _ohmEthPriceFeed.latestRoundData();
+            (uint256 roundId, int256 ohmEthPriceInt, , uint256 updatedAt, uint256 answeredInRound) = _ohmEthPriceFeed.latestRoundData();
             // Use a multiple of observation frequency to determine what is too old to use.
             // Price feeds will not provide an updated answer if the data doesn't change much.
             // This would be similar to if the feed just stopped updating; therefore, we need a cutoff.
             if (updatedAt < block.timestamp - 3 * uint256(observationFrequency))
                 revert Price_BadFeed(address(_ohmEthPriceFeed));
+            // Example of a new custom error to handle stale prices.
+            if (answeredInRound < roundId)
+                revert Price_Stale();
+            // Example of a new custom error to handle incomplete rounds.
+            if (updatedAt == 0)
+                revert Price_IncompleteRound();
             ohmEthPrice = uint256(ohmEthPriceInt);

             int256 reserveEthPriceInt;
-            (, reserveEthPriceInt, , updatedAt, ) = _reserveEthPriceFeed.latestRoundData();
+            (uint256 roundId, reserveEthPriceInt, , updatedAt, uint256 answeredInRound) = _reserveEthPriceFeed.latestRoundData();
             if (updatedAt < block.timestamp - uint256(observationFrequency))
                 revert Price_BadFeed(address(_reserveEthPriceFeed));
+            // Example of a new custom error to handle stale prices.
+            if (answeredInRound < roundId)
+                revert Price_Stale();
+            // Example of a new custom error to handle incomplete rounds.
+            if (updatedAt == 0)
+                revert Price_IncompleteRound();
             reserveEthPrice = uint256(reserveEthPriceInt);
         }
Oighty commented 2 years ago

Duplicate. See comment on #441.