Core Contracts
Regnum Aurum Acquisition Corp
HardhatReal World AssetsNFT
77,280 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Incorrect User Reward Accounting in FeeCollector's Claim Function Causes DOS on Subsequent Claims

uddercover

Summary

In the FeeCollector contract, the mechanism used to update user reward tracking during claims causes a denial-of-service (DOS) condition. After a user makes their first claim, the contract updates their reward record to the entire totalDistributed value, making subsequent reward calculations return zero even when additional fees are collected. This design flaw causes users to be permanently unable to claim further rewards, locking funds that they are entitled to.

Vulnerability Details

The issue arises because FeeCollector::userRewards for each user is set to the total distributed amount in FeeCollector::claimRewards, making it mathematically impossible for most users to receive future rewards.

After the first claim, users with voting power <= ((totalDistributed at the time of their previous claim * 100% )/ totalDistributed at the time of their current claim) of totalVotingPower (i.e x% of total voting power) will be unable to claim again as at that point, their share would be less than or equal to userRewards[user], causing `FeeCalculator::_calculatePendingRewards` to return 0.

function _calculatePendingRewards(address user) public 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;
}

This would be a large part of the user base as even if the difference between the previous totalDistributed and the current totalDistributed is very large (causing a very small percentage requirement for example 0.5%), all users who claimed at a particular period cannot each have the necessary percentage voting power required because a percentage of anything must add up to 100%, 500 users cannot each have 0.5% of the total.

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;
// Transfer rewards
raacToken.safeTransfer(user, pendingReward);
​
emit RewardClaimed(user, pendingReward);
return pendingReward;
}

POC

To use foundry in the codebase, follow the hardhat guide here: Foundry-Hardhat hybrid integration by Nomic foundation.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
import {FeeCollector} from "../../../../contracts/core/collectors/FeeCollector.sol";
import {RAACToken} from "../../../../contracts/core/tokens/RAACToken.sol";
import {veRAACToken} from "../../../../contracts/core/tokens/veRAACToken.sol";
import {Test, console} from "forge-std/Test.sol";
​
contract TestSuite is Test {
FeeCollector feeCollector;
RAACToken raacToken;
veRAACToken veRAACTok;
address treasury;
address repairFund;
address admin;
uint256 initialSwapTaxRate = 100; //1%
uint256 initialBurnTaxRate = 50; //0.5%
​
function setUp() public {
treasury = makeAddr("treasury");
repairFund = makeAddr("repairFund");
admin = makeAddr("admin");
​
raacToken = new RAACToken(admin, initialSwapTaxRate, initialBurnTaxRate);
veRAACTok = new veRAACToken(address(raacToken));
feeCollector = new FeeCollector(address(raacToken), address(veRAACTok), treasury, repairFund, admin);
​
vm.startPrank(admin);
raacToken.setFeeCollector(address(feeCollector));
raacToken.setMinter(admin);
vm.stopPrank();
}
​
function testDOSAfterFirstClaim() public {
//three users mints veraac and each have varying percentages of totalVotingSupply
address user1 = makeAddr("user1");
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
​
uint256 maxDuration = veRAACTok.MAX_LOCK_DURATION();
uint256 minDuration = veRAACTok.MIN_LOCK_DURATION();
uint256 mintAmount = 5e18;
uint256 feeAmount = 1e18;
​
vm.startPrank(admin);
raacToken.mint(admin, feeAmount * 2);
raacToken.mint(user1, mintAmount);
raacToken.mint(user2, mintAmount);
raacToken.mint(user3, mintAmount);
raacToken.approve(address(feeCollector), feeAmount * 2);
feeCollector.collectFee(feeAmount, 0);
vm.stopPrank();
​
vm.startPrank(user1);
raacToken.approve(address(veRAACTok), mintAmount);
veRAACTok.lock(mintAmount, maxDuration);
console.log("User1 veRaac balance: ", veRAACTok.balanceOf(user1));
vm.stopPrank();
​
vm.startPrank(user2);
raacToken.approve(address(veRAACTok), mintAmount);
veRAACTok.lock(mintAmount, minDuration);
console.log("User2 veRaac balance: ", veRAACTok.balanceOf(user2));
vm.stopPrank();
​
vm.startPrank(user3);
raacToken.approve(address(veRAACTok), mintAmount);
veRAACTok.lock(mintAmount, maxDuration);
console.log("User3 veRaac balance: ", veRAACTok.balanceOf(user3));
vm.stopPrank();
​
//admin calls distribute
vm.prank(admin);
feeCollector.distributeCollectedFees(); //a bug in this function prevents this from working i.e `if (contractBalance < totalFees) revert InsufficientBalance();` in _processDistributions function as contractBalance would always be less than tracked fees because of the raacToken feeOnTransfer.
uint256 previousTotalDistributedAmount = feeCollector.totalDistributed();
console.log("totalDistributed value after first distribution: ", previousTotalDistributedAmount);
​
//users claim tokens
vm.prank(user1);
feeCollector.claimRewards(user1);
​
vm.prank(user2);
feeCollector.claimRewards(user2);
​
vm.prank(user3);
feeCollector.claimRewards(user3);
​
vm.warp(7 days + 1);
//admin calls distribute again
vm.startPrank(admin);
feeCollector.collectFee(feeAmount, 0);
feeCollector.distributeCollectedFees();
vm.stopPrank();
uint256 currentTotalDistributedAmount = feeCollector.totalDistributed();
console.log("totalDistributed value after second distribution: ", currentTotalDistributedAmount);
​
//users can no longer claim tokens as none of them have more than 50% of total voting power i.e (previousTotalDistributed amount/currentTotalDistributed amount) * feeCollector.totalVotingPower(). And so _calculatePendingRewards function always returns 0
vm.prank(user1);
vm.expectRevert();
feeCollector.claimRewards(user1);
​
vm.prank(user2);
vm.expectRevert();
feeCollector.claimRewards(user2);
​
vm.prank(user3);
vm.expectRevert();
feeCollector.claimRewards(user3);
​
assertEq(0, feeCollector.getPendingRewards(user1)); //function made public for ease of access
assertEq(0, feeCollector.getPendingRewards(user2));
assertEq(0, feeCollector.getPendingRewards(user3));
console.log("Balance of feeCollector: ", raacToken.balanceOf(address(feeCollector)));
}
}

Impact

Most users are unable to claim rewards they are entitled to, causing accumulation of unclaimable rewards.

Tools Used

Manual review, foundry test suite

Recommendations

Consider redesigning the claim function to track actual user rewards claimed, and using that in determining the user's future claim amount.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 month ago
Submission Judgement Published
Validated
Assigned finding tags:

FeeCollector::claimRewards sets `userRewards[user]` to `totalDistributed` seriously grieving users from rewards

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.