eeshenggoh - No check for Arbitrum/Optimisim sequencer down in Chainlink feeds

[github-actions]

eeshenggoh

high

No check for Arbitrum/Optimisim sequencer 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

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

    function getExpectedExchange(address yieldToken) external view returns (uint256) {
        uint256 expectedExchange;
        address[] memory token = new address[](1);
        uint256 totalToSwap = TokenUtils.safeBalanceOf(rewardToken, address(this));


        // Ensure that round is complete, otherwise price is stale.
        (
            uint80 roundID,
            int256 opToUsd,
            ,
            uint256 updateTime,
            uint80 answeredInRound
        ) = IChainlinkOracle(opToUsdOracle).latestRoundData();
        
        require(
            opToUsd > 0, 
            "Chainlink Malfunction"
        );


        if( updateTime < block.timestamp - 1200 seconds ) {
            revert("Chainlink Malfunction");
        }


        // Ensure that round is complete, otherwise price is stale.
        (
            uint80 roundIDEth,
            int256 ethToUsd,
            ,
            uint256 updateTimeEth,
            uint80 answeredInRoundEth
        ) = IChainlinkOracle(ethToUsdOracle).latestRoundData();
        
        require(
            ethToUsd > 0, 
            "Chainlink Malfunction"
        );


        if( updateTimeEth < block.timestamp - 1200 seconds ) {
            revert("Chainlink Malfunction");
        }

This causes the RewardRouter::distributeRewards to donate undervalued value of rewards.

Impact

Potentially be exploited by malicious actors to gain an unfair advantage,k causing protocol to donate the undervalued rewards.

Code Snippet

https://github.com/sherlock-audit/2024-04-alchemix/blob/32e4902f77b05ea856cf52617c55c3450507281c/v2-foundry/src/utils/collectors/OptimismRewardCollector.sol#L97C1-L130C10

Tool used

Manual Review

Recommendation

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

[midori-fuse]

Escalate

Per the contest README:

Q: If the codebase is to be deployed on an L2, what should be the behavior of the protocol in case of sequencer issues (if applicable)? Should Sherlock assume that the Sequencer won't misbehave, including going offline?

It is acceptable to assume the sequencer won't misbehave or go offline.

Thus, this issue is invalid.

[sherlock-admin3]

Escalate

Per the contest README:

Q: If the codebase is to be deployed on an L2, what should be the behavior of the protocol in case of sequencer issues (if applicable)? Should Sherlock assume that the Sequencer won't misbehave, including going offline?

It is acceptable to assume the sequencer won't misbehave or go offline.

Thus, this issue is invalid.

You've created a valid escalation!

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.

[goheesheng]

Escalate

Per the contest README:

Q: If the codebase is to be deployed on an L2, what should be the behavior of the protocol in case of sequencer issues (if applicable)? Should Sherlock assume that the Sequencer won't misbehave, including going offline?

It is acceptable to assume the sequencer won't misbehave or go offline.

Thus, this issue is invalid.

The term 'misbehave' is slightly ambiguous. Anyways, not sure why Sherlock can assume that L2 sequencer won't misbehave when in practical usage it is possible.

To add on, the developer knows that the L2 sequencer can be down, yet didn't mitigate it.

[Blngogo]

When it comes to security in web3 i don't think that we should rely on assumptions!

[m4ttm]

When it comes to security in web3 i don't think that we should rely on assumptions!

As per the scope of the audit, this was an acceptable assumption to make. We all know this issue, the devs also knew this issue and it was clearly stated to be out of scope.

[goheesheng]

When it comes to security in web3 i don't think that we should rely on assumptions!

As per the scope of the audit, this was an acceptable assumption to make. We all know this issue, the devs also knew this issue and it was clearly stated to be out of scope.

Interesting, if the dev knew the issue, shouldn't the dev implement mitigation, instead of after someone reports it?
I think that those who reported it as an issue assume that the dev didn't know that code snippet is vulnerable regardless of the readme stated as "acceptable".

[m4ttm]

When it comes to security in web3 i don't think that we should rely on assumptions!

As per the scope of the audit, this was an acceptable assumption to make. We all know this issue, the devs also knew this issue and it was clearly stated to be out of scope.

Interesting, if the dev knew the issue, shouldn't the dev implement mitigation, instead of after someone reports it? I think that those who reported it as an issue assume that the dev didn't know that code snippet is vulnerable regardless of the readme stated as "acceptable".

In the context of the audit we're to assume that the sequencer doesn't misbehave or go offline. With those assumptions the snippet is not vulnerable.

[WangSecurity]

Agree with the escalation, as it's said in the README: "It is acceptable to assume the sequencer won't misbehave or go offline".

Planning to accept the escalation and invalidate the issue.

[Nilay27]

Agree with the escalation, as it's said in the README: "It is acceptable to assume the sequencer won't misbehave or go offline".

Planning to accept the escalation and invalidate the issue.

@WangSecurity, during the contest, I had asked the sponsors regarding this; their simple assumption was that the sequencer going down would mean transactions wouldn't go through and wouldn't lead to any fund loss. However, according to them, if it lead to any fund loss, it might be worth considering.

There has been a similar issue where as per Readme such issues were not considered valid, but the final judgment accepted the issue based on two factors:

1. The network which was vulnerable to sequencer was mentioned in ReadMe
2. The concerned owners were restricted.

Reference to judgment can be found here: sherlock-audit/2023-10-notional-judging#2 (comment)

This issue also persists in Alchemix's scenario, given that the Arbitrum is mentioned in the readme and the router owners are restricted.

This was why I submitted this as a bug in issue #170.

I know that as per Sherlock's guidelines, ReadME > Sponsor's chat, but we should consider the fact that when we ask sponsors about the questions in ReadME, they might not be aware of all the ways it could lead to a loss.

This is the case here as the sponsor's assumption in ReadME did not cover all the scenarios and missed out on how it could lead to fund loss.
This could be one reason sponsors have Confirmed this issue and added a Will Fix tag.

Because of these reasons, I feel this should remain valid; the rest is up to you to decide.

[goheesheng]

Agree with the escalation, as it's said in the README: "It is acceptable to assume the sequencer won't misbehave or go offline".

Planning to accept the escalation and invalidate the issue.

@WangSecurity, during the contest, I had asked the sponsors regarding this; their simple assumption was that the sequencer going down would mean transactions wouldn't go through and wouldn't lead to any fund loss. However, according to them, if it lead to any fund loss, it might be worth considering.

There has been a similar issue where as per Readme such issues were not considered valid, but the final judgment accepted the issue based on two factors:

1. The network which was vulnerable to sequencer was mentioned in ReadMe
2. The concerned owners were restricted.

Reference to judgment can be found here: sherlock-audit/2023-10-notional-judging#2 (comment)

This issue also persists in Alchemix's scenario, given that the Arbitrum is mentioned in the readme and the router owners are restricted.

This was why I submitted this as a bug in issue #170.

I know that as per Sherlock's guidelines, ReadME > Sponsor's chat, but we should consider the fact that when we ask sponsors about the questions in ReadME, they might not be aware of all the ways it could lead to a loss.

This is the case here as the sponsor's assumption in ReadME did not cover all the scenarios and missed out on how it could lead to fund loss.
This could be one reason sponsors have Confirmed this issue and added a Will Fix tag.

Because of these reasons, I feel this should remain valid; the rest is up to you to decide.

Well, if let say no one reports this, and the protocol goes live. Sherlock will not be liable but there will definitely be some reputation damage because of the developer wrong assumption of how sequencer works and not mitigate them.

[WangSecurity]

I completely understand both of your points. But, under Sherlock's Rules and README of the contest, this issue has to be invalid.

[Nilay27]

I completely understand both of your points. But, under Sherlock's Rules and README of the contest, this issue has to be invalid.

I understand what you mean, and I won't oppose your decision. Given that this is an ambiguous case, the final call can go either way.

However, I have a suggestion. Since Sherlock highlights that contest README > Protocol documentation > Protocol answers.

Given that often, due to a lack of security implications and attack vector awareness of protocol and developers, should we avoid asking such questions in README, which leads to security flaws?

Asking such questions in README can often lead to answers that then go on to become a rule during the judging process. Thus, situations like this one can occur, where we know that the issue can cause vulnerabilities, but due to the hierarchy, README overwrites everything.

Ideally, the README should only explain how the protocol works, the relevant workflow, and how to run and build the code, along with other instructions.

Adding questions related to security assumptions will often lead to ambiguous scenarios.

Let me know your thoughts on this @WangSecurity; maybe I can open this discussion in the feedback channel.

[WangSecurity]

That's actually the point of README to override Rules and allow protocols to change the rules specifically to their contest. If you want to continue this discussion, you can do so in Discord to not spam here.

[Evert0x]

Result:
Invalid
Has Duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• midori-fuse: accepted