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

Incorrect Reward Accumulation in claimRewards Function of `FeeCollector` Contract

roccomania

Summary

The FeeCollector::claimRewards function incorrectly updates the userRewards mapping by setting it to totalDistributed instead of accumulating the pending rewards. This bug prevents users from claiming rewards after their first claim, as subsequent claims will always revert with an InsufficientBalance error.

Vulnerability Details

Explanation

The issue arises in the claimRewards function, where the userRewards mapping is updated incorrectly. Instead of adding the pendingReward to the existing userRewards[user], the function sets userRewards[user] to totalDistributed. This leads to incorrect reward tracking and prevents users from claiming rewards more than once.

Root Cause in the Contract Function

In the claimRewards function, the following line is problematic:

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; //@audit-issue It should be +=pendingReward
// Transfer rewards
raacToken.safeTransfer(user, pendingReward);
emit RewardClaimed(user, pendingReward);
return pendingReward;
}

This line overwrites the user's reward balance with totalDistributed, which is incorrect. The correct behavior should be to accumulate the pending rewards by adding them to the existing balance:

Additionally, the _calculatePendingRewards function calculates the user's share of rewards based on their voting power and compares it to their current userRewards balance:

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;
}

Since userRewards[user] is incorrectly set to totalDistributed after the first claim, the share will always be less than or equal to userRewards[user] in subsequent claims after the FeeCollector collects more fees. This causes the function to return 0, triggering the InsufficientBalance error.

Proof of Concept

Scenario Example

  1. First Claim: A user claims their rewards successfully. The userRewards mapping is incorrectly set to totalDistributed.

  2. Second Claim: The user attempts to claim rewards after additional fees are collected. The _calculatePendingRewards function returns 0 because share <= userRewards[user], causing the transaction to revert with an InsufficientBalance error.

Code

The vulnerability is demonstrated in the following Foundry test suite. Convert to foundry project using the steps highlighted here. Then in the test/ folder create a Test file named FeeCollectorTest.t.sol and paste the test into it. Make sure the imports path are correct and run the test using forge test --mt testUnableClaimSubsequentReward :

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
​
import "forge-std/Test.sol";
import "contracts/core/tokens/RAACToken.sol";
import {IFeeCollector} from "contracts/interfaces/core/collectors/IFeeCollector.sol";
import {FeeCollector} from "contracts/core/collectors/FeeCollector.sol";
import {veRAACToken} from "contracts/core/tokens/veRAACToken.sol";
​
contract FeeCollectorTest is Test {
RAACToken public raacToken;
FeeCollector public feeCollector;
veRAACToken public veRaaCToken;
​
address owner;
address user1;
address user2;
address treasury;
address repairFund;
​
uint256 constant ONE_YEAR = 365 * 24 * 3600;
uint256 constant INITIAL_MINT = 10000 ether;
uint256 constant SWAP_TAX_RATE = 100; // 1%
uint256 constant BURN_TAX_RATE = 50; // 0.5%
​
IFeeCollector.FeeType defaultFeeType;
​
function setUp() public {
owner = address(this);
user1 = address(0x1);
user2 = address(0x2);
treasury = address(0x3);
repairFund = address(0x5);
​
raacToken = new RAACToken(owner, SWAP_TAX_RATE, BURN_TAX_RATE);
veRaaCToken = new veRAACToken(address(raacToken));
feeCollector = new FeeCollector(address(raacToken), address(veRaaCToken), treasury, repairFund, owner);
​
// Setup initial configuration
raacToken.setFeeCollector(address(feeCollector));
raacToken.manageWhitelist(address(feeCollector), true);
raacToken.manageWhitelist(address(veRaaCToken), true);
raacToken.setMinter(owner);
veRaaCToken.setMinter(owner);
​
// Setup roles
feeCollector.grantRole(feeCollector.FEE_MANAGER_ROLE(), owner);
feeCollector.grantRole(feeCollector.DISTRIBUTOR_ROLE(), owner);
​
// Mint initial tokens and approve
raacToken.mint(user1, INITIAL_MINT);
raacToken.mint(user2, INITIAL_MINT);
​
vm.prank(user1);
raacToken.approve(address(feeCollector), type(uint256).max);
vm.prank(user2);
raacToken.approve(address(feeCollector), type(uint256).max);
​
vm.prank(user1);
raacToken.approve(address(veRaaCToken), type(uint256).max);
vm.prank(user2);
raacToken.approve(address(veRaaCToken), type(uint256).max);
​
defaultFeeType = IFeeCollector.FeeType({
veRAACShare: 5000, // 50%
burnShare: 1000, // 10%
repairShare: 1000, // 10%
treasuryShare: 3000 // 30%
});
​
for (uint8 i = 0; i < 8; i++) {
feeCollector.updateFeeType(i, defaultFeeType);
}
​
// Create veRAACToken locks
vm.prank(user1);
veRaaCToken.lock(1000 ether, ONE_YEAR);
vm.prank(user2);
veRaaCToken.lock(1000 ether, ONE_YEAR);
vm.warp(block.timestamp + 1 weeks);
}
​
function testUnableClaimSubsequentReward() public {
// Amount to collect (100 RAAC)
uint256 amount = 100 ether;
​
// Collect fee
vm.prank(user1);
feeCollector.collectFee(amount, 0);
​
vm.prank(user2);
feeCollector.collectFee(amount, 1);
​
feeCollector.distributeCollectedFees();
feeCollector.claimRewards(user1);
​
vm.warp(block.timestamp + 8 days);
​
vm.prank(user1);
feeCollector.collectFee(amount, 0);
​
feeCollector.distributeCollectedFees();
​
// Expect this claim to fail
vm.expectRevert(IFeeCollector.InsufficientBalance.selector);
feeCollector.claimRewards(user1);
}
}
​

In this test:

  • The user claims rewards successfully the first time.

  • The user attempts to claim rewards again after additional fees are collected and distributed.

  • The second claim fails with an InsufficientBalance error due to the incorrect update of userRewards.

Impact

  • Loss of Rewards: Users lose access to their rightful rewards due to the incorrect implementation.

  • Protocol Inefficiency: The protocol's reward distribution mechanism becomes inefficient and unreliable.

Tools Used

  • Foundry: Used to write and execute the test suite that demonstrates the vulnerability.

  • Manual Review

Recommendations

  1. Fix Reward Accumulation:

    • Update the claimRewards function to correctly accumulate the pending rewards by adding them to the existing userRewards balance.

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; // Fix: Add pendingReward to existing balance
// Transfer rewards
raacToken.safeTransfer(user, pendingReward);
emit RewardClaimed(user, pendingReward);
return pendingReward;
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 7 months 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.

Give us feedback!