Submission Details
Severity:
Incorrect assignment of claimed user rewards in claimUserRewards could prevent user from collecting further rewards
Summary
Due to an incorrect assigmnet of the userRewards[user] in the claimRewards function of the FeeCollector contract for user to the totalDistributed could lead the user to be unable to claim rewards in future distribution periods after his/her initial claim.
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
//@Audit incorrect value, this should be the user shares assigned to the after user claim not the total distributed rewards
userRewards[user] = totalDistributed;
// Transfer rewards
raacToken.safeTransfer(user, pendingReward);
emit RewardClaimed(user, pendingReward);
return pendingReward;
}
Vulnerability Details
Since we are assiging the totalDistributed to the user rewards(userRewards[user]) after we complete the initial claim of rewards this value would be higher than the actual shares that the user claimed. In future claims for a different distribution period of fees we have the following logic inside of the _calculatePendingRewards function:
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;
//@Audit even if user rewards shares have grown we are comparing them to the previous
//total distributable rewards, which would mean if share for user have grown by less than the value
//of the previos period total distributal rewards user will get 0 rewards
return share > userRewards[user] ? share - userRewards[user] : 0;
}
Since user shares could have grown by a smaller amount than the totalDistributed value for the previous distribution the call to the claimRewards function results in a InsufficientBalance revert as rewards to claim is incorrectly returned as 0.
Impact
User unable to claim RAACToken rewards from FeeCollector contract if new shares are less than the value of the total distributed rewards from the previous period.
Tools Used
Manual Review
Unit test
PoC
Place the test inside of the FeeCollector.test.js file inside of the describe("Fee Collection and Distribution") test suite group
it("PoC user can't claim rewards due to incorrect distributed user rewards snapshot", async function () {
//We burn the initial minted tokens for user1 for easier tracability of the test
await raacToken.connect(user1).burn(await raacToken.balanceOf(user1.address));
//We distribute the collected fees from the setup
await feeCollector.connect(owner).distributeCollectedFees();
let distributedRewards = await feeCollector.totalDistributed();
console.log("Total distributed rewards after first fee collection: ", distributedRewards);
//We expect the total distributed rewards for VAAC holders to be 5000000000000000, this is 50% of the collected fees
expect(distributedRewards).to.equal(5000000000000000);
//We simulate that the period has passed
await time.increase(WEEK);
const initialBalance = await raacToken.balanceOf(user1.address);
console.log("Initial rewards token balance of user: ", initialBalance.toString());
expect(initialBalance).to.equal(0);
//We collect the rewards that the user is obligated to
const collectedReward = await feeCollector.connect(user1).claimRewards(user1.address);
//User should have now received the Raac token rewards in his address
const user1Balance = await raacToken.balanceOf(user1.address);
console.log("User1 rewards token balance after claiming: ", user1Balance.toString());
expect(user1Balance.toString()).to.equal("3205479452054796");
//We now collect a new protocol fee for the new period that some user provides
await feeCollector.connect(user2).collectFee(5000000000000000, 0);
//We distribute the collected fees from the new period, in this case we will have only the protocol fee this time
await feeCollector.connect(owner).distributeCollectedFees();
distributedRewards = await feeCollector.totalDistributed();
console.log("Total distributed rewards after second fee collection: ", distributedRewards);
//We expect the total distributed rewards for VAAC holders to be 5000000000000000 + 50% of the newly collected 5000000000000000
// since the protocol fee has a weight of 50%
expect(distributedRewards).to.equal(7500000000000000n);
//We simulate that the period has passed
await time.increase(WEEK);
//Distribution fails because the new amount of shares generated for the user < the total distribution from the previous period
//This is incorrect as user shares have increased and user should get the (currentShares - alreadyDistributedShares) amount of RAAC reward tokens
await expect(feeCollector.connect(user1).claimRewards(user1.address)).to.be.revertedWithCustomError(feeCollector, "InsufficientBalance")
});
Console log output from test:
Total distributed rewards: 5000000000000000n
Initial rewards token balance of user: 0
User1 rewards token balance after claiming: 3205479452054796
Total distributed rewards: 7500000000000000n
Recommendations
Assigned the user claimed shares to the userRewards mapping instead of the 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;
+ userRewards[user] = pendingReward;
// Transfer rewards
raacToken.safeTransfer(user, pendingReward);
emit RewardClaimed(user, pendingReward);
return pendingReward;
}
Updated PoC after changes
Console log output:
Total distributed rewards after first fee collection: 5000000000000000n
Initial rewards token balance of user: 0
User1 rewards token balance after first claiming: 3205479452054796
Total distributed rewards after second fee collection: 7500000000000000n
User1 rewards token balance after second claiming: 4712328767123292
it("PoC user claims rewards after multiple distribution periods", async function () {
//We burn the initial minted tokens for user1 for easier tracability of the test
await raacToken.connect(user1).burn(await raacToken.balanceOf(user1.address));
//We distribute the collected fees from the setup
await feeCollector.connect(owner).distributeCollectedFees();
let distributedRewards = await feeCollector.totalDistributed();
console.log("Total distributed rewards after first fee collection: ", distributedRewards);
//We expect the total distributed rewards for VAAC holders to be 5000000000000000, this is 50% of the collected fees
expect(distributedRewards).to.equal(5000000000000000);
//We simulate that the period has passed
await time.increase(WEEK);
const initialBalance = await raacToken.balanceOf(user1.address);
console.log("Initial rewards token balance of user: ", initialBalance.toString());
expect(initialBalance).to.equal(0);
//We collect the rewards that the user is obligated to
const collectedReward = await feeCollector.connect(user1).claimRewards(user1.address);
//User should have now received the Raac token rewards in his address
const user1Balance = await raacToken.balanceOf(user1.address);
console.log("User1 rewards token balance after first claiming: ", user1Balance.toString());
expect(user1Balance.toString()).to.equal("3205479452054796");
//We now collect a new protocol fee for the new period that some user provides
await feeCollector.connect(user2).collectFee(5000000000000000, 0);
//We distribute the collected fees from the new period, in this case we will have only the protocol fee this time
await feeCollector.connect(owner).distributeCollectedFees();
distributedRewards = await feeCollector.totalDistributed();
console.log("Total distributed rewards after second fee collection: ", distributedRewards);
//We expect the total distributed rewards for VAAC holders to be 5000000000000000 + 50% of the newly collected 5000000000000000
// since the protocol fee has a weight of 50%
expect(distributedRewards).to.equal(7500000000000000n);
//We simulate that the period has passed
await time.increase(WEEK);
// Distribution now correctly distributes rewards for user the second time
await feeCollector.connect(user1).claimRewards(user1.address);
const user1Balance2 = await raacToken.balanceOf(user1.address);
console.log("User1 rewards token balance after second claiming: ", user1Balance2.toString());
expect(user1Balance2.toString()).to.equal("4712328767123292");
});
Updates
Lead Judging Commences
inallhonesty 7 months ago
Submission Judgement Published Assigned finding tags:
FeeCollector::claimRewards sets `userRewards[user]` to `totalDistributed` seriously grieving users from rewards
Prize pool breakdown
Total prize
77,280 USDC
nSLOC
3,86420
USDC / LOC
74,000
3,280
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.