ast3ros - Price Oracle contract does not work in Arbitrum and Optimism

[sherlock-admin]

ast3ros

medium

Price Oracle contract does not work in Arbitrum and Optimism

Summary

The PriceOracle contract relies on the Chainlink Oracle FeedRegistry to get the price of tokens. However, the FeedRegistry is not available in L2 networks such as Arbitrum and Optimism. This means that the PriceOracle contract will fail to return the price of tokens in these networks.

Vulnerability Detail

The PriceOracle contract uses the Chainlink Oracle FeedRegistry to get the latest round data for a pair of tokens.

    (, int256 price,,,) = registry.latestRoundData(base, quote);

https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/protocol/oracle/PriceOracle.sol#L67

The Iron Bank is deployed in mainnet, Arbitrum and Optimism. However, according to the Chainlink documentation, the FeedRegistry is only available in mainnet and not in Arbitrum and Optimism.

https://docs.chain.link/data-feeds/feed-registry#contract-addresses

This means that the PriceOracle contract will not be able to get the price of tokens in these L2 networks. This will affect the functionalities of the protocol that depend on the token price, such as liquidation.

https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/protocol/pool/IronBank.sol#L827-L828

Impact

The PriceOracle contract will not function properly in L2 networks. This will break the protocol functions that rely on the token price.

Code Snippet

https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/protocol/oracle/PriceOracle.sol#L67
https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/protocol/pool/IronBank.sol#L827-L828

Tool used

Manual Review

Recommendation

Reimplement the PriceOracle contract by reading the price feed from AggregatorV3Interface instead of FeedRegistry. Example:
https://docs.chain.link/data-feeds/l2-sequencer-feeds#example-code

[thangtranth]

Escalate for 10 USDC

Please help me to review. This is not a duplication of #440.

• bin2chen - getPriceFromChainlink() doesn't check If Arbitrum sequencer is down in Chainlink feeds #440 is about checking If Arbitrum sequencer is down.
• This issue is about the current PriceOracle contract is wrongly implemented because the FeedRegistry is not existed in Arbitrum and Optimism.

They are two different issues with two different root causes.

[sherlock-admin]

Escalate for 10 USDC

Please help me to review. This is not a duplication of #440.

• bin2chen - getPriceFromChainlink() doesn't check If Arbitrum sequencer is down in Chainlink feeds #440 is about checking If Arbitrum sequencer is down.
• This issue is about the current PriceOracle contract is wrongly implemented because the FeedRegistry is not existed in Arbitrum and Optimism.

They are two different issues with two different root causes.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[ib-tycho]

Regarding the mistake in the contest details mentioned in the README, we apologize for any confusion caused. When we stated that we would deploy on Arbitrum and Optimism, we meant that we would make the necessary modifications before deployment. This is our standard practice of maintaining contracts with different branches, same as what we did in v1: https://github.com/ibdotxyz/compound-protocol/branches

We are aware of the absence of a registry on OP and Arb, as pointed out by some individuals. We would like to inquire if it is possible to offer the minimum reward for an oracle issue on L2. Thank you.

[0xffff11]

Even though the issue is valid, there will be no deployment on L2s as stated by sponsor. Unfortunately, it is invalid due to a miss-understanding from the docs. As watson said, it is not a duplicate of #440 so it should be de-duplicated and closed.

[thangtranth]

Hi everyone,

I totally understand the reasoning and I don’t mean to push this issue. However, I’m concerned that it will set a precedent when Watsons do not know what the scope is and where the source of truth is for each contest.

There are some duplication of this issue such as #18 and #239 which I believe those Watsons also pay attention to every line of codes and check Oracle registries in each L2 to ensure that the code is safely deployed in stated L2 in Readme.

In my opinion, if this issue is invalid, then all other issues related to L2 such as Sequencer should also be marked as invalid since the code is not used in L2.

[hrishibhat]

Result:
Medium
Has duplicates
Although there was an error in the Readme, this would have been easily handled during deployment.
However, this is still a valid issue from the contest POV.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• thangtranth: accepted

[jacksanford1]

Issue was labeled "Won't Fix" by protocol team. Categorizing as an acknowledged issue.