Chainlink oracle will return the wrong price if the aggregator hits minAnswer

[hats-bug-reporter[bot]]

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0x4a3ccd98ca0a903ed3a33296e1fc25a0689969201d2fcae12efb73956506a0f4 Severity: medium

Description: Description\ Chainlink aggregators have a built in circuit breaker if the price of an asset goes outside of a predetermined price band. The result is that if an asset experiences a huge drop in value (i.e. LUNA crash) the price of the oracle will continue to return the minPrice instead of the actual price of the asset. This would allow user to continue borrowing with the asset but at the wrong price. This is exactly what happened to Venus on BSC when LUNA imploded.

Reference link- https://rekt.news/venus-blizz-rekt/

Affected code: https://github.com/Velvet-Capital/velvet-core/blob/849629b1aacf32d84634d8c4ef1378527bce3bb3/contracts/oracle/PriceOracle.sol#L33

Velvet contracts have used chainlink pricefeeds which is implemented as below in PriceOracle.sol:

  function _latestRoundData(
    address base,
    address quote
  ) internal view override returns (int256) {
    // Fetch the latest round data from the aggregator for the given token pair.
    (, int256 answer, , uint256 updatedAt, ) = tokenPairToAggregator[base]
      .aggregators[quote]
      .latestRoundData();

    // Validate that the retrieved price is a valid non-zero value.
    if (updatedAt + oracleExpirationThreshold < block.timestamp)
      revert ErrorLibrary.PriceOracleExpired();

    if (answer <= 0) revert ErrorLibrary.PriceOracleInvalid();

    return answer;
  }

Here, the function does not check the price acceptable range which is prone to similar issue as happened with LUNA.

Recommendations\ Consider using the following checks.

For example:

(uint80, int256 answer, uint, uint, uint80) = oracle.latestRoundData();

// minPrice check
require(answer > minPrice, "Min price exceeded");
// maxPrice check
require(answer < maxPrice, "Max price exceeded");

[langnavina97]

DUPLICATE #2

We are aware of the dependency on Chainlink. We decided not to implement a secondary oracle to cross-check the values because the price oracle is only being used for calculating the performance fee.

This is out of scope, it was already pointed out by the auditors.

@0xRizwan