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

2 stars 1 forks source link

Chainlink oracle data feed is not further validated and can return stale answer #311

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

Although the protocol recognizes that Chainlink oracles can provide outdated answers, using stale answers without further validation might not be a good practice. Moreover, in the _updateExchangeRate function, where the latestRoundData method is used, there are already checks that can possibly revert, such as if (_answer <= 0) {revert OracleLTEZero(oracleMultiply)} and if (_exchangeRate > type(uint224).max) revert PriceTooLarge(). Hence, adding more require statements that further validate whether the answers returned by latestRoundData are stale or not is appeared to be consistent.

Proof of Concept

The following steps can occur.

  1. When calling the following _updateExchangeRate function, (, int256 _answer, , , ) = AggregatorV3Interface(oracleMultiply).latestRoundData() and (, int256 _answer, , , ) = AggregatorV3Interface(oracleDivide).latestRoundData(); are executed.

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

    function _updateExchangeRate() internal returns (uint256 _exchangeRate) {
        ...

        uint256 _price = uint256(1e36);
        if (oracleMultiply != address(0)) {
            (, int256 _answer, , , ) = AggregatorV3Interface(oracleMultiply).latestRoundData();
            if (_answer <= 0) {
                revert OracleLTEZero(oracleMultiply);
            }
            _price = _price * uint256(_answer);
        }

        if (oracleDivide != address(0)) {
            (, int256 _answer, , , ) = AggregatorV3Interface(oracleDivide).latestRoundData();
            if (_answer <= 0) {
                revert OracleLTEZero(oracleDivide);
            }
            _price = _price / uint256(_answer);
        }

        ...
    }
  1. The values of _answer returned by the two latestRoundData function calls are further checked against 0.
  2. Yet, there are no checks for verifying returned variables other than _answer. Hence, there is no guarantee if the returned answers are stale.

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2022-08-frax/blob/main/src/contracts/FraxlendPairCore.sol#L516-L547 can be changed to the following code.

    function _updateExchangeRate() internal returns (uint256 _exchangeRate) {
        ...

        uint256 _price = uint256(1e36);
        if (oracleMultiply != address(0)) {
            (uint80 _roundId, int256 _answer, , uint256 _timestamp, uint80 _answeredInRound) = AggregatorV3Interface(oracleMultiply).latestRoundData();
            require(_answeredInRound >= _roundId, "answer is stale");
            require(_timestamp > 0, “round is incomplete”);
            if (_answer <= 0) {
                revert OracleLTEZero(oracleMultiply);
            }
            _price = _price * uint256(_answer);
        }

        if (oracleDivide != address(0)) {
            (uint80 _roundId, int256 _answer, , uint256 _timestamp, uint80 _answeredInRound) = AggregatorV3Interface(oracleDivide).latestRoundData();
            require(_answeredInRound >= _roundId, "answer is stale");
            require(_timestamp > 0, “round is incomplete”);
            if (_answer <= 0) {
                revert OracleLTEZero(oracleDivide);
            }
            _price = _price / uint256(_answer);
        }

        ...
    }
amirnader-ghazvini commented 2 years ago

Duplicate of #179