Chainlink's latestRoundData() might return stale or incorrect data

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-frax/blob/main/src/contracts/FraxlendPairCore.sol#L524 https://github.com/code-423n4/2022-08-frax/blob/main/src/contracts/FraxlendPairCore.sol#L532

Vulnerability details

Impact

The only value being checked from the return of latestRoundData() is _answer.

Chainlink will return more fields that can be checked to ensure the data is not stale/incorrect.

Proof of Concept

Lack of checks inside the function updateExchangeRate() might cause incorrect data being processed.

File: contracts/FraxlendPairCore.sol
524: (, int256 _answer, , , ) = AggregatorV3Interface(oracleMultiply).latestRoundData();
532: (, int256 _answer, , , ) = AggregatorV3Interface(oracleDivide).latestRoundData();

https://github.com/code-423n4/2022-08-frax/blob/main/src/contracts/FraxlendPairCore.sol

Recommended Mitigation Steps

Check other fields returned from chainlink to ensure latestRoundData is working as expected.

Make sure to add new custom errors such as OracleStalePrice and OracleIncompleteRound. E.g. the following changes could be made:

$ git diff --no-index FraxlendPairCore.sol.orig FraxlendPairCore.sol
diff --git a/FraxlendPairCore.sol.orig b/FraxlendPairCore.sol
index a712d46..66790c1 100644
--- a/FraxlendPairCore.sol.orig
+++ b/FraxlendPairCore.sol
@@ -521,18 +521,34 @@ abstract contract FraxlendPairCore is FraxlendPairConstants, IERC4626, ERC20, Ow

         uint256 _price = uint256(1e36);
         if (oracleMultiply != address(0)) {
-            (, int256 _answer, , , ) = AggregatorV3Interface(oracleMultiply).latestRoundData();
+            (uint80 _roundID, int256 _answer, , uint256 _timestamp, uint80 _answeredInRound) = AggregatorV3Interface(oracleMultiply).latestRoundData();
             if (_answer <= 0) {
                 revert OracleLTEZero(oracleMultiply);
             }
+            if (_answeredInRound < _roundID) {
+                // Example of a new custom error to handle stale prices.
+                revert OracleStalePrice(_answeredInRound, _roundID);
+            }
+            if (_timestamp == 0) {
+                // Example of a new custom error to handle incomplete rounds.
+                revert OracleIncompleteRound();
+            }
             _price = _price * uint256(_answer);
         }

         if (oracleDivide != address(0)) {
-            (, int256 _answer, , , ) = AggregatorV3Interface(oracleDivide).latestRoundData();
+            (uint80 _roundID, int256 _answer, , uint256 _timestamp, uint80 _answeredInRound) = AggregatorV3Interface(oracleMultiply).latestRoundData();
             if (_answer <= 0) {
                 revert OracleLTEZero(oracleDivide);
             }
+            if (_answeredInRound < _roundID) {
+                // Example of a new custom error to handle stale prices.
+                revert OracleStalePrice(_answeredInRound, _roundID);
+            }
+            if (_timestamp == 0) {
+                // Example of a new custom error to handle incomplete rounds.
+                revert OracleIncompleteRound();
+            }
             _price = _price / uint256(_answer);
         }

[lucyoa]

While this is correct, it is part of "Known Issues" and should not be reported as a part of this engagement.

[amirnader-ghazvini]

Duplicate of #179