Reward calculation might be wrong

Summary

Game.addToTotalRewards updates rewards, but the lastRebalancingPeriod are updated larger value than it should be after it is used in the reward calculation. So the reward calculation can be wrong.

Vulnerability Detail

Game.rebalanceBasket calls addToTotalRewards to update rewards and then setBasketRebalancingPeriod to update basket.lastRebalancingPeriod.

  function rebalanceBasket(
    uint256 _basketId,
    int256[][] memory _deltaAllocations
  ) external onlyBasketOwner(_basketId) nonReentrant {
    uint256 vaultNumber = baskets[_basketId].vaultNumber;
    for (uint k = 0; k < chainIds.length; k++) {
      require(!isXChainRebalancing[vaultNumber][chainIds[k]], "Game: vault is xChainRebalancing");
    }

    addToTotalRewards(_basketId);
    int256 totalDelta = settleDeltaAllocations(_basketId, vaultNumber, _deltaAllocations);

    lockOrUnlockTokens(_basketId, totalDelta);
    setBasketTotalAllocatedTokens(_basketId, totalDelta);
    setBasketRebalancingPeriod(_basketId, vaultNumber);
  }

In addToTotalRewards, basket.totalUnRedeemedRewards is updated by the difference between rewards rates of currentRebalancingPeriod and lastRebalancingPeriod.

function addToTotalRewards(uint256 _basketId) internal onlyBasketOwner(_basketId) {

    ...

    uint256 currentRebalancingPeriod = vaults[vaultNum].rebalancingPeriod;
    uint256 lastRebalancingPeriod = baskets[_basketId].lastRebalancingPeriod;

    ...

        int256 lastRebalanceReward = getRewardsPerLockedToken(
          vaultNum,
          chain,
          lastRebalancingPeriod,
          i
        );
        int256 currentReward = getRewardsPerLockedToken(
          vaultNum,
          chain,
          currentRebalancingPeriod,
          i
        );
        baskets[_basketId].totalUnRedeemedRewards +=
          (currentReward - lastRebalanceReward) *
          allocation;

In setBasketRebalancingPeriod, lastRebalancingPeriod is updated to rebalancingPeriod + 1.

baskets[_basketId].lastRebalancingPeriod = vaults[_vaultNumber].rebalancingPeriod + 1;

So we lose the reward difference between rebalancingPeriod and rebalancingPeriod + 1. As a result, reward calculation will be incorrect.

Impact

Reward calculation will be wrong.

Code Snippet

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

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

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

Tool used

Manual Review

Recommendation

Set lastRebalancingPeriod by rebalancingPeriod without adding 1.

    baskets[_basketId].lastRebalancingPeriod = vaults[_vaultNumber].rebalancingPeriod;

Duplicate of #119

sherlock-audit / 2023-01-derby-judging

XKET - Reward calculation might be wrong #383