nilay27 - OptimismRewardCollector::getExpectedExchange() is missing max/min price check for ChainlinkOracle's latestRoundData

[github-actions]

nilay27

medium

OptimismRewardCollector::getExpectedExchange() is missing max/min price check for ChainlinkOracle's latestRoundData

Summary

getExpectedExchange uses Chainlink's latestRoundData method to fetch the prices for opToUsd and ethToUsd.
The issue is that Chainlink aggregators have a built-in circuit breaker, that is, if an asset experiences a huge drop in value (i.e. LUNA crash) the price of the oracle will continue to return the minPrice instead of the actual price of the asset.

Vulnerability Detail

RewardRouter::distributeRewards() would fetch the wrong amount via IRewardCollector(rewards[vault].rewardCollectorAddress).getExpectedExchange(vault) and will call claimAndDonateRewards() with wrong minimumAmountOut, thus causing significant loss in getting the debtToken via exchange.

Something similar happened to Venus on BSC when LUNA imploded.

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,  // @audit : no check for min/max price in Chinlink Oracle 
            ,
            uint256 updateTime,
            uint80 answeredInRound
        ) = IChainlinkOracle(opToUsdOracle).latestRoundData();
        
      ........

        // Ensure that round is complete, otherwise price is stale.
        (
            uint80 roundIDEth,
            int256 ethToUsd,  // @audit : no check for min/max price in Chinlink Oracle 
            ,
            uint256 updateTimeEth,
            uint80 answeredInRoundEth
        ) = IChainlinkOracle(ethToUsdOracle).latestRoundData();
               

        ........
      
    .......
        return expectedExchange;
    }

Impact

In the event that an asset crashes (i.e. LUNA), the oracle prices will give false values leading to wrong amount/failure to exchange of DebtToken, leading to breaking of functionality of distributeRewards in RewardRouter and claimAndDonateRewards in OptimismRewardCollector

Code Snippet

https://github.com/sherlock-audit/2024-04-alchemix/blob/main/v2-foundry/src/utils/collectors/OptimismRewardCollector.sol#L97-L130

Tool used

Manual Review

Recommendation

Some check like this can be added to avoid returning of the min price or the max price in case of the price crashes to o that when the price edges close to minAnswer or maxAnswer it starts reverting, to avoid consuming stale prices when Chainlink freezes.

          require(answer < _maxPrice, "Upper price bound breached");
          require(answer > _minPrice, "Lower price bound breached");

[Nilay27]

@Hash01011122 This should be a duplicate of #80.

Also, I think this should be a valid finding because as per the codeblock, 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 must define the range of acceptable values as minPrice and maxPrice and check that the answers returned by the protocol are within this range.

As per the recommendation below:

Some check like this can be added to avoid returning of the min price or the max price in case of the price crashes to o that when the price edges close to minAnswer or maxAnswer it starts reverting.

There have been similar findings that have been validated in the past:

1. Sherlock:

   • Dodo
   • Blueberry

2. C4Rena:

   • Moonwell
   • Asymmetry