Avci - _validateAndGetPrice() doesn't check If Arbitrum sequencer is down in Chainlink feeds

[sherlock-admin]

Avci

medium

_validateAndGetPrice() 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:

solidity function _validateAndGetPrice(AggregatorV2V3Interface feed_, uint48 updateThreshold_)
        internal
        view
        returns (uint256)
    {
        // Get latest round data from feed
        (uint80 roundId, int256 priceInt, , uint256 updatedAt, uint80 answeredInRound) = feed_
            .latestRoundData();
        // @audit check if Arbitrum L2 sequencer is down in Chainlink feeds: medium
        // Validate chainlink price feed data
        // 1. Answer should be greater than zero
        // 2. Updated at timestamp should be within the update threshold
        // 3. Answered in round ID should be the same as the round ID
        if (
            priceInt <= 0 ||
            updatedAt < block.timestamp - uint256(updateThreshold_) ||
            answeredInRound != roundId
        ) revert BondOracle_BadFeed(address(feed_));
        return uint256(priceInt);
    }

Impact

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

Code Snippet

https://github.com/sherlock-audit/2023-02-bond-0xdanial/blob/0d6f979c9f361bc1101f429b3bb09264577b9a71/bonds/src/BondChainlinkOracle.sol#L129

Tool used

Manual Review

Recommendation

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

[Oighty]

Agree this should be fixed for using the Chainlink Oracle Contract on L2s. I think the best way to handle is to have a mainnet version of the contract (as is) and L2 version of the contract which implements the sequencer feed check.

[UsmannK]

Escalate for 10 USDC.

Watson states that the arbitrum sequencer may temporarily go down and cause stale prices to be read from the oracle. This is incorrect; the arbitrum sequencer going down cannot result in stale prices to be accepted.

Stale prices will have an old updatedAt timestamp and be rejected by the following code:
https://github.com/sherlock-audit/2023-02-bond/blob/8a326a4b39fdaf9eaf5911cfd3e9676a83c24a58/bonds/src/BondChainlinkOracle.sol#L141-L146

        // Validate chainlink price feed data
        // 1. Answer should be greater than zero
        // 2. Updated at timestamp should be within the update threshold
        // 3. Answered in round ID should be the same as the round ID
        if (
            priceInt <= 0 ||
            updatedAt < block.timestamp - uint256(updateThreshold_) ||
            answeredInRound != roundId
        ) revert BondOracle_BadFeed(address(feed_));

The watson's link (https://docs.chain.link/data-feeds/l2-sequencer-feeds#arbitrum) is actually a metadata feed about historical uptime/downtime data that is not related to the supposed issue.

[sherlock-admin]

Escalate for 10 USDC.

Watson states that the arbitrum sequencer may temporarily go down and cause stale prices to be read from the oracle. This is incorrect; the arbitrum sequencer going down cannot result in stale prices to be accepted.

Stale prices will have an old updatedAt timestamp and be rejected by the following code:
https://github.com/sherlock-audit/2023-02-bond/blob/8a326a4b39fdaf9eaf5911cfd3e9676a83c24a58/bonds/src/BondChainlinkOracle.sol#L141-L146

        // Validate chainlink price feed data
        // 1. Answer should be greater than zero
        // 2. Updated at timestamp should be within the update threshold
        // 3. Answered in round ID should be the same as the round ID
        if (
            priceInt <= 0 ||
            updatedAt < block.timestamp - uint256(updateThreshold_) ||
            answeredInRound != roundId
        ) revert BondOracle_BadFeed(address(feed_));

The watson's link (https://docs.chain.link/data-feeds/l2-sequencer-feeds#arbitrum) is actually a metadata feed about historical uptime/downtime data that is not related to the supposed issue.

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.

[Oighty]

Issue fixed here: https://github.com/Bond-Protocol/bonds/pull/53

[xiaoming9090]

Fixed in https://github.com/Bond-Protocol/bonds/pull/53

[hrishibhat]

Escalation rejected

Updating the escalation resolution.
Considering this issue as a valid medium, additional sponsor comments:

If it updates again within the update threshold. The feeds typically can update several times within a threshold period if the price is moving a lot
when the sequencer is down, the new price won't be reported to the chain. the feed on the L2 will return the value it had when it went down

[sherlock-admin]

Escalation rejected

Updating the escalation resolution.
Considering this issue as a valid medium, additional sponsor comments:

If it updates again within the update threshold. The feeds typically can update several times within a threshold period if the price is moving a lot
when the sequencer is down, the new price won't be reported to the chain. the feed on the L2 will return the value it had when it went down

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.