Rewards are distributed in a epoch even if there are no stakers/deposits, locking the rewards forever

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xce04d08d4bcf161311d406633d2527510b4267866d5787086e34c60918cd78ab Severity: high

Description: Description

SingelTokenVirtualRewarderUpgradeable contract's Rewarding logic is similar to Synthetix rewards distribution contract,with modifations. The scenario where there are zero deposits(zero supply), all the rewards added will be locked in the contract

Proof of Concept (PoC) File

    function notifyRewardAmount(uint256 amount_) external onlyStrategy {
        if (amount_ == 0) {
            revert ZeroAmount();
        }

        uint256 currentEpoch = _currentEpoch();
        rewardsPerEpoch[currentEpoch] += amount_;

        emit NotifyReward(amount_, currentEpoch);
    }

Strategy updates the new reward amount to be distributed in the current epoch, but if there are no stakers i.e zero totalSuplly all the rewards added for distribution in this epoch will be locked forever.

if we see harvest() function which Harvests rewards for a specific tokenId. it skips the reward for epoch which had supply = 0,

    function harvest(uint256 tokenId_) external onlyStrategy returns (uint256 reward) {
        reward = _calculateAvailableRewardsAmount(tokenId_);

    .................................
    }

    function _calculateAvailableRewardsAmount(uint256 tokenId_) internal view returns (uint256 reward) {
     .....................................................

          uint256 currentEpoch = _currentEpoch();
        uint256 notHarvestedEpochCount = (currentEpoch - startEpoch) / _WEEK;

        for (uint256 i; i < notHarvestedEpochCount; ) {
            reward += _calculateRewardPerEpoch(tokenId_, startEpoch);//@audit-issue if supply zero reward for that epoch is lost

            startEpoch += _WEEK;
            unchecked {
                i++;
            }
        }
    }

    function _calculateRewardPerEpoch(uint256 tokenId_, uint256 epoch_) internal view returns (uint256) {
        uint256 balance = VirtualRewarderCheckpoints.getAmount(
            tokensInfo[tokenId_].balanceCheckpoints,
            tokensInfo[tokenId_].checkpointLastIndex,
            epoch_
        );

        uint256 supply = VirtualRewarderCheckpoints.getAmount(totalSupplyCheckpoints, totalSupplyCheckpointLastIndex, epoch_);

        if (supply == 0) {
            return 0;
        }

        return (balance * rewardsPerEpoch[epoch_ + _WEEK]) / supply;
    }

Because of this, even when there are no users staking, the accounting logic still thinks rewards are being dispersed during that timeframe (because the harvest updates the lastEarnEpoch to new one)

In such cases , the rewards that should have gone to the first deposits will instead accrue to nobody, and be locked in the contract forever

[0xmahdirostami]

Lead: Valid scenario, although the function notifyRewardAmount cannot be called with arbitrary data as it is handled inside the strategy contract

[Tri-pathi]

@0xmahdirostami This issue doesn't come from calling notifyRewardAmount with arbitrary data from the strategy contract. notifyRewardAmount is working as expected: notifying reward tokens for distribution.

Suppose there is some supply, but stakers withdraw their stakes, making the supply 0, and the reward amount will be lost.

I believe this is a valid issue. The same issue is accepted in many contest platforms since the scenario is possible and it will lead to the loss of reward tokens- https://solodit.xyz/issues/rewards-are-calculated-as-distributed-even-if-there-are-no-stakers-locking-the-rewards-forever-cyfrin-none-cyfrin-templedao-v21-markdown

[0xmahdirostami]

Yes there are similarities with the provided issue report, however, the Fenix staking contracts uses an indexing mechanism to keep track of the total supplies, thus, when total supply becomes zero, even during an epoch, thus it would return 0 for that index. It would be beneficial providing a detailed POC for demonstrating the loss here if in entire epoch there is no stakers however the rewards are distributed.

[Tri-pathi]

@0xmahdirostami

I can't see any implementation that prevents calling notifyRewardAmount when the supply goes to zero.

Suppose there is one staker with some supply. When the strategy calls notifyRewardAmount, the staker could front-run the transaction and withdraw all their tokens, making the supply zero again and resulting in a loss of reward tokens

Although frontrunning doesn't make direct sense in this context, but it highlights how a loss could occur if notifyRewardAmount is called when the total supply is zero