Core Contracts

Summary

The FeeCollector::userRewards and veRAACToken contracts are not synchronized correctly, leading to miscalculations in reward distribution. New users locking tokens can inadvertently claim rewards meant for existing users, resulting in losses for the original token holders.

Vulnerability Details

The reward calculation mechanism in the FeeCollector contract relies on the voting power from the veRAACToken contract, userRewards[user], and totalDistributed. As shown below, any changes in totalVotingPower and userVotingPower impact the share value allocated to users:

However, userRewards[user] is only updated when the FeeCollector::claimRewards() function is called:

When a user locks tokens using veRAACToken::lock(), it alters both userVotingPower and totalVotingPower, affecting reward calculations for all users. Since userRewards[user] is not updated accordingly, the new user ends up claiming rewards that belong to others.

Poc

Add the following test to test/unit/core/collectors/FeeCollector.test.js and execute it:

output:

This test confirms that if a user does not claim their rewards promptly, a newly locked user can receive a portion of the rewards intended for others, leading to financial losses for original token holders.

Impact

The misalignment between userRewards[user] and voting power updates allows new users to claim rewards that should belong to existing users, effectively causing a loss of rewards for the original participants.

Tools Used

Manual Review

Recommendations

To mitigate the reward distribution issue and prevent losses for previously locked users, we recommend implementing the following fixes:

1. Update userRewards[user] Before Modifying

   Voting PowerBefore a user locks tokens and gains VotingPower, their pending rewards should be claimed to update userRewards[user].

   After updating userRewards[user], the user's VotingPower can then be modified.

2. Avoid Using Dynamic Data in _calculatePendingRewards()

   The function _calculatePendingRewards() currently relies on dynamically calculated voting power (userVotingPower and totalVotingPower). This approach introduces an inconsistency when a new user locks tokens, as their VotingPower is immediately included in the reward calculation.

   Even if userRewards[user] is correctly updated for the new user, the dynamically calculated VotingPower will cause the new user's share to be included in the reward pool. This results in the original user's rewards being locked in the contract instead of being claimable.

   To resolve this, reward calculations should be decoupled from dynamic voting power updates and instead be based on fixed snapshots of VotingPower at specific distribution periods. This can be done by integrating _calculatePendingRewards() with FeeCollector::distributeCollectedFees() to ensure that rewards are distributed according to predefined periods rather than fluctuating with every user action.