Incorrect use of `maxLengthRewards` in `_claimCvgCvxRewards()` leads to wrong reward calculations

[hats-bug-reporter[bot]]

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0x08dbaa377d1adbabda87f288fefcdf65d2d1d26bc20b652796fde018183c7346 Severity: high

Description: Description\ StakingServiceBase.sol is the base of staking contracts of Convex integration and its used by both CvgCvxStakingPositionService.sol and CvxAssetStakingService.sol contracts. The contract tracks staking shares per CvgCycle also for a cycle in the past.

StakingServiceBase.sol has variable named numberOfUnderlyingRewards which is the maximum amount of rewards claimable through CVX. numberOfUnderlyingRewards is incremented during the processCvxRewards each time a new reward ERC20 is distributed.

claimCvgCvxRewards and claimCvgCvxMultiple are used by users to claim CVG and CVX rewards for a Staking Position on one OR several already passed AND not claimed cycles.

Both of these functions has used internal function _claimCvgCvxRewards() which is implemented as:

    function _claimCvgCvxRewards(
        uint256 tokenId
    ) internal returns (uint256, ICommonStruct.TokenAmount[] memory tokenAmounts) {
        uint128 nextClaimableCvg = _nextClaims[tokenId].nextClaimableCvg;
        uint128 nextClaimableCvx = _nextClaims[tokenId].nextClaimableCvx;
        uint128 actualCycle = stakingCycle;
        uint256 lengthHistory = _stakingHistoryByToken[tokenId].length;

        /// @dev If never claimed on this token
        if (nextClaimableCvx == 0) {
            /// @dev Set the lastClaimed as the first action.
            nextClaimableCvx = uint128(_stakingHistoryByToken[tokenId][0]);
        }
        require(actualCycle > nextClaimableCvx, "ALL_CVX_CLAIMED_FOR_NOW");

        /// @dev Total amount of CVG, accumulated through all cycles and minted at the end of the function
        uint256 _cvgClaimable;

@>      uint256 maxLengthRewards = nextClaimableCvx;
        /// @dev Array of all rewards from StakeDao, all cycles are accumulated in this array and transfer at the end of the function
        ICommonStruct.TokenAmount[] memory _totalRewardsClaimable = new ICommonStruct.TokenAmount[](maxLengthRewards);

        uint256 newLastClaimCvx;
        bool isCvxRewards;
        for (; nextClaimableCvx < actualCycle; ) {

            . . .  some code

                if (_cycleInfo[nextClaimableCvx].isCvxProcessed) {
@>                  for (uint256 erc20Id; erc20Id < maxLengthRewards; ) {

            . . .  some code

      } 
      }

This function has used local variable maxLengthRewards which is iterated to calculate total claimable rewards. However, this local variable has used incorrect value.

      uint256 maxLengthRewards = nextClaimableCvx;

It has used nextClaimableCvx which is used to set the last claimed. maxLengthRewards should have used numberOfUnderlyingRewards value which is correct to calculate the total claimable rewards.

Impact

Incorrect calculation of total claimable rewards due to wrong value of maxLengthRewards leads to loss of user rewards. This breaks the intended design due to wrong logic.

Recommendation to fix\

Use numberOfUnderlyingRewards instead of nextClaimableCvx as value of maxLengthRewardsin _claimCvgCvxRewards()

Consider below changes:

    function _claimCvgCvxRewards(
        uint256 tokenId
    ) internal returns (uint256, ICommonStruct.TokenAmount[] memory tokenAmounts) {
        uint128 nextClaimableCvg = _nextClaims[tokenId].nextClaimableCvg;
        uint128 nextClaimableCvx = _nextClaims[tokenId].nextClaimableCvx;
        uint128 actualCycle = stakingCycle;
        uint256 lengthHistory = _stakingHistoryByToken[tokenId].length;

        /// @dev If never claimed on this token
        if (nextClaimableCvx == 0) {
            /// @dev Set the lastClaimed as the first action.
            nextClaimableCvx = uint128(_stakingHistoryByToken[tokenId][0]);
        }
        require(actualCycle > nextClaimableCvx, "ALL_CVX_CLAIMED_FOR_NOW");

        /// @dev Total amount of CVG, accumulated through all cycles and minted at the end of the function
        uint256 _cvgClaimable;

-        uint256 maxLengthRewards = nextClaimableCvx; 
+        uint256 maxLengthRewards = numberOfUnderlyingRewards; 
        /// @dev Array of all rewards from StakeDao, all cycles are accumulated in this array and transfer at the end of the function
        ICommonStruct.TokenAmount[] memory _totalRewardsClaimable = new ICommonStruct.TokenAmount[](maxLengthRewards);

        uint256 newLastClaimCvx;
        bool isCvxRewards;
        for (; nextClaimableCvx < actualCycle; ) {

            . . .  some code

      }

[0xdeth]

Duplicate of #37.