Summary
In the FeeCollector::claimRewards function, the contract updates userRewards[user] to the value of totalDistributed. Because totalDistributed is the global total number of tokens that have been distributed historically to all users rather than a particular user’s cumulative claimed balance, subsequent claims will result in pendingReward being calculated as zero. In other words, if a user claims once, they will no longer receive any share for newly added rewards.
Vulnerability Details
Within the claimRewards function, after computing the user’s pro-rata share of totalDistributed, the contract erroneously updates the userRewards[user] mapping and sets it to be equal to totalDistributed instead of the actual number of tokens that the user is entitled to claim (pendingReward).
function claimRewards(address user) external override nonReentrant whenNotPaused returns (uint256) {
uint256 pendingReward = _calculatePendingRewards(user);
if (pendingReward == 0) revert InsufficientBalance();
userRewards[user] = totalDistributed;
}
This effectively stores the system-wide total instead of tracking the user’s total claimed amount. During the next distribution, the _calculatePendingRewards function subtracts userRewards[user] from the user’s new pro-rata share. Because userRewards[user] is set to a value higher than the user’s actual claimed tokens, the result is zero or a negative difference (which floors to zero). The user is therefore blocked from claiming further distributions.
function _calculatePendingRewards(address user) internal view returns (uint256) {
return share > userRewards[user] ? share - userRewards[user] : 0;
}
Impact
Loss of funds for users. Affected users will not be able to claim their future token rewards from subsequent distributions, effectively losing tokens by never receiving newly allocated rewards.
PoC
Put the following test inside the FeeCollector.test.js file.
it.only("should prove flawed claimedRewards logic", async function () {
const SWAP_TAX_RATE = 100;
const BURN_TAX_RATE = 50;
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);
const lendingFeeGross =
(ethers.parseEther("30") * grossMultiplier) / BigInt(10000);
const swapTaxGross =
(ethers.parseEther("20") * grossMultiplier) / BigInt(10000);
await feeCollector.connect(user1).collectFee(protocolFeeGross, 0);
await feeCollector.connect(user1).collectFee(lendingFeeGross, 1);
await feeCollector.connect(user1).collectFee(swapTaxGross, 6);
await feeCollector.connect(user1).collectFee(protocolFeeGross, 0);
await feeCollector.connect(user1).collectFee(lendingFeeGross, 1);
await feeCollector.connect(user1).collectFee(swapTaxGross, 6);
await feeCollector.connect(owner).distributeCollectedFees();
await time.increase(WEEK);
const initialBalance = await raacToken.balanceOf(user1.address);
const userRewardsBefore = await feeCollector.userRewards(user1.address);
console.log("Balance of user before claim = ", initialBalance);
console.log("userRewards before first claim = ", userRewardsBefore);
await feeCollector.connect(user1).claimRewards(user1.address);
const balanceAfterClaim = await raacToken.balanceOf(user1.address);
const userRewardsAfterClaim1 = await feeCollector.userRewards(
user1.address
);
console.log("Balance of user after claim 1 = ", balanceAfterClaim);
console.log("userRewards after first claim = ", userRewardsAfterClaim1);
expect(await raacToken.balanceOf(user1.address)).to.be.gt(initialBalance);
await feeCollector.connect(user1).collectFee(protocolFeeGross, 0);
await feeCollector.connect(user1).collectFee(lendingFeeGross, 1);
await feeCollector.connect(user1).collectFee(swapTaxGross, 6);
await feeCollector.connect(owner).distributeCollectedFees();
await time.increase(WEEK);
await expect(feeCollector.connect(user1).claimRewards(user1.address)).to
.be.reverted;
});
});
Test output
npm run test:unit:collectors
> @raac/core@1.0.0 test:unit:collectors
> npx hardhat test test/unit/core/collectors
FeeCollector
Fee Collection and Distribution
Balance of user before claim = 8999970000000000000000n
userRewards before first claim = 0n
Balance of user after claim 1 = 8999979616438356164390n
userRewards after first claim = 15000000000000000n
✔ should prove flawed claimedRewards logic (65ms)
1 passing (2s)
You can see that userRewards = 15000000000000000 while the actual number of claimed tokens was 9616438356164390.
The difference is an extra ~5383561600000000 that the protocol stores as claimed by the user, although they were not claimed.
Tools Used
Manual review
Recommended Mitigation
Track the cumulative tokens the user has already claimed. The simplest fix is to increment the user’s claimed balance by the newly claimed token amount.
function claimRewards(address user) external override nonReentrant whenNotPaused returns (uint256) {
//..
uint256 pendingReward = _calculatePendingRewards(user);
if (pendingReward == 0) revert InsufficientBalance();
// Reset user rewards before transfer
- userRewards[user] = totalDistributed;
+ userRewards[user] += pendingReward ;
//..
//..
}
Like this, the mapping should properly track the rewards claimed by each user individually.
