hyh - Vault can lose rewards due to lack of precision

[sherlock-admin]

hyh

medium

Vault can lose rewards due to lack of precision

Summary

Vault's storePriceAndRewards() can accrue no rewards as precision can be lost when price() has low decimals.

For example, price() has 6 decimals for Idle USDC and USDT strategies.

Vulnerability Detail

Suppose storePriceAndRewards() is called for Idle USDC protocol, this USDC Vault is relatively new and Totalunderlying = TotalUnderlyingInProtocols - BalanceVault = USD 30k, performance fee is 5% and it's 1 mln Derby tokens staked, i.e. totalAllocatedTokens = 1_000_000 * 1e18, while lastPrices[_protocolId] = 1_100_000 (which is 1.1 USDC, Idle has price scaled by underlying decimals and tracks accumulated strategy share price).

Let's say this provider shows stable returns with 1.7% APY, suppose market rates are low and this is somewhat above market. In bi-weekly terms it can correspond to priceDiff = 730, as, roughly excluding price appreciation that doesn't influence much here, (730.0 / 1100000 + 1)**26 - 1 = 1.7%.

In this case rewardPerLockedToken will be 30000 * 10**6 * 5 * 730 / (1_000_000 * 1_100_000 * 100) = 0 for all rebalancing periods, i.e. no rewards at all will be allocated for the protocol despite it having positive performance.

Impact

Rewards can be lost for traders who allocated to protocols where price() has low decimals.

Code Snippet

Vault's storePriceAndRewards() calculates strategy performance reward as nominator / denominator, where nominator = _totalUnderlying * performanceFee * priceDiff:

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

  /// @notice Stores the historical price and the reward per rounded locked token, ignoring decimals.
  /// @dev formula yield protocol i at time t: y(it) = (P(it) - P(it-1)) / P(it-1).
  /// @dev formula rewardPerLockedToken for protocol i at time t: r(it) = y(it) * TVL(t) * perfFee(t) / totalLockedTokens(t)
  /// @dev later, when the total rewards are calculated for a game player we multiply this (r(it)) by the locked tokens on protocol i at time t
  /// @param _totalUnderlying Totalunderlying = TotalUnderlyingInProtocols - BalanceVault.
  /// @param _protocolId Protocol id number.
  function storePriceAndRewards(uint256 _totalUnderlying, uint256 _protocolId) internal {
    uint256 currentPrice = price(_protocolId);
    if (lastPrices[_protocolId] == 0) {
      lastPrices[_protocolId] = currentPrice;
      return;
    }

    int256 priceDiff = int256(currentPrice - lastPrices[_protocolId]);
    int256 nominator = (int256(_totalUnderlying * performanceFee) * priceDiff);
    int256 totalAllocatedTokensRounded = totalAllocatedTokens / 1E18;
    int256 denominator = totalAllocatedTokensRounded * int256(lastPrices[_protocolId]) * 100; // * 100 cause perfFee is in percentages

    if (totalAllocatedTokensRounded == 0) {
      rewardPerLockedToken[rebalancingPeriod][_protocolId] = 0;
    } else {
      rewardPerLockedToken[rebalancingPeriod][_protocolId] = nominator / denominator;
    }

    lastPrices[_protocolId] = currentPrice;
  }

price() is Provider's exchangeRate():

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

  /// @notice Get price for underlying protocol
  /// @param _protocolNum Protocol number linked to an underlying protocol e.g compound_usdc_01
  /// @return protocolPrice Price per lp token
  function price(uint256 _protocolNum) public view returns (uint256) {
    IController.ProtocolInfoS memory protocol = controller.getProtocolInfo(
      vaultNumber,
      _protocolNum
    );
    return IProvider(protocol.provider).exchangeRate(protocol.LPToken);
  }

IdleProvider's 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) {}

Tool used

Manual Review

Recommendation

Consider enhancing performance of the rewards calculation and Game's baskets[_basketId].totalUnRedeemedRewards, for example:

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

  uint256 public uScale;

  uint256 public minimumPull;

