CvxRewardDistributor.sol#claimMultipleStaking() - tokens already in sdt claimable or not in cvx claimable would lead to a wrong claim

[hats-bug-reporter[bot]]

Github username: @PlamenTSV Twitter username: @p_tsanev Submission hash (on-chain): 0x733ba17e245174f744b0d2b9d538414c3ed3d0c2f34b7e5b21f7ab378b50fbed Severity: medium

Description: Description\ The claimMultipleStaking() function, per the comments, intends to claim the accumulated rewards on Convex from different services in 1 transaction. We do so by firstly iterating over all staking services and iterating over all claimed CVX rewards of the positions. Here we iterate over a merged array of CVX rewards in the SDT claimable and have a number of sanity checks performed wrongly, which would lead to wrongly calculating _totalCvxClaimable.

Attack Scenario\ The scenario has 2 sides:

1. The token is not in the totalCvxClaimable. Then we initialize the token and assume that we move to the next iteration
2. The token is already inside, in which case we increment _totalCvxClaimable by the amount. In both cases we invoke the break keyword with the developer assumption being, per the comment: /// @dev Pass to the next token However invoking break will stop the loop entirely, instead of moving on to the next token. Even if the next token has a claimable reward for the current position, it would not be claimed and would remain stuck, since an attempt to reclaim it would fail. The inner call to cvxStaking.claimCvgCvxMultiple would have already claimed the position.

Attachments

1. Proof of Concept (PoC) File

Recommendation Instead of invoking break, in order to 'skip' the current iteration, you need to invoke the continue keyword and increment the the loop iterator before the continue call:

if (iteratedTotalClaimableToken == address(0)) {
    /// @dev Set token data in the totalClaimable array.
    _totalCvxClaimable[totalRewardIndex] = ICommonStruct.TokenAmount({
        token: _cvxRewards[positionRewardIndex].token,
        amount: _cvxRewards[positionRewardIndex].amount
    });

    /// @dev Pass to the next token
-    break; //@audit no incrementing iterator and wrong usage of break
+    unchecked {
+     ++totalRewardIndex;
+  }
+   continue;
}

/// @dev If the token is already in the totalSdtClaimable.
if (iteratedTotalClaimableToken == address(_cvxRewards[positionRewardIndex].token)) {
    /// @dev Increments the claimable amount.
    _totalCvxClaimable[totalRewardIndex].amount += _cvxRewards[positionRewardIndex].amount;
    /// @dev Pass to the next token
-    break;
+    unchecked {
+     ++totalRewardIndex;
+  }
+   continue;
}

[walk-on-me]

Break keywords are working as expected. It's a gas optimisation allowing to don't iterate over all the array of merged rewards.

SDT typos are residual from our StakeDao integration.

The mecanism is :

• We iterate through all the rewards coming from the service.

• In this same loop, we have an other loop searching in the global reward array the position of the same token.

• We are taking the iteratedTotalClaimableToken ( from the final array ) :

  • If it's equal to 0x000, it means the final array is either empty or that we arrived at the end of the array, and so in both case we fill the ""blank"" slot with the rewards from _sdtRewards.
  • If it's equal to the actual in _sdtRewards ( first loop ), we can add what was previously added in the final Array.
  • In these both case, we can break the imbricated forLoop because we already filled the final array
  • Else none of both case is respected, we continue to iterate over the fullArray until we either find the matching reward with the _sdtRewards or if we arrive at the end of the array => we so add the the token in the full reward array."

We so considers the issue as Invalid