code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-amphora/blob/179384321c36b669f48bc0485bbc1f807fba8fac/core/solidity/contracts/periphery/oracles/ChainlinkStalePriceLib.sol#L11-L14

Vulnerability details

Impact

Chainlink is heavily relied upon in the Amphora protocol, note that asides this blog from openzeppelin https://blog.openzeppelin.com/secure-smart-contract-guidelines-the-dangers-of-price-oracles/ mentioning that it is possible that Chainlink’s "multisigs immediately block access to price feeds at will". Oracles can also be taken down for safety reasons, which is why it's a pretty popular practise to wrap chainlink queries in a try/catch, and if the call fails for whatever reason the fallback mechanicsm is there to sort things out and prevent a denial of service from occurring when trying to access the price feed.

Proof of Concept

Note that this issue exists in all instances of queries to chainlink, but for breviy reasons, this report would only focus on the instance in ChainlinkStalePriceLib. Here is the getCurrentPrice() function from ChainlinkStalePriceLib.sol:

function getCurrentPrice(AggregatorV2V3Interface _aggregator) internal view returns (uint256 _price) {
  //@audit M chainlink query could revert if the oracle goes offline
  (, int256 _answer,,,) = _aggregator.latestRoundData();
  if (_answer <= 0) revert Chainlink_NegativePrice();
  _price = uint256(_answer);
}

As seen from this the getCurrentPrice() function in the ChainlinkStalePriceLib.sol contract may be susceptible to a Denial of Service (DoS) attack. If the call to latestRoundData() reverts due to any reason, such as the oracle being offline or multisigs deciding to halt the data feed, this easily halts the execution of the getCurrentPrice() function. This incapacitates any processes reliant on this function and impair the functionality of the entire contract or even larger systems relying on it (i.e all functionalities relying on the queries to chainlink's price feed )

Tool used

Manual Audit

Recommendation

Use a try/catch block around the latestRoundData() calls. If these calls revert, the catch block should handle the failure accordingly. This can include a fallback mechanism, an alternative oracle call, or a contingency procedure to pause operations or any reasonable mechanism

Assessed type

DoS

c4-pre-sort commented 1 year ago

minhquanym marked the issue as low quality report

minhquanym commented 1 year ago

Inflated severity. Seems like Low/NC

c4-pre-sort commented 1 year ago

minhquanym marked the issue as primary issue

c4-judge commented 1 year ago

dmvt changed the severity to QA (Quality Assurance)