Skip to content
This repository has been archived by the owner on Dec 17, 2023. It is now read-only.

bin2chen - getPriceFromChainlink() doesn't check If Arbitrum sequencer is down in Chainlink feeds #440

Open
sherlock-admin opened this issue Jun 11, 2023 · 5 comments
Labels
Disagree With Severity The sponsor disputed the severity of this issue Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue Won't Fix The sponsor confirmed this issue will not be fixed

Comments

@sherlock-admin
Copy link
Contributor

bin2chen

medium

getPriceFromChainlink() doesn't check If Arbitrum sequencer is down in Chainlink feeds

Summary

When utilizing Chainlink in L2 chains like Arbitrum, it's important to ensure that the prices provided are not falsely perceived as fresh, even when the sequencer is down. This vulnerability could potentially be exploited by malicious actors to gain an unfair advantage.

Vulnerability Detail

There is no check:
getPriceFromChainlink

    function getPriceFromChainlink(address base, address quote) internal view returns (uint256) {
@>      (, int256 price,,,) = registry.latestRoundData(base, quote);
        require(price > 0, "invalid price");

        // Extend the decimals to 1e18.
        return uint256(price) * 10 ** (18 - uint256(registry.decimals(base, quote)));
    }

Impact

could potentially be exploited by malicious actors to gain an unfair advantage.

Code Snippet

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

Tool used

Manual Review

Recommendation

code example of Chainlink:
https://docs.chain.link/data-feeds/l2-sequencer-feeds#example-code

@github-actions github-actions bot added Medium A valid Medium severity issue Has Duplicates A valid issue with 1+ other issues describing the same vulnerability labels Jun 19, 2023
This was referenced Jun 19, 2023
This was referenced Jun 19, 2023
@0xffff11
Copy link
Collaborator

Valid medium

@ib-tycho
Copy link

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.

@ib-tycho
Copy link

We'll fix this when deploying on L2, but we disagree with Severity. I would consider this as Low

@ib-tycho ib-tycho added Disagree With Severity The sponsor disputed the severity of this issue Will Fix The sponsor confirmed this issue will be fixed labels Jun 22, 2023
@0xffff11
Copy link
Collaborator

According to past reports and sponsor confirmed that they will fix the issue. The issue will remain as a medium.

@sherlock-admin sherlock-admin added the Reward A payout will be made for this issue label Jun 25, 2023
@ibsunhub ibsunhub removed the Will Fix The sponsor confirmed this issue will be fixed label Jul 7, 2023
@MLON33 MLON33 added the Won't Fix The sponsor confirmed this issue will not be fixed label Jul 7, 2023
@MLON33
Copy link

MLON33 commented Jul 7, 2023

Assuming this issue is acknowledged by the protocol team and won’t be fixed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Disagree With Severity The sponsor disputed the severity of this issue Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue Won't Fix The sponsor confirmed this issue will not be fixed
Projects
None yet
Development

No branches or pull requests

5 participants