Malicious user can block rewards accumulation by playing harvesting rewards

[hats-bug-reporter[bot]]

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

Description: Description

SingleTokenVirtualRewarderUpgradeable::harvest harvests rewards for a specific tokenId. It calculates the available rewards for the token and updates the lastEarnEpoch. On subsequent calls, it harvests from the next epoch of lastEarnEpoch. The issue is that an epoch extends for a week, and rewards issued after the harvest in an epoch are not considered in the reward calculation. Thus, a malicious user can call harvest from the strategy at the very start of every epoch, rendering all rewards of that specific epoch useless

Proof of Concept (PoC) File

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

        uint256 currentEpoch = _currentEpoch();
        tokensInfo[tokenId_].lastEarnEpoch = currentEpoch;

        emit Harvest(tokenId_, reward, currentEpoch);
        return reward;
    }

harvest calculates rewards within (lastEarnEpoch, currentEpoch] using the_calculateAvailableRewardsAmountinternal method and updates the lastEarnEpoch to currentEpoch.

A malicious user can call harvest from the strategy at the very start of every epoch, i.e., at block.timestamp = (block.timestamp / WEEK) * WEEK, which will ignore all the notified rewards in the epoch.

solidity

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

        uint256 currentEpoch = _currentEpoch();
//@audit-issue Rewards become useless since harvest can front run notifyRewardAmount in a epcch
        rewardsPerEpoch[currentEpoch] += amount_;

        emit NotifyReward(amount_, currentEpoch);
    }

A malicious user only needs to harvest once per epoch before any notifyRewardAmount() to block reward calculations.

Revised Code File (Optional)

Implement a cooldown period in two consecutive harvesting rewards or add a trusted admin who will harvest rewards at appropriate times

[0xmahdirostami]

The scenario is right, however the harvest and notify functions can only called by the Strategy contract

[Tri-pathi]

@0xmahdirostami

however the harvest and notify functions can only called by the Strategy contract

How does this line invalidate the issue ?

[0xmahdirostami]

Upon investigating the issue, it was discovered that partially withdrawing a position or calling the harvest function multiple times during an epoch can result in the loss of subsequent rewards distributed to the contract after the harvest. This occurs because the system uses epoch indexes rounded down to the start of the epoch. The harvest function, callable by the strategy and after the detachment of an NFT, allows the NFT owner to withdraw their staked amount and harvest rewards. This pattern prevents the loss here, however, I acknowledge that multiple attaches/detaches within an epoch lead to imprecise calculations. Providing a proof of concept (POC) would be beneficial to assess the impact of this issue.

[Tri-pathi]

Heyy @0xmahdirostami , I think you have got the idea of issue.

Impact is very clear since harvest() will be called every time detaching the NFT from strategy contracts, so Malicious users can easily perform these attacks. I have already explained in the submission that calling harvest() in the starting of the epoch will prevent any further reward accumulation in that epoch since lastEarnEpoch is updated to current epoch.

The scenario is right, however the harvest and notify functions can only called by the Strategy contract

I was confused from the above comments since harvest() doesn't need any special trusted admin access in the strategy contracts anyone can detach their NFT, withdrawing all associated rewards .