Summary
Once rewards are claimed by a user, further rewards calculations are incorrect and the users gets less rewards than it must be.
Vulnerability Details
Function FeeCollector::claimRewards sets userRewards[user] to totalDistributed:
function claimRewards(address user) external override nonReentrant whenNotPaused returns (uint256) {
if (user == address(0)) revert InvalidAddress();
uint256 pendingReward = _calculatePendingRewards(user);
if (pendingReward == 0) revert InsufficientBalance();
userRewards[user] = totalDistributed;
raacToken.safeTransfer(user, pendingReward);
emit RewardClaimed(user, pendingReward);
return pendingReward;
}
Function FeeCollector::_calculatePendingRewards calculates rewards using time-weighted average and subtracts the rewards that already has been claimed:
function _calculatePendingRewards(address user) internal view returns (uint256) {
uint256 userVotingPower = veRAACToken.getVotingPower(user);
if (userVotingPower == 0) return 0;
uint256 totalVotingPower = veRAACToken.getTotalVotingPower();
if (totalVotingPower == 0) return 0;
uint256 share = (totalDistributed * userVotingPower) / totalVotingPower;
return share > userRewards[user] ? share - userRewards[user] : 0;
}
But the issue here is that the value of userRewards[user] after rewards are claimed is totalDistributed, and if there are other holders of veRAACToken the calculation will be incorrect since totalDistributed is total distributed fee across all users.
I created a test that shows that the user does not get rewards. You can add the test to FeeCollector tests in ``:
it("should allow users to claim rewards several times", async function () {
await feeCollector.connect(owner).distributeCollectedFees();
await time.increase(WEEK);
const initialBalance = await raacToken.balanceOf(user1.address);
await feeCollector.connect(user1).claimRewards(user1.address);
expect(await raacToken.balanceOf(user1.address)).to.be.gt(initialBalance);
expect(await feeCollector.getPendingRewards(user1.address)).to.be.eq(0);
const taxRate = SWAP_TAX_RATE + BURN_TAX_RATE;
const grossMultiplier = BigInt(BASIS_POINTS * BASIS_POINTS) / BigInt(BASIS_POINTS * BASIS_POINTS - taxRate * BASIS_POINTS);
const protocolFeeGross = ethers.parseEther("50") * grossMultiplier / BigInt(10000);
await feeCollector.connect(user1).collectFee(protocolFeeGross, 0);
await feeCollector.connect(owner).distributeCollectedFees();
await time.increase(WEEK);
expect(await feeCollector.getPendingRewards(user1.address)).to.be.gt(0);
});
Impact
Users get less rewards than they expected to get. This may lead to not getting rewards at all after the first claim if the user's share of veRAACToken is not large.
Tools Used
Manual review
Recommendations
Function FeeCollector::claimRewards must set userRewards[user] to pendingReward :
function claimRewards(address user) external override nonReentrant whenNotPaused returns (uint256) {
if (user == address(0)) revert InvalidAddress();
uint256 pendingReward = _calculatePendingRewards(user);
if (pendingReward == 0) revert InsufficientBalance();
// Reset user rewards before transfer
- userRewards[user] = totalDistributed;
+ userRewards[user] = pendingReward;
// Transfer rewards
raacToken.safeTransfer(user, pendingReward);
emit RewardClaimed(user, pendingReward);
return pendingReward;
}
