hats-finance / Velvet-Capital-0x0bb0c08fd9eeaf190064f4c66f11d18182961f77

Core smart contracts of Velvet Capital
Other
0 stars 1 forks source link

Unhandled chainlink revert would lock price oracle access #14

Open hats-bug-reporter[bot] opened 1 week ago

hats-bug-reporter[bot] commented 1 week ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0xeb8df6f7a7695cf9dd0cdf1b0ed0c5bda5bdc20ba790bc9a3a95a5b4aa24a512 Severity: medium

Description: Description\

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();

     . . . some code

and In PriceOracleL2.sol:

  function _latestRoundData(
    address base,
    address quote
  ) internal view override returns (int256) {
    // Retrieve the latest round data from the sequencer uptime feed
    (, int256 answer, uint256 startedAt, , ) = sequencerUptimeFeed
      .latestRoundData();

   . . .  some code

The above functions makes use of Chainlink's latestRoundData() to get the latest price. However, there is no fallback logic to be executed when the access to the Chainlink data feed is denied by Chainlink's multisigs. 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.

Referring chainlink documentation on how chainlink services are updated. Please note chainlink multisig holds the power of Chainlink’s multisigs can immediately block access to price feeds at will.

Onchain updates take place at the smart contract level, where a multi-signature safe (multisig) is used to modify onchain parameters relating to a Chainlink service. This can include replacing faulty nodes on a specific oracle network, introducing new features such as Offchain Reporting, or resolving a smart contract logic error. The multisig-coordinated upgradability of Chainlink services involves time-tested processes that balance collusion-resistance with the flexibility required to implement improvements and adjust parameters.

Reference link: https://chain.link/faqs#how-are-chainlink-services-updated

Openzeppelin reference: 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.

This is similar Medium severity finding found in juicebox audit at code4rena. Reference link:- https://solodit.xyz/issues/m-09-unhandled-chainlink-revert-would-lock-all-price-oracle-access-code4rena-juicebox-juicebox-v2-contest-git

Impact\ Call to latestRoundData could potentially revert and make it impossible to query any prices. This could lead to permanent denial of service.

Recommendations\ 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.

langnavina97 commented 1 week ago

If the price feed is not available, the transaction should revert to ensure that no fees are charged without accurate price data. We will add warnings on the front-end to inform asset managers about the status of the price feeds that can be used for charging performance fees. This proactive approach helps maintain transparency and allows asset managers to make informed decisions about fee assessments. Additionally, by reverting the transaction in the absence of a valid price feed, we prevent any incorrect calculations and maintain the integrity of the system.