Blockian - Permanent freezing of unclaimed yield

[github-actions[bot]]

Blockian

high

Permanent freezing of unclaimed yield

Permanent freezing of unclaimed yield

Description

A user can accidentally freeze potential rewards from the RewardsManager.sol

Root Cause

On claimRewards in RewardsManager contract the epochToClaim_ is not limited to the currentBurnEpoch(), thus allowing a user to send epochToClaim_ > currentBurnEpoch, rendering isEpochClaimed[tokenId_][epochToClaim_] true from all epochs from 0 - epochToClaim_ and disallowing the user rewards he may have received in future epochs

    function claimRewards(
        uint256 tokenId_,
        uint256 epochToClaim_ // not limited
    ) external override {
        if (msg.sender != stakes[tokenId_].owner) revert NotOwnerOfDeposit();

        if (isEpochClaimed[tokenId_][epochToClaim_]) revert AlreadyClaimed(); // will revert for epochs the user didn't receive the rewards from yet

        _claimRewards(tokenId_, epochToClaim_);
    }

Impact

By accidentally sending the wrong epoch a user may freeze unclaimed yield with no way of getting it back

POC

Add this test to the RewardsManager.t.sol

    function testClaimRewardsFroozeUnclaimedYield() external {
        skip(10);

        uint256[] memory depositIndexes = new uint256[](5);
        depositIndexes[0] = 9;
        depositIndexes[1] = 1;
        depositIndexes[2] = 2;
        depositIndexes[3] = 3;
        depositIndexes[4] = 4;
        MintAndMemorializeParams memory mintMemorializeParams = MintAndMemorializeParams({
            indexes: depositIndexes,
            minter: _minterOne,
            mintAmount: 1000 * 1e18,
            pool: _poolOne
        });

        uint256 tokenIdOne = _mintAndMemorializePositionNFT(mintMemorializeParams);
        _stakeToken(address(_poolOne), _minterOne, tokenIdOne);

        uint256 currentBurnEpoch = _poolOne.currentBurnEpoch();

        changePrank(_minterOne);
        _rewardsManager.claimRewards(tokenIdOne, currentBurnEpoch + 10);

        for (uint i = 1; i <= 10; i++) {
            vm.expectRevert(IRewardsManagerErrors.AlreadyClaimed.selector);
            _rewardsManager.claimRewards(tokenIdOne, currentBurnEpoch + i); // the user got all 10 next burn epochs rewards frozen as he receives AlreadyClaimed for all of them 
        }
    }

Fix suggestions

There are 2 main ways to fix this issue. Either limit the for loops that depend on epochToClaim_ for example instead of

for (uint256 epoch = lastBurnEpoch; epoch < epochToClaim_; )

use

uint256 curBurnEpoch = IPool(ajnaPool).currentBurnEpoch();
uint256 maxEpoch = epochToClaim_ > curBurnEpoch ? curBurnEpoch : epochToClaim_;
for (uint256 epoch = lastBurnEpoch; epoch < maxEpoch; )

Which is a BAD solution (explanation in detail in the next issue)

Or simply add a require statement when calling claimRewards

    function claimRewards(
        uint256 tokenId_,
        uint256 epochToClaim_
    ) external override {
        if (epochToClaim_ > IPool(stakes[tokenId_].ajnaPool).currentBurnEpoch()) revert EpochNotAvailableYet();

        // rest of the function
    }

Which is a better solution