Juicebox V2 contest - bardamu's results

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBChainlinkV3PriceFeed.sol#L44

Vulnerability details

Impact

Price data returned by latestRoundData could be stale. This would create a situation where any contract consuming price information from the configured Chainlink aggregator would rely on an incorrect asset price.

Proof of Concept

As can be seen in function currentPrice the answer from feed.latestRoundData() is assumed to be valid if it is non-zero and returned to the caller. However, aggregator data could be stale and therefore be outdated, which would propagate an incorrect price.

Tools Used

vim

Recommended Mitigation Steps

The caller should always check that latestRoundData is returning sane and up to date values.

Properties updatedAt and answeredInRound are of particular interest here. For example, answeredInRound could be checked against the current roundId to verify price information has been updated within a certain threshold.

function latestRoundData()
public
override
view
checkAccess()
returns (
  uint80 roundId,
  int256 answer,
  uint256 startedAt,
  uint256 updatedAt,
  uint80 answeredInRound
)
{
return super.latestRoundData();
}

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBChainlinkV3PriceFeed.sol#L44

Vulnerability details

Impact

Call to latestRoundData could potentially revert and make it impossible to query any prices. Feeds cannot be changed after they are configured (https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBPrices.sol#L115) so this would result in a permanent denial of service.

Proof of Concept

Chainlink's multisigs can immediately block access to price feeds at will. Therefore, to prevent denial of service scenarios, it is recommended to query Chainlink price feeds using a defensive approach with Solidity’s try/catch structure. In this way, if the call to the price feed fails, the caller contract is still in control and can handle any errors safely and explicitly.

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBPrices.sol#L69

if (_feed != IJBPriceFeed(address(0))) return _feed.currentPrice(_decimals);

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBChainlinkV3PriceFeed.sol#L42-L44

function currentPrice(uint256 _decimals) external view override returns (uint256) {
  // Get the latest round information. Only need the price is needed.
  (, int256 _price, , , ) = feed.latestRoundData();

Refer to https://blog.openzeppelin.com/secure-smart-contract-guidelines-the-dangers-of-price-oracles/ for more information regarding potential risks to account for when relying on external price feed providers.

Tools Used

vim

Recommended Mitigation Steps

Surround the call to latestRoundData() with try/catch instead of calling it directly. In a scenario where the call reverts, the catch block can be used to call a fallback oracle or handle the error in any other suitable way.