Price Oracle will return the wrong price for asset if price hits below/above minAnswer/maxPrice

[hats-bug-reporter[bot]]

Github username: @@giorgiodalla Twitter username: 0xAuditism Submission hash (on-chain): 0xf121c61d3ba25c9062b1419cd9667c33771b93ff3d5fecd3160c4bd8a5758655 Severity: low

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.

Attack Scenario\

.ChainlinkAggregators have minPrice and maxPrice circuit breakers built into them. This means that if the price of the asset drops below the minPrice, the protocol will continue to value the token at minPrice instead of it's actual value. This will allow users to take out huge amounts of bad debt and bankrupt the protocol. So once a user sees that the oracle isn't reporting the correct values he can completely abuse the oracle

Attachments

1. Proof of Concept (PoC) File

  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;
  }

We can see here above that the oracle integrator does the usual sanity check for staleness and bigger than 0. But it s not being verfiied if it is within Min-Max.

2. Revised Code File (Optional) Consider adding this line too.

• if (answer >= maxPrice or answer <= minPrice) revert();

[langnavina97]

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.

@GiorgioDalla

[GiorgioDalla]

Thank you for coming back so quickly, since it is true that it will be used for only the performance fee the impact is low, very unlikely to happen. Hence I completely respect your decision, but I haven't see this issue in the google sheets form, would you mind pointing me to it ? If you are aware of it I think it would be wise to explicitly state in the code that this isn't best practice, so that other builders can use your project as a reliable template. Also it will reduce the number of bug bounty inquiries you will have once your project is deployed, saving you precious time.
Finally the mitigation route doesn't require a second oracle feed, it is just a check to see if the price is within min max bounds.