Misalignment Due to Hardcoded Decimal Assumption in Chainlink Oracle Integration

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Oracle/RenzoOracle.sol#L79-L80
https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Oracle/RenzoOracle.sol#L96-L97

Vulnerability details

Impact

The impact of this issue stems from HAL-01 of the Halborn audit for an inadequate mitigation. Currently, RenzoOracle.sol assumes that all Chainlink oracles will return prices with a decimal precision of 18. This assumption is hardcoded into the contract’s logic for price calculations. If Chainlink or any other used oracle service changes the decimal precision for any price feed, the smart contract will still perform calculations assuming 18 decimals, which can lead to significant financial discrepancies. This could result in incorrect asset valuations, under or over-issuance of tokens, and potentially exploitable conditions if the discrepancy is identified by malicious actors. The financial stakes and trust in the protocol could be severely compromised.

Specifically, the following two calls will be affected when referencing/calling:

1. renzoOracle.lookupTokenValue by RestakeManager.calculateTVLs()
2. renzoOracle.lookupTokenAmountFromValue by WithdrawQueue.withdraw()

Proof of Concept

The setOracleAddress function explicitly checks and enforces that the decimals for a newly set oracle address are 18:

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Oracle/RenzoOracle.sol#L60-L62

        // Verify that the pricing of the oracle is 18 decimals - pricing calculations will be off otherwise
        if (_oracleAddress.decimals() != 18)
            revert InvalidTokenDecimals(18, _oracleAddress.decimals());

However, if the decimals of an oracle were to change after this initial setting (which is possible if Chainlink updates an oracle contract), the lookupTokenValue and lookupTokenAmountFromValue functions do not check or adapt to this change, as they use a hardcoded scale factor based on 18 decimals:

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Oracle/RenzoOracle.sol#L79-L80

        // Price is times 10**18 ensure value amount is scaled
        return (uint256(price) * _balance) / SCALE_FACTOR;

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Oracle/RenzoOracle.sol#L96-L97

        // Price is times 10**18 ensure token amount is scaled
        return (_value * SCALE_FACTOR) / uint256(price);

As an example how Chainlink could possibly change decimal() (say from 18 to 8 or some other integer), let's dive into the verified contract of STETH / ETH price feed. As seen in the current code snippet, decimals() of the price feed contract is dependent on currentPhase.aggregator.decimals():

https://etherscan.io/address/0x86392dC19c0b719886221c78AB11eb8Cf5c52812#code#L431

  /**
   * @notice represents the number of decimals the aggregator responses represent.
   */
  function decimals()
    external
    view
    override
    returns (uint8)
  {
    return currentPhase.aggregator.decimals();
  }

And notably, the contract owner is allowed to propose and confirm/change the aggregator:

https://etherscan.io/address/0x86392dC19c0b719886221c78AB11eb8Cf5c52812#code#L481

  /**
   * @notice Allows the owner to propose a new address for the aggregator
   * @param _aggregator The new address for the aggregator contract
   */
  function proposeAggregator(address _aggregator)
    external
    onlyOwner()
  {
    proposedAggregator = AggregatorV2V3Interface(_aggregator);
  }

  /**
   * @notice Allows the owner to confirm and change the address
   * to the proposed aggregator
   * @dev Reverts if the given address doesn't match what was previously
   * proposed
   * @param _aggregator The new address for the aggregator contract
   */
  function confirmAggregator(address _aggregator)
    external
    onlyOwner()
  {
    require(_aggregator == address(proposedAggregator), "Invalid proposed aggregator");
    delete proposedAggregator;
    setAggregator(_aggregator);
  }

Consequently, currentPhase will be changed by invoking setAggregator() in the code block above:

https://etherscan.io/address/0x86392dC19c0b719886221c78AB11eb8Cf5c52812#code#L495

  function setAggregator(address _aggregator)
    internal
  {
    uint16 id = currentPhase.id + 1;
    currentPhase = Phase(id, AggregatorV2V3Interface(_aggregator));
    phaseAggregators[id] = AggregatorV2V3Interface(_aggregator);
  }

In conclusion, while the hardcoding approach has some gas saving benefit, it does not justify a high/critical impact ahead albeit at a low likelihood.

Tools Used

Manual

Recommended Mitigation Steps

Modify the contract to retrieve and use the decimal value from the oracle dynamically within each function that requires price data. This can ensure that any changes in decimal precision are handled correctly at runtime.

For instance, lookupTokenValue() may be refactored as follows:

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Oracle/RenzoOracle.sol#L71-L81

    function lookupTokenValue(IERC20 _token, uint256 _balance) public view returns (uint256) {
        AggregatorV3Interface oracle = tokenOracleLookup[_token];
        if (address(oracle) == address(0x0)) revert OracleNotFound();

        (, int256 price, , uint256 timestamp, ) = oracle.latestRoundData();
        if (timestamp < block.timestamp - MAX_TIME_WINDOW) revert OraclePriceExpired();
        if (price <= 0) revert InvalidOraclePrice();

-        // Price is times 10**18 ensure value amount is scaled
-        return (uint256(price) * _balance) / SCALE_FACTOR;

+        uint256 decimals = oracle.decimals();
+        return (uint256(price) * _balance) / (10**decimals);
    }

Assessed type

Decimal

[jatinj615]

External dependency creating a technical debt.

[alcueca]

If Chainlink or any other used oracle service changes the decimal precision for any price feed

The chances of that are quite minimal, and everyone would get rekt everywhere.

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-a

[c4-judge]

alcueca marked the issue as grade-b

[mystery0x]

If Chainlink or any other used oracle service changes the decimal precision for any price feed

The chances of that are quite minimal, and everyone would get rekt everywhere.

Exactly like you said, the impact will be critical albeit the chances of that are quite minimal. A combination of high/critical impact and low likelihood should be classified as a medium finding IMO. The report is value added for better code engineering despite the very fact that the contract can be upgraded to rectify it. Thank you for reconsidering.