+ uint256 public BASE_SCALE = 1e18;

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

  /// @notice Stores the historical price and the reward per rounded locked token, ignoring decimals.
  /// @dev formula yield protocol i at time t: y(it) = (P(it) - P(it-1)) / P(it-1).
  /// @dev formula rewardPerLockedToken for protocol i at time t: r(it) = y(it) * TVL(t) * perfFee(t) / totalLockedTokens(t)
  /// @dev later, when the total rewards are calculated for a game player we multiply this (r(it)) by the locked tokens on protocol i at time t
  /// @param _totalUnderlying Totalunderlying = TotalUnderlyingInProtocols - BalanceVault.
  /// @param _protocolId Protocol id number.
  function storePriceAndRewards(uint256 _totalUnderlying, uint256 _protocolId) internal {
    uint256 currentPrice = price(_protocolId);
    if (lastPrices[_protocolId] == 0) {
      lastPrices[_protocolId] = currentPrice;
      return;
    }

    int256 priceDiff = int256(currentPrice - lastPrices[_protocolId]);
-   int256 nominator = (int256(_totalUnderlying * performanceFee) * priceDiff);
+   int256 nominator = (int256(_totalUnderlying * performanceFee) * priceDiff * BASE_SCALE);
    int256 totalAllocatedTokensRounded = totalAllocatedTokens / 1E18;
    int256 denominator = totalAllocatedTokensRounded * int256(lastPrices[_protocolId]) * 100; // * 100 cause perfFee is in percentages

    if (totalAllocatedTokensRounded == 0) {
      rewardPerLockedToken[rebalancingPeriod][_protocolId] = 0;
    } else {
      rewardPerLockedToken[rebalancingPeriod][_protocolId] = nominator / denominator;
    }

    lastPrices[_protocolId] = currentPrice;
  }

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Game.sol#L69-L70

  // percentage of tokens that will be sold at negative rewards
  uint256 internal negativeRewardFactor;

