claim all rewards function fails to accrue rewards correctly.

[hats-bug-reporter]

Github username: @ArnieGod
Submission hash (on-chain): 0x5719a34f5400575f90b590069b529599686f752ffacaa11896483ee11599d99b
Severity: medium severity

Description:

Vulnerability Report

Description

In IncentivesController.sol

  function claimReward(
    address[] calldata incentivizedAssets,
    address reward,
    uint256 amountToClaim,
    address to
  ) external override returns (uint256) {
    if (amountToClaim == 0) {
      return 0;
    }

    address user = msg.sender;
    DistributionTypes.UserAssetState[] memory userState = _getUserState(incentivizedAssets, user);
    _batchUpdate(user, userState);

the function above calls into

_batchUpdate(user, userState);

this function call will accrue rewards and update the reward timestamp. However when we call claimAllRewards

  function claimAllRewards(
    address[] calldata incentivizedAssets,
    address to
  ) external override returns (address[] memory, uint256[] memory) {
    address[] memory rewards = _allRewards;
    uint256[] memory amounts = new uint256[](_allRewards.length);
    address user = msg.sender;

the _batchUpdate(user, userState) function is not called, therefore the accrued rewards and the rewards timestamp is never updated.
additionally,

 function _updateReward(
    DistributionTypes.Reward storage reward,
    uint256 totalSupply,
    uint8 decimals
  ) internal returns (uint256, bool) {
    bool updated;
    uint256 newIndex = _getAssetIndex(reward, totalSupply, decimals);

    if (newIndex != reward.index) {
      reward.index = newIndex;
      updated = true;
    }
    reward.lastUpdateTimestamp = uint128(block.timestamp);

    return (newIndex, updated);
  }

the reward.lastUpdateTimestamp is not updated correctly as well, the impact is the lastUpdateTimestamp can be stale and make the check below ineffective

 function _getAssetIndex(
    DistributionTypes.Reward storage reward,
    uint256 totalSupply,
    uint8 decimals
  ) internal view returns (uint256) {
    if (
      reward.emissionPerSecond == 0 ||
      totalSupply == 0 ||
      reward.lastUpdateTimestamp == block.timestamp ||
      reward.lastUpdateTimestamp >= reward.endTimestamp
    ) {
      return reward.index;
    }

impact

claimAllRewards does not take the pending accured reward into consideration and the lastUpdateTimestamp can be stale.

code snippet

VMEX-0x050183b53cf62bcd6c2a932632f8156953fd146f/packages/contracts/contracts/protocol/incentives/IncentivesController.sol

Lines 203 to 220 in fb396a3

	function claimAllRewards(
	address[] calldata incentivizedAssets,
	address to
	) external override returns (address[] memory, uint256[] memory) {
	address[] memory rewards = _allRewards;
	uint256[] memory amounts = new uint256[](_allRewards.length);
	address user = msg.sender;
	for (uint256 i = 0; i < incentivizedAssets.length; i++) {
	address asset = incentivizedAssets[i];
	for (uint256 j = 0; j < _allRewards.length; j++) {
	uint256 amount = _incentivizedAssets[asset].rewardData[rewards[j]].users[user].accrued;
	if (amount != 0) {
	amounts[j] += amount;
	_incentivizedAssets[asset].rewardData[rewards[j]].users[user].accrued = 0;
	}
	}
	}

recommendation

I recommend adding _batchUpdate(user, userState) to the claimAllRewards function.

[skyao2002]

Thanks for the submission. This is a valid mistake on our end, and the team was aware of this issue and the fix is in a separate branch pending to merge to master well before the audit started. However, the impact of the issue is not as severe as you claim, and our team did not roll out the fix because it was deemed to be low severity and can be rolled out in the patch.

The users will not be able to claim all their funds by calling this function, but they can still retrieve all their funds by calling the claimReward() function. There is no loss of funds. lastUpdateTimestamp could be stale, but there are no vulnerabilities exposed from the stale value. Please submit a proof of concept if you would like to disagree with this analysis.

The bug does not meet any of the requirements for a medium severity issue:

• Gas griefing attacks (make users overpay for gas)
• Attacks that make essential functionality of the contracts temporarily unusable or inaccessible
• Short-term freezing of user funds

This bug will be moved to a low severity issue.