Lines of code

Vulnerability details

Impact

Some rewards might be locked inside the contract due to the rounding loss.

Proof of Concept

_claimAndSyncRewards() claimed the rewards from the staking contract and tracks rewardsPerShare with the current supply.

    function _claimAndSyncRewards() internal virtual {
        uint256 _totalSupply = totalSupply();
        if (_totalSupply == 0) {
            return;
        }
        _claimAssetRewards();
        uint256 balanceAfterClaimingRewards = rewardToken.balanceOf(address(this));

        uint256 _rewardsPerShare = rewardsPerShare;
        uint256 _previousBalance = lastRewardBalance;

        if (balanceAfterClaimingRewards > _previousBalance) {
            uint256 delta = balanceAfterClaimingRewards - _previousBalance;
            // {qRewards/share} += {qRewards} * {qShare/share} / {qShare}
            _rewardsPerShare += (delta * one) / _totalSupply; //@audit possible rounding loss
        }
        lastRewardBalance = balanceAfterClaimingRewards;
        rewardsPerShare = _rewardsPerShare;
    }

It uses one as a multiplier and from this setting we know it has the same decimals as underlying(thus totalSupply).

My concern is _claimAndSyncRewards() is called for each deposit/transfer/withdraw in _beforeTokenTransfer() and it will make the rounding problem more serious.

1. Let’s consider underlyingDecimals = 18. totalSupply = 10**6 with 18 decimals, rewardToken has 6 decimals.
   And total rewards for 1 year are 1M rewardToken for 1M totalSupply.
2. With the above settings, _claimAndSyncRewards() might be called every 1 min due to the frequent user actions.
3. Then expected rewards for 1 min are 1000000 / 365 / 24 / 60 = 1.9 rewardToken = 1900000 wei.
4. During the division, it will be 1900000 * 1018 / (1000000 * 1018) = 1.
   So users would lose almost 50% of rewards due to the rounding loss and these rewards will be locked inside the contract.

Tools Used

Manual Review

Recommended Mitigation Steps

I think there would be 2 mitigation.

1. Use a bigger multiplier.

2. Keep the remainders and use them next time in _claimAndSyncRewards() like this.

   if (balanceAfterClaimingRewards > _previousBalance) {
   uint256 delta = balanceAfterClaimingRewards - _previousBalance; //new rewards
   uint256 deltaPerShare = (delta * one) / _totalSupply; //new delta per share

      // decrease balanceAfterClaimingRewards so remainders can be used next time
      balanceAfterClaimingRewards = _previousBalance + deltaPerShare * _totalSupply / one;
   
      _rewardsPerShare += deltaPerShare;
   

   }
   lastRewardBalance = balanceAfterClaimingRewards;

Assessed type

Math

---

The text was updated successfully, but these errors were encountered:

All reactions