sherlock-audit / 2023-05-ironbank-judging

2 stars 2 forks source link

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

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

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

0xffff11 commented 1 year ago

Valid medium

ib-tycho commented 1 year ago

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 commented 1 year ago

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

0xffff11 commented 1 year ago

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

MLON33 commented 1 year ago

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