QA Report

[code423n4]

Chainlink oracle aggregator data is insufficiently validated in ConnextPriceOracle.sol

Lines of code

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

Vulnerability details

Impact

The function getPriceFromChainlink() in ConnextPriceOracle.sol fetches the latestRoundData() from a registered aggregator (Chainlink oracle feed) for a specified token. However, neither round completeness or the quoted timestamp are checked to ensure that the reported price is not stale.

Since Connext is creating bridged representations of tokens from other chains, it is vital for the reported prices of tokens to be accurate. While Connext also consults the DEX reported price of the tokens, some pairs with thin liquidity could also return inaccurate 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;
    }

    return 0;
  }

Note that the other returned variables roundId, startedAt, updatedAt, and answeredInRound are omitted from the return result of aggregator.latestRoundData().

Mitigation steps

Add additional validation:

    ...
     (uint80 roundID, int256 price, , uint256 updatedAt, uint80 answeredInRound) = aggregator.latestRoundData();
    require(answeredInRound >= roundID, "ChainLink: Stale price");
    require(updatedAt != 0, "ChainLink: Round not complete");
    ...

Tools Used

Manual review

[jakekidd]

best description of issue with mitigation step here :)

[0xleastwood]

While the issue is valid, this particular part of the codebase is not in active use, however, it may be used in the future. It would make sense to downgrade this to QA.

[IllIllI000]

@0xleastwood what makes you say that it's not in active use? The readme shows how it's being used https://github.com/code-423n4/2022-06-connext/tree/main/contracts#how-to-set-price-information-in-connextpriceoracle

[0xleastwood]

https://github.com/code-423n4/2022-06-connext/tree/main/contracts#how-to-set-price-information-in-connextpriceoracle

Can you find me an example where this is being used anywhere in the bridge transfer logic?

[IllIllI000]

https://github.com/code-423n4/2022-06-connext/tree/main/contracts#how-to-set-price-information-in-connextpriceoracle

Can you find me an example where this is being used anywhere in the bridge transfer logic?

It's deployed along with everything else https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/deploy/02_deployConnext.ts#L159 are you really trying to claim that if two pieces don't directly interact, that you can arbitrarily downgrade any finding in one of them?

[0xleastwood]

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/deploy/02_deployConnext.ts#L159

It's being deployed but not used anywhere. I've spoken to the team about this and they may potentially use the contracts in the future but for now that's not the case. Context matters when determining the severity of an issue.