Frequently and Repeated Calls to `setRewards()` with Zero Total Supply Results in Imprecise Reward Rate Calculations

[hats-bug-reporter[bot]]

Github username: @MatinR1 Twitter username: MatinRezaii1 Submission hash (on-chain): 0x397e9f1417ce4ae30974d1f722a3b3e4e9dd1f65ec8e5dc30262e578b963d6a0 Severity: high

Description: Description\

The rewards cannot be fully distributed and are ultimately locked in the contract.

The function setRewards() calls _setRewards():

    function _setRewards(uint amount, uint duration)
        internal
        validAmount(amount)
        validDuration(duration)
    {
        _update(address(0));
        // If rewardsEnd is already reached
        if (block.timestamp >= rewardsEnd) {
            // Dont include previous reward Rate
            rewardRate = amount / duration;
        } else {
            // Calculate remaining rewards supposed to go back into the pool
            uint remainingRewards = (rewardsEnd - block.timestamp) * rewardRate;
            // Add new Amount to previous amount and calculate rate
            rewardRate = (amount + remainingRewards) / duration;
        }
    ...
    }

This means that for the cases where the block.timestamp < rewardsEnd or in other words, the rewardsEnd is not reached, the remainings reward is calculated, and the result is added to the rewardRate. There is a vulnerability in the _setRewards() function, to be specific inside the _calculateRewardValue() function, when the total supply is zero. The mentioned function first checks for a zero total supply and then updates the reward rate:

    function _calculateRewardValue() internal view returns (uint) {
        // In case the totalSupply is 0 the rewardValue doesnt change
        if (totalSupply == 0) {
            return rewardValue;
        }

        return (_getRewardDistributionTimestamp() - lastUpdate) // Get the time difference between the last time it was updated and now (or in case the reward period ended the rewardEnd timestamp)
            * rewardRate // Multiply it with the rewardrate to get the rewards distributed for all of the stakers together
            * 1e36 // for the later division we need a value to compensate for the loss of precision. This value will be counteracted in earned()
            / totalSupply // divide it by the totalSupply to get the rewards per token
            + rewardValue; // add the old rewardValue to the new "single" rewardValue
    }

This tells us that if the totalSupply() is zero, then the reward value does not change and it returns the previous reward value. This itself is not an issue, however, if considered the reaminingRewards calculations of setRewards(), it becomes an issue. Consider this situation:

1. The initial state is as follows: totalSupply = 0, rewardsEnd = 0, duration = 100
2. call setRewards(1000, 100) at time 0: rewardPerToken = 0, rewardRate = 1000 / 100 = 10, rewardsEnd = 0 + 100 = 100
3. call setRewards(1000, 100) at time 50: rewardPerToken = 0, leftover = (100 - 50) * 10 = 500, reward = 1000 + 500 = 1500, rewardRate = 1500 / 100 = 15
4. But at this moment, there are 2000 reward tokens in the contract which are not recorded in _calculateRewardValue(). The rewardRate should have been 2000 / duration = 2000 / 100 = 20 instead of the current 15.

Attack Scenario\ POC is provided for this type of vulnerability.

Attachments

1. Proof of Concept (PoC) File

Make _calculateRewardValue() function public and add this test to LM_PC_Staking_v1Test:

    function testSetRewardsIfTotalSupplyIsZero() public {

        uint256 newReward = 1000;
        uint256 newReward2 = 1000;
        uint256 duration = 100;

        stakingManager.setRewards(newReward, duration);

        assertEq(stakingManager.rewardRate(), 10);
        assertEq(stakingManager.rewardsEnd(), 101);
        assertEq(stakingManager._calculateRewardValue(), 0);
        // advance the blocktime by duration / 2 to simulate that the period is almost finished.
        vm.warp(block.timestamp + (duration / 2));

        stakingManager.setRewards(newReward2, duration);

        assertEq(stakingManager.rewardRate(), 15);
        assertEq(stakingManager.rewardsEnd(), 151);
        assertEq(stakingManager._calculateRewardValue(), 0);
    }

Output:

[⠔] Compiling...
No files changed, compilation skipped

Running 1 test for test/modules/logicModule/LM_PC_Staking_v1.t.sol:LM_PC_Staking_v1Test
[PASS] testSetRewardsIfTotalSupplyIsZero() (gas: 126358)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.69ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

The last call to rewardRate() must be 20 instead of 15. This test shows the significant reward loss when the total supply is zero.

2. Revised Code File (Optional)

In _setRewards(), if _calculateRewardValue() returns 0, consider all previously accumulated tokens inside the remainingRewards.

[PlamenTSV]

The 500 tokens you deem "stuck" are not stuck, they are tokens that have been distributed to stakers.

The issue is that those 500 tokens are stuck if nobody stakes for that period, which is a dupe of #66.

@0xmahdirostami

[MatinR1]

I respectfully disagree with the assertion that Issue #123 is a duplicate of Issue #66. While both issues pertain to the setRewards() function, they address fundamentally different aspects:

1. Issue #66 focuses on general inefficiencies and potential miscalculations in the reward setting process, without specifying conditions under which the issue is most impactful.
2. Issue #123 specifically identifies the scenario where multiple and frequent calls to setRewards() during zero total supply lead to rewards being locked and not distributed, highlighting a precise condition (zero total supply) and its specific impact on reward distribution. The unique focus and the specific conditions highlighted in Issue#123 are not covered in Issue#66, making them distinct issues. Therefore, As I demonstrated in the Proof of Concept, which is available for anyone to test, Issue#123 should be considered independently to address its unique implications on the protocol.

[FHieser]

Yes @MatinR1 I agree that this is a different usecase, but this still is based on the assumption that the reward should be fully available in the next reward period. If we assume that the distribution of funds in the first half is done (even if 50% of the funds didnt reach anyone) then your claim doesnt hold up anymore