c4-bot-3 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseOracleHub/OracleHelper.sol#L270

Vulnerability details

Details

According to the project description, wiselending would be launched on both eth mainnet and arbitrum chains. The chainlink pricefeed in OracleHelper validates the chainlink price to ensure it does not return stale price through the _chainlinkIsDead function. Additionally, it also does the check for sequencer uptime if on arbitrum chain through a separate function sequencerIsDead. It then combines both checks in the WiseOracleHub.chainlinkIsDead function. The _chainlinkIsDead validation is used within OracleHelper.getETHPriceInUSD which returns the price of ETH in USD.

src: contracts/WiseOracleHub/OracleHelper.sol
 function getETHPriceInUSD()
        public
        view
        returns (uint256)
    {
@>       if (_chainLinkIsDead(ETH_USD_PLACEHOLDER) == true) {
            revert OracleIsDead();
        }

        return _validateAnswer(
            ETH_USD_PLACEHOLDER
        );
    }

This function is then used in WiseOracleHub.getTokensPriceInUSD and WiseOracleHub.getTokensPriceFromUSD. The issue comes in because using just the _chainlinkIsDead function to get eth price in OracleHelper does not consider whether the current chain is arbitrum.

Impact

A user on arbitrum who calls the OracleHelper.getEthPrice and the other functions which rely on this including WiseOracleHub.getTokensPriceInUSD and WiseOracleHub.getTokensPriceFromUSD either directly or through usage of the protocol may falsely assume that the price it returns is validated on the arbitrum chain although the arbitrum sequencer may be dead. This can also impact other aspects of the protocol as well as any other external protocols which may rely on the data from these functions as these are potentially very important functions.

Proof of Concept

If the user who calls these functions is on arbitrum, they would get an answer which have not passed through the sequencer validation on the arbitrum chain. There may be an assumption that only users on eth mainnet would call these functions, however, this is not explicitly stated or dissallowed.

Tools Used

Manual review

Recommended Mitigation Steps

Use both the chainlinkIsDead and sequencerIsDead functions in the OracleHelper.getETHPriceInUSD`.

 function getETHPriceInUSD()
        public
        view
        returns (uint256)
    {

+     if (sequencerIsDead == true) {
+            revert OracleIsDead();
        }
      if (_chainLinkIsDead(ETH_USD_PLACEHOLDER) == true) {
           revert OracleIsDead();
       }

        return _validateAnswer(
            ETH_USD_PLACEHOLDER
        );
    }

Additionally, if these functions are expected to be called only by eth mainnet users, then it should be explicitly dissallowed if the user is not on eth mainnet.


 function getETHPriceInUSD()
        public
        view
        returns (uint256)
    {
+      if (IS_ARBITRUM_CHAIN == true) {
            revert;
        }

      if (_chainLinkIsDead(ETH_USD_PLACEHOLDER) == true) {
           revert OracleIsDead();
       }

        return _validateAnswer(
            ETH_USD_PLACEHOLDER
        );
    }

Assessed type

Oracle

GalloDaSballo commented 5 months ago

Unclear to me whether the check should be done there, or is done somewhere else

c4-pre-sort commented 5 months ago

GalloDaSballo marked the issue as sufficient quality report

c4-pre-sort commented 5 months ago

GalloDaSballo marked the issue as primary issue