hyh - IdleProvider's balanceUnderlying and calcShares outputs are misscaled by up to 10^12

[sherlock-admin]

hyh

high

IdleProvider's balanceUnderlying and calcShares outputs are misscaled by up to 10^12

Summary

balanceUnderlying() returns values scaled with Idle LP token decimals instead of the underlying decimals.

calcShares() returns values scaled with underlying decimals instead of the LP token decimals.

Vulnerability Detail

Core reason is exchangeRate() output is similarly and incorrectly processed in both functions. This results in wrong decimals of the both return values.

Idle LP tokens have 18 decimals, while underlying token decimals can differ: for example, while Idle USDC tokens have 18 decimals, USDC have 6.

Impact

Amount of the underlying held by Idle pool and amount of shares needed for a withdrawal can be misstated by magnitudes wheneven underlying token decimals aren't 18, which can lead to protocol-wide losses.

Specifically for USDC and USDT cases balanceUnderlying() is overstated by 10^12, while calcShares() is understated by 10^12.

Code Snippet

balanceUnderlying() cancels out exchangeRate() decimals with dividing by 10^(Underlying Token Decimals), which results in Idle LP token decimals:

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Providers/IdleProvider.sol#L79-L92

  /// @notice Get balance from address in underlying token
  /// @dev balance = poolvalue * shares / totalsupply
  /// @param _address Address to request balance from, most likely an Vault
  /// @param _iToken Address of protocol LP Token eg cUSDC
  /// @return balance in underlying token
  function balanceUnderlying(
    address _address,
    address _iToken
  ) public view override returns (uint256) {
    uint256 balanceShares = balance(_address, _iToken);
    uint256 price = exchangeRate(_iToken);
    uint256 decimals = IERC20Metadata(IIdle(_iToken).token()).decimals();
    return (balanceShares * price) / 10 ** decimals;
  }

As balance() returns Idle token balance and have Idle LP token decimals of 18:

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Providers/IdleProvider.sol#L109-L111

  function balance(address _address, address _iToken) public view override returns (uint256) {
    return IIdle(_iToken).balanceOf(_address);
  }

While exchangeRate() is scaled with underlying token decimals:

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Providers/IdleProvider.sol#L113-L118

  /// @notice Exchange rate of underyling protocol token
  /// @param _iToken Address of protocol LP Token eg yUSDC
  /// @return price of LP token
  function exchangeRate(address _iToken) public view override returns (uint256) {
    return IIdle(_iToken).tokenPrice();
  }

https://github.com/Idle-Labs/idle-contracts/blob/develop/contracts/IdleTokenV3_1.sol#L240-L245

  /**
   * IdleToken price calculation, in underlying
   *
   * @return : price in underlying token
   */
  function tokenPrice() external view returns (uint256) {}

Idle LP token decimals are fixed, while underlying token decimals vary.

For example, while USDC underlying have 6 decimals, it's 18 decimals for Idle USDC:

https://etherscan.io/token/0x5274891bec421b39d23760c04a6755ecb444797c

https://etherscan.io/token/0xcddb1bceb7a1979c6caa0229820707429dd3ec6c

