dimah7 - Missing circuit breaker checks in getExpectedExchange() for Chainlink's price feed

[github-actions]

dimah7

medium

Missing circuit breaker checks in getExpectedExchange() for Chainlink's price feed

Summary

The OptimismRewardCollector::getExpectedExchange function relies on Chainlink oracle to fetch the prices of OP/USD and ETH/USD. However Chainlink aggregators have a built in circuit breaker if the price of an asset goes outside of a predetermined price band. If an asset experiences a major drop or rise in value, the price feeds will continue to return minAnswer/maxAnswer instead of the actual price. A real-world example can be seen here. In our case wrong expectedExchange value will be returned, which is then subsequently used as an input parameter in claimAndDonateRewards function:

/// @dev Distributes grant rewards and triggers reward collector to claim and donate
    function distributeRewards(address vault) external returns (uint256) {
        .
        .
        .
    @>    return IRewardCollector(rewards[vault].rewardCollectorAddress).claimAndDonateRewards(vault, IRewardCollector(rewards[vault].rewardCollectorAddress).getExpectedExchange(vault) * slippageBPS / BPS);
    }


    function claimAndDonateRewards(address token, uint256 minimumAmountOut) external returns (uint256) {
        require(msg.sender == rewardRouter, "Must be Reward Router"); 

        // Amount of reward token claimed plus any sent to this contract from grants.
        uint256 amountRewardToken = IERC20(rewardToken).balanceOf(address(this));

        if (amountRewardToken == 0) return 0;

        if (debtToken == 0xCB8FA9a76b8e203D8C3797bF438d8FB81Ea3326A) {
            // Velodrome Swap Routes: OP -> USDC -> alUSD
            IVelodromeSwapRouter.Route[] memory routes = new IVelodromeSwapRouter.Route[](2);
            routes[0] = IVelodromeSwapRouter.Route(0x4200000000000000000000000000000000000042, 0x7F5c764cBc14f9669B88837ca1490cCa17c31607, false, 0xF1046053aa5682b4F9a81b5481394DA16BE5FF5a);
            routes[1] = IVelodromeSwapRouter.Route(0x7F5c764cBc14f9669B88837ca1490cCa17c31607, 0xCB8FA9a76b8e203D8C3797bF438d8FB81Ea3326A, true, 0xF1046053aa5682b4F9a81b5481394DA16BE5FF5a);
            TokenUtils.safeApprove(rewardToken, swapRouter, amountRewardToken);
            IVelodromeSwapRouter(swapRouter).swapExactTokensForTokens(amountRewardToken, minimumAmountOut, routes, address(this), block.timestamp);
        } else if (debtToken == 0x3E29D3A9316dAB217754d13b28646B76607c5f04) {
            // Velodrome Swap Routes: OP -> alETH
            IVelodromeSwapRouter.Route[] memory routes = new IVelodromeSwapRouter.Route[](2);
            routes[0] = IVelodromeSwapRouter.Route(0x4200000000000000000000000000000000000042, 0x4200000000000000000000000000000000000006, false, 0xF1046053aa5682b4F9a81b5481394DA16BE5FF5a);
            routes[1] = IVelodromeSwapRouter.Route(0x4200000000000000000000000000000000000006, 0x3E29D3A9316dAB217754d13b28646B76607c5f04, true, 0xF1046053aa5682b4F9a81b5481394DA16BE5FF5a);
            TokenUtils.safeApprove(rewardToken, swapRouter, amountRewardToken);
            IVelodromeSwapRouter(swapRouter).swapExactTokensForTokens(amountRewardToken, minimumAmountOut, routes, address(this), block.timestamp);
        } else {
            revert IllegalState("Reward collector `debtToken` is not supported");
        }
        .
        .
        .
    }

As can be seen this will result in the collector receiving the wrong amount after the swap or the swap transaction reverting. Code snippet to swapExactTokensForTokens function here

Vulnerability Detail

There isn't a check for the price to be in the preffered range, but only to not be zero or non-negative:

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");
        }
        .
        .
        .

Similar issues can be seen here:
sherlock-audit/2023-02-blueberry-judging#18

https://solodit.xyz/issues/m-2-priceoracle-will-use-the-wrong-price-if-the-chainlink-registry-returns-price-outside-minmax-range-sherlock-none-iron-bank-git

https://solodit.xyz/issues/m-02-missing-check-for-the-maxmin-price-in-the-chainlinkoraclesol-contract-code4rena-moonwell-moonwell-git

https://solodit.xyz/issues/m-09-missing-circuit-breaker-checks-in-ethpercvx-for-chainlinks-price-feed-code4rena-asymmetry-finance-asymmetry-finance-git

Impact

Can result in the collector claiming different values than it should or cause the swap function to revert, because of insufficient output amount.

Code Snippet

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

Tool used

Manual Review

Recommendation

