29068 - [SC - Medium] AaveOracle contract does not verify price stale...

Submitted on Mar 6th 2024 at 14:02:31 UTC by @Paludo0x for Boost | ZeroLend

Report ID: #29068

Report type: Smart Contract

Report severity: Medium

Target: https://explorer.zksync.io/address/0x785765De3E9ac3D8eEb42B4724A7FEA8990142B8

Impacts:

• Direct theft of any user funds, whether at-rest or in-motion, other than unclaimed yield

Description

Brief/Intro

AaveOracle contract relies on Pyth oracles to retrieve assets price feeds and the actual implementation doesn't check whether prices are fresh or staled. Assets prices are retrieved by means of function AaveOracle::getAssetPrice(). For instance this function is called by LiquidationLogic::_calculateAvailableCollateralToLiquidate which "calculates how much of a specific collateral can be liquidated, given a certain amount of debt asset". Therefore a wrong calculation can lead to liquidate q wrong amount of base assets.

Vulnerability Details

This is the code snippet of function AaveOracle::getAssetPrice().

  function getAssetPrice(address asset) public view override returns (uint256) {
    AggregatorInterface source = assetsSources[asset];

    if (asset == BASE_CURRENCY) {
      return BASE_CURRENCY_UNIT;
    } else if (address(source) == address(0)) {
      return _fallbackOracle.getAssetPrice(asset);
    } else {
      int256 price = source.latestAnswer();
      if (price > 0) {
        return uint256(price);
      } else {
        return _fallbackOracle.getAssetPrice(asset);
      }
    }
  }

The price oracle address is stored in mapping assetsSources[asset]. The price is retrieved by means of int256 price = source.latestAnswer(); and only if price = 0the _fallbackOracle is called. Therefore even if price is far from the actual market price it's deemed acceptable.

Let's take as an example https://explorer.zksync.io/address/0xf531672C92Ad4658c54B4fBE855029Df43c57390#contract which is the oracle for asset 0x4B9eb6c0b6ea15176BBF62841C6B2A8a398cb656 (Dai Stablecoin).

You can see that PythAggregatorV3::latestAnswer() calls pyth.getPriceUnsafe(priceId);

    function latestAnswer() public view virtual returns (int256) {
        PythStructs.Price memory price = pyth.getPriceUnsafe(priceId);
        return int256(price.price);
    }

pyth address correspond to 0xf087c864AEccFb6A2Bf1Af6A0382B0d0f6c5D834 which is a proxy contract. While the implementation can be found here: https://explorer.zksync.io/address/0xb1b239054fa2e37da736e43102d175f21c5f7450#contract

You can see that getPriceUnsafe just query the price feed without checking if it is up to date

function getPriceUnsafe(
    bytes32 id
) public view virtual override returns (PythStructs.Price memory price) {
    PythStructs.PriceFeed memory priceFeed = queryPriceFeed(id);
    return priceFeed.price;
}

Impact Details

This are examples of where AaveOracle::getAssetPrice() is used:

• LiquidationLogic::_calculateAvailableCollateralToLiquidate which is called by LiquidationLogic::executeLiquidationCall which i turns is called by Pool::liquidationCall`

• GenericLogic::calculateUserAccountData retrieves assets price to verify account health factor, and it's called in turn by ValidationLogic::validateHealthFactor or ValidationLogic.validateBorrow

That means that if price is staled, especially in a turbolent market condition, can lead to incorrect liquidations or worse, lack of liquidation, and health factor wrong calculation

Fix suggestion

The advice is to implement a verification if the feed last update is within a thresold by means of PythAggregatorV3::latestTimestamp()

function latestTimestamp() public view returns (uint256) {
        PythStructs.Price memory price = pyth.getPriceUnsafe(priceId);
        return price.publishTime;
    }

Proof of concept

The requirement of price staleness verification is suggested by Pyth protocol in NATSPEC of function pyth.getPriceUnsafe(priceId);

 /// @notice Returns the price of a price feed without any sanity checks.
    /// @dev This function returns the most recent price update in this contract without any recency checks.
    /// This function is unsafe as the returned price update may be arbitrarily far in the past.
    ///
    /// Users of this function should check the `publishTime` in the price to ensure that the returned price is
    /// sufficiently recent for their application. If you are considering using this function, it may be
    /// safer / easier to use either `getPrice` or `getPriceNoOlderThan`.
    /// @return price - please read the documentation of PythStructs.Price to understand how to use this safely.

Next the following sentence can be found at function docs: https://docs.pyth.network/price-feeds/api-reference/evm/get-price-unsafe

"This function may return a price from arbitrarily far in the past. It is the caller's responsibility to check the returned publishTime to ensure that the update is recent enough for their use case."