code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/helper/LPOracle.sol#L56-L65 https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/helper/OracleConvert.sol#L37-L46

Vulnerability details

Impact

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

Proof of Concept

OcaleConvert.sol

function getAnswer(AggregatorV3Interface priceFeed) internal view returns (int256) {
    (
      , 
      int price,
      ,
      uint timeStamp,
    ) = priceFeed.latestRoundData();
    require(timeStamp > 0, "Round not complete");
    return price;
  }

LPOracle.sol

function getAnswer(AggregatorV3Interface priceFeed) internal view returns (int256) {
    (
      , 
      int price,
      ,
      uint timeStamp,
    ) = priceFeed.latestRoundData();
    require(timeStamp > 0, "Round not complete");
    return price;
  }

ChainlinkFeed latestRoundData pulls the associated aggregator and requests round data from it. 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.

Example: TokenA has a minPrice of $1. The price of TokenA drops to $0.10. The aggregator still returns $1 allowing the user to borrow against TokenA as if it is $1 which is 10x it's actual value.

Note: Chainlink oracles are used a just one piece of the OracleAggregator system and it is assumed that using a combination of other oracles, a scenario like this can be avoided. However this is not the case because the other oracles also have their flaws that can still allow this to be exploited. As an example if the chainlink oracle is being used with a UniswapV3Oracle which uses a long TWAP then this will be exploitable when the TWAP is near the minPrice on the way down. In a scenario like that it wouldn't matter what the third oracle was because it would be bypassed with the two matching oracles prices. If secondary oracles like Band are used a malicious user could DDOS relayers to prevent update pricing. Once the price becomes stale the chainlink oracle would be the only oracle left and it's price would be used.

Tools Used

Manual Review

Recommended Mitigation Steps

PriceOrcale should check the returned answer against the minPrice/maxPrice and revert if the answer is outside of the bounds:

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

Assessed type

Invalid Validation

141345 commented 1 year ago

Maybe combine with https://github.com/code-423n4/2023-08-goodentry-findings/issues/10

c4-pre-sort commented 1 year ago

141345 marked the issue as primary issue

c4-sponsor commented 1 year ago

Keref marked the issue as sponsor acknowledged

c4-sponsor commented 1 year ago

Keref marked the issue as disagree with severity

Keref commented 1 year ago

This is acknowledged already and out of scope.

c4-judge commented 1 year ago

gzeon-c4 changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

gzeon-c4 marked the issue as grade-c

Nabeel-javaid commented 1 year ago

Hey! if you see you the known issues on the contest page then you can see that the liveliness of the oracle is a known issue and it is stated that "ave lending pool wouldn't work in case of stale/zero price feed." but there isn't anything mentioned about min/max price check. stale price and min/max is 2 different issues

gzeon-c4 commented 1 year ago

This is a valid issue, but the risk is low. QA score is given according to the overall quality of the QA issues submitted by the warden.

Nabeel-javaid commented 1 year ago

This is a valid issue, but the risk is low. QA score is given according to the overall quality of the QA issues submitted by the warden.

historically this issue has been marked as medium many times, I wonder if its low risk then why haven't this issue been raised before

here are some references

1 2 3