code-423n4 / 2022-04-mimo-findings

0 stars 0 forks source link

Missing Validations In Chainlink's `latestRoundData` Function #142

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/inception/priceFeed/ChainlinkInceptionPriceFeed.sol#L74-L80 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/oracles/BalancerV2LPOracle.sol#L101-L102 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/oracles/GUniLPOracle.sol#L103-L104

Vulnerability details

Impact

Here, latestRoundData() is missing an additional validation to ensure that the round is complete.

Proof of Concept

Affected code:

core/contracts/inception/priceFeed/ChainlinkInceptionPriceFeed.sol:74:    (, int256 eurAnswer, , uint256 eurUpdatedAt, ) = _eurOracle.latestRoundData();
core/contracts/inception/priceFeed/ChainlinkInceptionPriceFeed.sol:78:    (, int256 answer, , uint256 assetUpdatedAt, ) = _assetOracle.latestRoundData();
core/contracts/oracles/BalancerV2LPOracle.sol:101:    (, int256 answerA, , uint256 assetUpdatedAtA, ) = oracleA.latestRoundData();
core/contracts/oracles/BalancerV2LPOracle.sol:102:    (, int256 answerB, , uint256 assetUpdatedAtB, ) = oracleB.latestRoundData();
core/contracts/oracles/GUniLPOracle.sol:103:    (, int256 answerA, , uint256 assetUpdatedAtA, ) = oracleA.latestRoundData();
core/contracts/oracles/GUniLPOracle.sol:104:    (, int256 answerB, , uint256 assetUpdatedAtB, ) = oracleB.latestRoundData();

Tools Used

Manual code review. Chainlink best practices.

Recommended Mitigation Steps

Consider adding missing checks.

As an example:

      (uint80 eurRoundID, int256 eurAnswer, , uint256 eurUpdatedAt, uint80 eurAnsweredInRound) = _eurOracle.latestRoundData(); //@audit The eurUpdatedAt data feed property is the timestamp of an answered round and eurAnsweredInRound is the round it was updated in

      require(eurUpdatedAt > 0, "EUR price data is stale"); //@audit A timestamp with zero value means the round is not complete and should not be used.
      require(eurAnswer > 0, "EUR price data not valid");
      require(eurAnsweredInRound >= eurRoundID, "EUR price data is stale"); //@audit If eurAnsweredInRound is less than eurRoundID, the eurAnswer is being carried over. If eurAnsweredInRound is equal to eurRoundID, then the eurAnswer is fresh.
m19 commented 2 years ago

Duplicate of #53