+ uint256 public BASE_SCALE = 1e18;

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Game.sol#L296-L306

  function redeemNegativeRewards(
    uint256 _basketId,
    uint256 _unlockedTokens
  ) internal returns (uint256) {
    int256 unredeemedRewards = baskets[_basketId].totalUnRedeemedRewards;
    if (unredeemedRewards > negativeRewardThreshold) return 0;

    uint256 tokensToBurn = (uint(-unredeemedRewards) * negativeRewardFactor) / 100;
    tokensToBurn = tokensToBurn < _unlockedTokens ? tokensToBurn : _unlockedTokens;

-   baskets[_basketId].totalUnRedeemedRewards += int((tokensToBurn * 100) / negativeRewardFactor);
+   baskets[_basketId].totalUnRedeemedRewards += int((tokensToBurn * 100 * BASE_SCALE) / negativeRewardFactor);

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Game.sol#L542-L553

  /// @notice redeem funds from basket in the game.
  /// @dev makes a call to the vault to make the actual transfer because the vault holds the funds.
  /// @param _basketId Basket ID (tokenID) in the BasketToken (NFT) contract.
  function redeemRewards(uint256 _basketId) external onlyBasketOwner(_basketId) {
-   int256 amount = baskets[_basketId].totalUnRedeemedRewards;
+   int256 amount = baskets[_basketId].totalUnRedeemedRewards / BASE_SCALE;
    require(amount > 0, "Nothing to claim");

    baskets[_basketId].totalRedeemedRewards += amount;
    baskets[_basketId].totalUnRedeemedRewards = 0;

    IVault(homeVault).redeemRewardsGame(uint256(amount), msg.sender);
  }

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Game.sol#L19-L32

  struct Basket {
    // the vault number for which this Basket was created
    uint256 vaultNumber;
    // last period when this Basket got rebalanced
    uint256 lastRebalancingPeriod;
    // nr of total allocated tokens
    int256 nrOfAllocatedTokens;
    // total build up rewards
-   int256 totalUnRedeemedRewards;
+   int256 totalUnRedeemedRewards;  // in {underlying + 18} decimals precision
    // total redeemed rewards
    int256 totalRedeemedRewards;
    // (basket => vaultNumber => chainId => allocation)
    mapping(uint256 => mapping(uint256 => int256)) allocations;
  }

[mister0y]

Possibly a high severity

[hrishibhat]

Given that this applies only for tokens with lower decimals considering this a valid medium

[SergeKireev]

Escalate for 10 USDC,

Given that this applies only for tokens with lower decimals considering this a valid medium

This indeed holds when underlying token has lower decimals such as USDC. But given the popularity of USDC, it is likely that it will be the most used asset as the underlying.

What makes it less likely is if price() needs to be of lower precision as stated at the beginning of the report

However this does not only hold when price() has low decimals as stated in the report, because priceDiff on nominator has the same precision as lastPrices[protocolId] on denominator, and thus the precision of the price does not change the result of the division.

This means that the example holds for all protocols, and the severity can be high

[sherlock-admin]

Escalate for 10 USDC,

Given that this applies only for tokens with lower decimals considering this a valid medium

This indeed holds when underlying token has lower decimals such as USDC. But given the popularity of USDC, it is likely that it will be the most used asset as the underlying.

What makes it less likely is if price() needs to be of lower precision as stated at the beginning of the report

However this does not only hold when price() has low decimals as stated in the report, because priceDiff on nominator has the same precision as lastPrices[protocolId] on denominator, and thus the precision of the price does not change the result of the division.

This means that the example holds for all protocols, and the severity can be high

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

Based on both the Sponsor comment and escalation, after further consideration, the impact of this issue is a valid high.

[sherlock-admin]

Escalation accepted

Based on both the Sponsor comment and escalation, after further consideration, this issue can be flagged as valid high.

This issue's escalations have been accepted!

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

[sherlock-admin]

Escalation accepted

Based on both the Sponsor comment and escalation, after further consideration, the impact of this issue is a valid high.

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/220

[dmitriia]

Fix: derbyfinance/derby-yield-optimiser#220

Fix looks ok, but it looks like an update in addToTotalRewards() is also needed:

https://github.com/derbyfinance/derby-yield-optimiser/blob/20a60d79ccb2c0532f1b68e6119c8d1983cfcf39/contracts/Game.sol#L429-L430

-      int256 allocation = basketAllocationInProtocol(_basketId, chain, i) / 1E18;
-      if (allocation == 0) continue;
+      int256 allocation = basketAllocationInProtocol(_basketId, chain, i);
+      if (allocation / BASE_SCALE == 0) continue;

[dmitriia]

Also, due to further changes the fix-review branch needs a couple of additional updates:

https://github.com/derbyfinance/derby-yield-optimiser/blob/20a60d79ccb2c0532f1b68e6119c8d1983cfcf39/contracts/Game.sol#L70-L71

- // threshold in vaultCurrency e.g USDC for when user tokens will be sold / burned. Must be negative
+ // threshold in vaultCurrency.decimals() * BASE_SCALE of 1e18 e.g 10 ^ (6 + 18) for USDC for when user tokens will be sold / burned. Must be negative
  int256 internal negativeRewardThreshold;

https://github.com/derbyfinance/derby-yield-optimiser/blob/20a60d79ccb2c0532f1b68e6119c8d1983cfcf39/contracts/Game.sol#L693-L697

  /// @notice Setter for threshold at which user tokens will be sold / burned
- /// @param _threshold treshold in vaultCurrency e.g USDC, must be negative
+ /// @param _threshold treshold in vaultCurrency.decimals() * BASE_SCALE of 1e18 e.g 10 ^ (6 + 18) for USDC, must be negative
  function setNegativeRewardThreshold(int256 _threshold) external onlyDao {
    negativeRewardThreshold = _threshold;
  }