Risk of blocking user's rewards withdrawal if `escrowPercentage > 1e18`

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L243-L288 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L178-L181 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L191-L202

Vulnerability details

Impact

In the MultiRewardStaking contract when the addRewardToken function is called by the owner he has the option to set an escrowPercentage which represent the percentage of rewards locked in the escrow contract when the claimRewards function is called, the value of escrowPercentage is supposed to be in basis point where 1e18 represents 100%.

The issue occurs if the owner by accident (or intentionnally) sets the escrowPercentage value greater than 1e18, this will cause the function _lockToken to always revert due to an underflow which will block the rewards withdrawal in the contract.

And because the escrowPercentage can not be updated after the call to addRewardToken, the rewards withdrawal process will remain blocked forever.

Proof of Concept

The issue occurs when the escrowPercentage value is set in the addRewardToken function :

File: utils/MultiRewardStaking.sol Line 243-288

function addRewardToken(
    IERC20 rewardToken,
    uint160 rewardsPerSecond,
    uint256 amount,
    bool useEscrow,
    uint192 escrowPercentage,
    uint32 escrowDuration,
    uint32 offset
  ) external onlyOwner {
    if (asset() == address(rewardToken)) revert RewardTokenCantBeStakingToken();

    RewardInfo memory rewards = rewardInfos[rewardToken];
    if (rewards.lastUpdatedTimestamp > 0) revert RewardTokenAlreadyExist(rewardToken);

    if (amount > 0) {
      if (rewardsPerSecond == 0) revert ZeroRewardsSpeed();
      rewardToken.safeTransferFrom(msg.sender, address(this), amount);
    }

    // Add the rewardToken to all existing rewardToken
    rewardTokens.push(rewardToken);

    if (useEscrow) {
      escrowInfos[rewardToken] = EscrowInfo({
        escrowPercentage: escrowPercentage,
        escrowDuration: escrowDuration,
        offset: offset
      });
      rewardToken.safeApprove(address(escrow), type(uint256).max);
    }

    uint64 ONE = (10**IERC20Metadata(address(rewardToken)).decimals()).safeCastTo64();
    uint32 rewardsEndTimestamp = rewardsPerSecond == 0
      ? block.timestamp.safeCastTo32()
      : _calcRewardsEnd(0, rewardsPerSecond, amount);

    rewardInfos[rewardToken] = RewardInfo({
      ONE: ONE,
      rewardsPerSecond: rewardsPerSecond,
      rewardsEndTimestamp: rewardsEndTimestamp,
      index: ONE,
      lastUpdatedTimestamp: block.timestamp.safeCastTo32()
    });

    emit RewardInfoUpdate(rewardToken, rewardsPerSecond, rewardsEndTimestamp);
  }

As you can see if useEscrow is set to true the escrowPercentage value is set directly without any check on its value which then allows the owner to set it greater than 1e18.

When later a user tries to claim its rewards for that given token he will call the claimRewards function which contain the following line of code :

File: utils/MultiRewardStaking.sol Line 178-181

if (escrowInfo.escrowPercentage > 0) {
    _lockToken(user, _rewardTokens[i], rewardAmount, escrowInfo);
    emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, true);
} 

So if escrowInfo.escrowPercentage is greater than zero the internal function _lockToken is called which in turn contains the following code :

File: utils/MultiRewardStaking.sol Line 191-202

function _lockToken(
    address user,
    IERC20 rewardToken,
    uint256 rewardAmount,
    EscrowInfo memory escrowInfo
  ) internal {
    uint256 escrowed = rewardAmount.mulDiv(uint256(escrowInfo.escrowPercentage), 1e18, Math.Rounding.Down);
    /** @audit
        This operation will underflow if escrowPercentage > 1e18
    */
    uint256 payout = rewardAmount - escrowed;

    rewardToken.safeTransfer(user, payout);
    escrow.lock(rewardToken, user, escrowed, escrowInfo.escrowDuration, escrowInfo.offset);
  }

The operation which calculate the final payout amount payout will underflow because when we have escrowInfo.escrowPercentage > 1e18 the value of escrowed will be greater than the rewardAmount.

And thus the function will revert and the user won't be able to claim his rewards.

Finally, because the MultiRewardStaking contract does not contain any function to update the value of escrowPercentage for a given token, the users will not be able to claim their rewards forever as the call to claimRewards function will always revert.

Tools Used

Manual review

Recommended Mitigation Steps

To avoid this issue add a check in the addRewardToken function to ensure that the value of escrowPercentage is always less or equal to 1e18 :

function addRewardToken(
    IERC20 rewardToken,
    uint160 rewardsPerSecond,
    uint256 amount,
    bool useEscrow,
    uint192 escrowPercentage,
    uint32 escrowDuration,
    uint32 offset
  ) external onlyOwner {
    if (asset() == address(rewardToken)) revert RewardTokenCantBeStakingToken();

    RewardInfo memory rewards = rewardInfos[rewardToken];
    if (rewards.lastUpdatedTimestamp > 0) revert RewardTokenAlreadyExist(rewardToken);

    if (amount > 0) {
      if (rewardsPerSecond == 0) revert ZeroRewardsSpeed();
      rewardToken.safeTransferFrom(msg.sender, address(this), amount);
    }

    // Add the rewardToken to all existing rewardToken
    rewardTokens.push(rewardToken);

    if (useEscrow) {
      /** @audit
         Add a check on the escrowPercentage value
      */
      if (escrowPercentage > 1e18) revert InvalidPercentage(escrowPercentage);

      escrowInfos[rewardToken] = EscrowInfo({
        escrowPercentage: escrowPercentage,
        escrowDuration: escrowDuration,
        offset: offset
      });
      rewardToken.safeApprove(address(escrow), type(uint256).max);
    }

    uint64 ONE = (10**IERC20Metadata(address(rewardToken)).decimals()).safeCastTo64();
    uint32 rewardsEndTimestamp = rewardsPerSecond == 0
      ? block.timestamp.safeCastTo32()
      : _calcRewardsEnd(0, rewardsPerSecond, amount);

    rewardInfos[rewardToken] = RewardInfo({
      ONE: ONE,
      rewardsPerSecond: rewardsPerSecond,
      rewardsEndTimestamp: rewardsEndTimestamp,
      index: ONE,
      lastUpdatedTimestamp: block.timestamp.safeCastTo32()
    });

    emit RewardInfoUpdate(rewardToken, rewardsPerSecond, rewardsEndTimestamp);
  }

[c4-judge]

dmvt marked the issue as duplicate of #251

[c4-sponsor]

RedVeil marked the issue as sponsor confirmed

[c4-judge]

dmvt changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by captainmangoC4

[c4-judge]

dmvt marked the issue as partial-50