c4-bot-7 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/staking/LockingMultiRewards.sol#L458-L477

Vulnerability details

Impact

In LockingMultiRewards contract: whenever a user stakes, withdraws or claims his rewards; his rewards are updated via _updateRewardsForUser() function, where it loops over reward tokens and updates the rewards[user_][token_] via _udpateUserRewards() function:

  function _updateRewardsForUser(address user) internal {
      uint256 balance = balanceOf(user);
      uint256 totalSupply_ = totalSupply();

      for (uint256 i; i < rewardTokens.length; ) {
          address token = rewardTokens[i];
          _udpateUserRewards(user, balance, token, _updateRewardsGlobal(token, totalSupply_));

          unchecked {
              ++i;
          }
      }
  }

  function _udpateUserRewards(address user_, uint256 balance_, address token_, uint256 rewardPerToken_) internal {
      rewards[user_][token_] = _earned(user_, balance_, token_, rewardPerToken_);
      userRewardPerTokenPaid[user_][token_] = rewardPerToken_;
  }

  function _earned(address user, uint256 balance_, address rewardToken, uint256 rewardPerToken_) internal view returns (uint256) {
      uint256 pendingUserRewardsPerToken = rewardPerToken_ - userRewardPerTokenPaid[user][rewardToken];

      return ((balance_ * pendingUserRewardsPerToken) / 1e18) + rewards[user][rewardToken];
  }

As can be noticed, user's rewards[user_][token_] is updated based on rewardPerToken_ - userRewardPerTokenPaid * user's balance, so that user's rewards balance can be updated only once per block as long as there's no update on rewardPerToken_ (via notifyRewardAmount()).

This can be exploited by any malicious user to grief other users by staking dust unlocked amount (1 wei) on their behalf via stakeFor() function:

function _stakeFor(address account, uint256 amount, bool lock_) internal {
      if (amount == 0) {
          revert ErrZeroAmount();
      }

      // This staking contract isn't using balanceOf, so it's safe to transfer immediately
      stakingToken.safeTransferFrom(msg.sender, address(this), amount);
      stakingTokenBalance += amount;

      _updateRewardsForUser(account);

      if (lock_) {
          _createLock(account, amount);
      } else {
          _balances[account].unlocked += amount;
          unlockedSupply += amount;

          emit LogStaked(account, amount);
      }
  }

and when the griefed user rewards are updated via _updateRewardsForUser(), the userRewardPerTokenPaid[user] will be set to the latest rewardPerToken_ , so when the griefed user calls any of the stake,withdraw or getRewards functions; the updated rewards will be less than he's entitled to because pendingUserRewardsPerToken = rewardPerToken_ - userRewardPerTokenPaid[user] = 0 (as long as the rewardPerToken hasn't been updated yet).

Proof of Concept

LockingMultiRewards._stakeFor function

function _stakeFor(address account, uint256 amount, bool lock_) internal {
        if (amount == 0) {
            revert ErrZeroAmount();
        }

        // This staking contract isn't using balanceOf, so it's safe to transfer immediately
        stakingToken.safeTransferFrom(msg.sender, address(this), amount);
        stakingTokenBalance += amount;

        _updateRewardsForUser(account);

        if (lock_) {
            _createLock(account, amount);
        } else {
            _balances[account].unlocked += amount;
            unlockedSupply += amount;

            emit LogStaked(account, amount);
        }
    }

Tools Used

Manual Review.

Recommended Mitigation Steps

Add a minimum staking amount for unlocked stakes similar to the minLockAmount used to check when creating locked stakes:

function _stakeFor(address account, uint256 amount, bool lock_) internal {
        if (amount == 0) {
            revert ErrZeroAmount();
        }

        // This staking contract isn't using balanceOf, so it's safe to transfer immediately
        stakingToken.safeTransferFrom(msg.sender, address(this), amount);
        stakingTokenBalance += amount;

        _updateRewardsForUser(account);

        if (lock_) {
            _createLock(account, amount);
        } else {
+           require(amount >= minUnlockedStakedAmount,"invalid staking amount");
            _balances[account].unlocked += amount;
            unlockedSupply += amount;

            emit LogStaked(account, amount);
        }
    }