c4-bot-10 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/staking/LockingMultiRewards.sol#L277-L286 https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/staking/LockingMultiRewards.sol#L292-L295

Vulnerability details

Impact

notifyRewardAmount() function enables operator to add rewards for whitelisted tokens by the owner (the owner sets the whitelisted rewards tokens via addReward() function):

    function notifyRewardAmount(address rewardToken, uint256 amount, uint minRemainingTime) public onlyOperators {
        if (!_rewardData[rewardToken].exists) {
            revert ErrInvalidTokenAddress();
        }

        _updateRewards();
        rewardToken.safeTransferFrom(msg.sender, address(this), amount);

        Reward storage reward = _rewardData[rewardToken];

        uint256 _nextEpoch = nextEpoch();
        uint256 _remainingRewardTime = _nextEpoch - block.timestamp;

        if (_remainingRewardTime < minRemainingTime) {
            revert ErrInsufficientRemainingTime();
        }

        // Take the remainder of the current rewards and add it to the amount for the next period
        if (block.timestamp < reward.periodFinish) {
            amount += _remainingRewardTime * reward.rewardRate;
        }

        // avoid `rewardRate` being 0
        if (amount < _remainingRewardTime) {
            revert ErrNotEnoughReward();
        }

        reward.rewardRate = amount / _remainingRewardTime;
        reward.lastUpdateTime = uint248(block.timestamp);
        reward.periodFinish = _nextEpoch;

        emit LogRewardAdded(amount);
    }

as can be noticed; the reward.rewardRate will have the same decimals of the rewards token.

Whenever the global rewards are updated via _updateRewardsGlobal(); the _rewardData[token_].rewardPerTokenStored is calculated by _rewardPerToken() funtion:

  function _updateRewardsGlobal(address token_, uint256 totalSupply_) internal returns (uint256 rewardPerToken_) {
      uint256 lastTimeRewardApplicable_ = lastTimeRewardApplicable(token_);
      rewardPerToken_ = _rewardPerToken(token_, lastTimeRewardApplicable_, totalSupply_);

      _rewardData[token_].rewardPerTokenStored = rewardPerToken_;
      _rewardData[token_].lastUpdateTime = uint248(lastTimeRewardApplicable_); // safe to cast as this will never overflow
  }

  function _rewardPerToken(address rewardToken, uint256 lastTimeRewardApplicable_, uint256 totalSupply_) public view returns (uint256) {
      if (totalSupply_ == 0) {
          return _rewardData[rewardToken].rewardPerTokenStored;
      }

      uint256 timeElapsed = lastTimeRewardApplicable_ - _rewardData[rewardToken].lastUpdateTime;
      uint256 pendingRewardsPerToken = (timeElapsed * _rewardData[rewardToken].rewardRate * 1e18) / totalSupply_;

      return _rewardData[rewardToken].rewardPerTokenStored + pendingRewardsPerToken;
  }

So as can be noticed; the pendingRewardsPerToken is calculated assuming that the totalSupply_ (that represents the total locked and unlocked amounts of staked token) is of 18 decimals (as it multiplies by 1e18 to get rid of the totalSupply decimals).

But this assumption will not be always true as the staking tokens can be of any decimal.
Same assumption is made when user's rewards are updated, the rewards[user_][token_] is updated via _earned() function:
```
  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];
  }
```
and as can be noticed, the pendingUserRewardsPerToken will have the same decimals of the rewards token, while the balance_ will have the decimals of the staking tokens, so when dividing by 1e18; we are assuming that the staking decimals are 18.
So if the contract is deployed with a staking token of decimals != 18; the rewards calculation will be incorrect, resulting in lower rewards amounts if the staking token decimals are < 18, and resulting in higher rewards amounts (draining the contract rewards) if the staking token decimals are > 18.

Proof of Concept

LockingMultiRewards._rewardPerToken function

    function _rewardPerToken(address rewardToken, uint256 lastTimeRewardApplicable_, uint256 totalSupply_) public view returns (uint256) {
        if (totalSupply_ == 0) {
            return _rewardData[rewardToken].rewardPerTokenStored;
        }

        uint256 timeElapsed = lastTimeRewardApplicable_ - _rewardData[rewardToken].lastUpdateTime;
        uint256 pendingRewardsPerToken = (timeElapsed * _rewardData[rewardToken].rewardRate * 1e18) / totalSupply_;

        return _rewardData[rewardToken].rewardPerTokenStored + pendingRewardsPerToken;
    }

LockingMultiRewards._earned function

    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];
    }

Tools Used

Manual Review.

Recommended Mitigation Steps

Instead of using a magic number to normalize the totalSupply and user balance (1e18); use an immutable variable to save this value based on the staking token decimals upon deployment:

    constructor(
        address _stakingToken,
        uint256 _lockingBoostMultiplerInBips,
        uint256 _rewardsDuration,
        uint256 _lockDuration,
        address _owner
    ) OperatableV2(_owner) {
        if (_lockingBoostMultiplerInBips <= BIPS) {
            revert ErrInvalidBoostMultiplier();
        }

        if (_lockDuration < MIN_LOCK_DURATION) {
            revert ErrInvalidLockDuration();
        }

        if (_rewardsDuration < MIN_REWARDS_DURATION) {
            revert ErrInvalidRewardDuration();
        }

        if (_lockDuration % _rewardsDuration != 0) {
            revert ErrInvalidDurationRatio();
        }

        stakingToken = _stakingToken;
        lockingBoostMultiplerInBips = _lockingBoostMultiplerInBips;
        rewardsDuration = _rewardsDuration;
        lockDuration = _lockDuration;
+       scalingFactor= IERC20(stakingToken).decimals();

        // kocks are combined into the same `rewardsDuration` epoch. So, if
        // a user stake with locking every `rewardsDuration` this should reach the
        // maximum number of possible simultaneous because the first lock gets expired,
        // freeing up a slot.
        maxLocks = _lockDuration / _rewardsDuration;
    }

    function _rewardPerToken(address rewardToken, uint256 lastTimeRewardApplicable_, uint256 totalSupply_) public view returns (uint256) {
        if (totalSupply_ == 0) {
            return _rewardData[rewardToken].rewardPerTokenStored;
        }

        uint256 timeElapsed = lastTimeRewardApplicable_ - _rewardData[rewardToken].lastUpdateTime;
-       uint256 pendingRewardsPerToken = (timeElapsed * _rewardData[rewardToken].rewardRate * 1e18) / totalSupply_;

+       uint256 pendingRewardsPerToken = (timeElapsed * _rewardData[rewardToken].rewardRate * scalingFactor) / totalSupply_;

        return _rewardData[rewardToken].rewardPerTokenStored + pendingRewardsPerToken;
    }

    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];
+       return ((balance_ * pendingUserRewardsPerToken) / scalingFactor) + rewards[user][rewardToken];
    }