Connext Amarok contest - sorrynotsorry's results

ChainLink price data could be stale

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L122-L137

Vulnerability details

Impact

At ConnextPriceOracle.sol, getPriceFromChainlink() function invokes aggregator.latestRoundData(). However, there are no checks if the return value indicates stale or subzero data. This could lead to wrong prices.

Proof of Concept

  function getPriceFromChainlink(address _tokenAddress) public view returns (uint256) {
    AggregatorV3Interface aggregator = aggregators[_tokenAddress];
    if (address(aggregator) != address(0)) {
      (, int256 answer, , , ) = aggregator.latestRoundData();


      // It's fine for price to be 0. We have two price feeds.
      if (answer == 0) {
        return 0;
      }


      // Extend the decimals to 1e18.
      uint256 retVal = uint256(answer);
      uint256 price = retVal.mul(10**(18 - uint256(aggregator.decimals())));


      return price;
    }

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L122-L137

According to the Chainlink documentation: “A timestamp with zero value means the round is not complete and should not be used.” “if answeredInRound < roundId could indicate stale data.”

Another reference

See example of mitigation here: https://github.com/cryptexfinance/contracts/blob/master/contracts/oracles/ChainlinkOracle.sol#L58-L65

Tools Used

Manual review

Recommended Mitigation Steps

Adding missing the checks to validate the data is not stale

(roundId, rawPrice,, updatedAt, answeredInRound) = aggregator.latestRoundData();
require(rawPrice > 0, "Chainlink price <= 0");
require(updateTime != 0, "Incomplete round");
require(answeredInRound >= roundId, "Stale price");