sherlock-audit / 2023-11-olympus-judging

9 stars 7 forks source link

cu5t0mPe0 - No check for active L2 Sequencer #171

Closed sherlock-admin closed 6 months ago

sherlock-admin commented 6 months ago

cu5t0mPe0

high

No check for active L2 Sequencer

Summary

Using Chainlink in L2 chains such as Arbitrum requires to check if the sequencer is down to avoid prices from looking like they are fresh although they are not according to their [recommendation](https://docs.chain.link/data-feeds/l2-sequencer-feeds#arbitrum)

Vulnerability Detail

The functions getOneFeedPrice and getTwoFeedPriceDiv and getTwoFeedPriceMul in the contract ChainlinkPriceFeeds will call _getFeedPrice, but these functions will not check whether the sequencer has been closed.

    function _getFeedPrice(
        AggregatorV2V3Interface feed_,
        uint256 updateThreshold_,
        uint8 feedDecimals_,
        uint8 outputDecimals_
    ) internal view returns (uint256) {
        FeedRoundData memory roundData;
        {
            try feed_.latestRoundData() returns ( 
                uint80 roundId,
                int256 priceInt,
                uint256 startedAt,
                uint256 updatedAt,
                uint80 answeredInRound
            ) {
                roundData = FeedRoundData(roundId, priceInt, startedAt, updatedAt, answeredInRound);
            } catch (bytes memory) {
                revert Chainlink_FeedInvalid(address(feed_));
            }
        }
        _validatePriceFeedResult(feed_, roundData, block.timestamp, uint256(updateThreshold_));

        uint256 price = uint256(roundData.priceInt);

        return price.mulDiv(10 ** outputDecimals_, 10 ** feedDecimals_);
    }

Although a try-catch statement is used to capture exceptions from external contracts, when the sequencer is closed, it will not send revert. The specific implementation is as follows:

  function getStatusAnswer(bool status) private pure returns (int256) {
    return status ? int256(1) : int256(0);
  }

https://vscode.blockscan.com/arbitrum-one/0xC1303BBBaf172C55848D3Cb91606d8E27FF38428

Impact

If the sequencer goes down, the protocol will allow users to continue to operate at the previous (stale) rates and this can be leveraged by malicious actors to gain unfair advantage.

Code Snippet

https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/modules/PRICE/submodules/feeds/ChainlinkPriceFeeds.sol#L174-L199

Tool used

Manual Review

Recommendation

It is recommended to follow the Chailink [example code](https://docs.chain.link/data-feeds/l2-sequencer-feeds#example-code)

nevillehuang commented 6 months ago

Invalid based on sherlock rules, given external admins are deemed as trusted in contest details here

  1. Chain re-org and network liveness related issues are not considered valid. Exception: If an issue concerns any kind of a network admin (e.g. a sequencer), can be remedied by a smart contract modification, the protocol team considers external admins restricted and the considered network was explicitly mentioned in the contest README, it may be a valid medium. It should be assumed that any such network issues will be resolved within 7 days, if that may be possible.