Implement the proper check for each returned price:

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.
        // @audit check if price cant be stale for more info JPEG-M06
        (
            uint80 roundID,
            int256 opToUsd,
            ,
            uint256 updateTime,
            uint80 answeredInRound
        ) = IChainlinkOracle(opToUsdOracle).latestRoundData();
        
        require(
            opToUsd > 0, 
            "Chainlink Malfunction"
        );
+       require(opToUsd >= minPrice && opToUsd <= maxPrice, "non-acceptable price"); 

        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"
        );
+       require(ethToUsd >= minPrice && ethToUsd <= maxPrice, "non-acceptable price"); 
        .
        .
        .
    }

Duplicate of #14

[Nilay27]

@Hash01011122 This should not be a duplicate of #14, as it is specifically for circuit breakers and not for Sequencer downtime. This is a separate issue from that as the root cause is different.

[Blngogo]

@Hash01011122 Based on Sherlock's duplication rules, this finding doesn't meet the criteria to be considered as duplicate of #14 , since fixing the root cause of #14 will not resolve this issue also

[ABDuullahi]

Escalate

Based on the above comments.

[sherlock-admin3]

Escalate

Based on the above comments.

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.

[WangSecurity]

As it was correctly mentioned by Watsons on Discrod, minAnswer and maxAnswer are no longer used. Hence, this report should stay invalid. Since I'm planning to invalidate #14 as well, this rejection will be rejected as well cause all issues will be invalid.

If #14 remains valid, these and other issues about min and max prices will be invalidated and escalation accepted.

[AuditorPraise]

As it was correctly mentioned by Watsons on Discrod, minAnswer and maxAnswer are no longer used. Hence, this report should stay invalid. Since I'm planning to invalidate #14 as well, this rejection will be rejected as well cause all issues will be invalid.

If #14 remains valid, these and other issues about min and max prices will be invalidated and escalation accepted.

The escalation is based on how this issue is judged as a dup of #14 when in fact they don't have the same root cause.

Either way i think since the escalation is solely on how the issue is wrongly duplicated, it's rejection or acceptance should lie solely on that, as they're clearly not duplicates.

[Nilay27]

As it was correctly mentioned by Watsons on Discrod, minAnswer and maxAnswer are no longer used. Hence, this report should stay invalid. Since I'm planning to invalidate #14 as well, this rejection will be rejected as well cause all issues will be invalid.

If #14 remains valid, these and other issues about min and max prices will be invalidated and escalation accepted.

@WangSecurity I would like to add my perspective and would suggest that this issue should remain valid, rest is up to you to decide.

There seems to be certain confusion around how minAnswer/maxAnswer is deprecated/not used.

The reason these are not used on most feeds are because most feeds would return the minAnswer as 1 (i.e 1e-8) but that is not the case with ethToUSDOracle.

The ethToUSDOracle address is 0x13e3Ee699D1909E989722E753853AE30b17e08c5.

If we check the aggregator, the address of the aggregator is 0x02f5E9e9dcc66ba6392f6904D5Fcf8625d9B19C9.

If we check the minAnswer for this, the value is 1e9 where the decimals are 8.

Hence, the minimum price that it will return for ETH is $10, which is not completely unreasonable.

So, the protocol should define the range of acceptable values as minPrice and maxPrice and check that the answers returned by the protocol are within this range.

I mentioned the same thing in #163, but since I do not have escalation rights now, it seems that went unnoticed.

[WangSecurity]

Thank you for that input @Nilay27 for that insightful input. But I still believe these are sanity checks and recommendation, so this report and the duplicates of 80 should be low/info. Hence, remain invalid.

@AuditorPraise thank you for that comment and it's a very good remark. The problem is that if #14 is invalidated after escalations, then based on Sherlock's rules of escalations, this escalation will be rejected cause it doesn't effect the reward distribution.

[Nilay27]

Thank you for that input @Nilay27 for that insightful input. But I still believe these are sanity checks and recommendation, so this report and the duplicates of 80 should be low/info. Hence, remain invalid.

@AuditorPraise thank you for that comment and it's a very good remark. The problem is that if #14 is invalidated after escalations, then based on Sherlock's rules of escalations, this escalation will be rejected cause it doesn't effect the reward distribution.

Thanks for the feedback, @WangSecurity. I appreciate your input and will not dispute the logic.

I submitted this issue because, until fairly recently, such issues were awarded medium severity on Sherlock.

Ref:
- Dodo
- Blueberry

But it seems now we are moving away from such findings and categorizing them as low/info.

[WangSecurity]

Thank you for these references, but Historical Decisions are not sources of truth under Sherlock's Rules. And I of course don't say it's wrong, but see this a low/info severity.

[Hash01011122]

This issue can be invalidated

[Evert0x]

Result:
Invalid
Duplicate of #14

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• ABDUullahi: rejected