balanceUnderlying() is used in Vault rebalancing:

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Vault.sol#L178-L192

  function rebalanceCheckProtocols(
    uint256 _newTotalUnderlying
  ) internal returns (uint256[] memory) {
    uint256[] memory protocolToDeposit = new uint[](controller.latestProtocolId(vaultNumber));
    uint256 latestID = controller.latestProtocolId(vaultNumber);
    for (uint i = 0; i < latestID; i++) {
      bool isBlacklisted = controller.getProtocolBlacklist(vaultNumber, i);

      storePriceAndRewards(_newTotalUnderlying, i);

      if (isBlacklisted) continue;
      setAllocation(i);

      int256 amountToProtocol = calcAmountToProtocol(_newTotalUnderlying, i);
      uint256 currentBalance = balanceUnderlying(i);

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Vault.sol#L111-L118

  function pullFunds(uint256 _value) internal {
    uint256 latestID = controller.latestProtocolId(vaultNumber);
    for (uint i = 0; i < latestID; i++) {
      if (currentAllocations[i] == 0) continue;

      uint256 shortage = _value - vaultCurrency.balanceOf(address(this));
      uint256 balanceProtocol = balanceUnderlying(i);

Similarly in calcShares() the shares scale is corrected wrongly, resulting with it having underlying decimals instead of LP token decimals (exchangeRate(_cToken) has decimals scale cancelled by 10 ** decimals, _amount is in underlying):

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Providers/IdleProvider.sol#L94-L103

  /// @notice Calculates how many shares are equal to the amount
  /// @dev shares = totalsupply * balance / poolvalue
  /// @param _amount Amount in underyling token e.g USDC
  /// @param _iToken Address of protocol LP Token eg cUSDC
  /// @return number of shares i.e LP tokens
  function calcShares(uint256 _amount, address _iToken) external view override returns (uint256) {
    uint256 decimals = IERC20Metadata(IIdle(_iToken).token()).decimals();
    uint256 shares = (_amount * (10 ** decimals)) / exchangeRate(_iToken);
    return shares;
  }

calcShares() is used in rebalancing as well:

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Vault.sol#L307-L315

  function withdrawFromProtocol(uint256 _protocolNum, uint256 _amount) internal {
    if (_amount <= 0) return;
    IController.ProtocolInfoS memory protocol = controller.getProtocolInfo(
      vaultNumber,
      _protocolNum
    );

    _amount = (_amount * protocol.uScale) / uScale;
    uint256 shares = IProvider(protocol.provider).calcShares(_amount, protocol.LPToken);

Tool used

Manual Review

Recommendation

Idle exchange rate is specifically scaled so that LPToken_balance * price has 18 + Underlying Token Decimals, so it's enough to use 1e18:

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Providers/IdleProvider.sol#L79-L92

  /// @notice Get balance from address in underlying token
  /// @dev balance = poolvalue * shares / totalsupply
  /// @param _address Address to request balance from, most likely an Vault
  /// @param _iToken Address of protocol LP Token eg cUSDC
  /// @return balance in underlying token
  function balanceUnderlying(
    address _address,
    address _iToken
  ) public view override returns (uint256) {
    uint256 balanceShares = balance(_address, _iToken);
    uint256 price = exchangeRate(_iToken);
-   uint256 decimals = IERC20Metadata(IIdle(_iToken).token()).decimals();
-   return (balanceShares * price) / 10 ** decimals;
+   return (balanceShares * price) / 1e18;
  }

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Providers/IdleProvider.sol#L94-L103

  /// @notice Calculates how many shares are equal to the amount
  /// @dev shares = totalsupply * balance / poolvalue
  /// @param _amount Amount in underyling token e.g USDC
  /// @param _iToken Address of protocol LP Token eg cUSDC
  /// @return number of shares i.e LP tokens
  function calcShares(uint256 _amount, address _iToken) external view override returns (uint256) {
-   uint256 decimals = IERC20Metadata(IIdle(_iToken).token()).decimals();
-   uint256 shares = (_amount * (10 ** decimals)) / exchangeRate(_iToken);
-   return shares;
+   return (_amount * 1e18) / exchangeRate(_iToken);
  }

[dmitriia]

Escalate for 10 USDC 202 and this aren't duplicates.

202 deals with CompoundProvider's treatment of cToken exchange rate in balanceUnderlying() and calcShares().

This issue is about IdleProvider's balanceUnderlying() and calcShares(). The fact that resulting recommendations look similar is a mere coincidence as Idle and Compound have different mechanics (i.e. both were misunderstood, but separately and in a different way).

[sherlock-admin]

Escalate for 10 USDC 202 and this aren't duplicates.

202 deals with CompoundProvider's treatment of cToken exchange rate in balanceUnderlying() and calcShares().

This issue is about IdleProvider's balanceUnderlying() and calcShares(). The fact that resulting recommendations look similar is a mere coincidence as Idle and Compound have different mechanics (i.e. both were misunderstood, but separately and in a different way).

You've created a valid escalation for 10 USDC!

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.

[hrishibhat]

Escalation accepted

Considering this issue separate from #202, although the functions and the mitigation is similar, the underlying calculations are different and need to be addressed separately.

[sherlock-admin]

Escalation accepted

Considering this issue separate from #202, although the functions and the mitigation is similar, the underlying calculations are different and need to be addressed separately.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[Theezr]

Fix: https://github.com/derbyfinance/derby-yield-optimiser/pull/221

[dmitriia]

Fix: derbyfinance/derby-yield-optimiser#221

Fix looks ok