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

`FeeCollector` does not allow users to claim multiple times

hardlydifficult

Summary

FeeCollector.sol does not track a user's prior claims correctly, as a result users are unable to claim rewards a second time.

Finding Description

When calculating a user's pending rewards, the total amount that user has earned so far is calculated first. Then we subtract how many reward tokens the user has already claimed (userRewards[user]):

function _calculatePendingRewards(address user) internal view returns (uint256) {
...
uint256 share = (totalDistributed * userVotingPower) / totalVotingPower;
return share > userRewards[user] ? share - userRewards[user] : 0;
}

Source: FeeCollector.sol#L486-L487

The value returned is how many tokens will be sent to the user for the current claim:

function claimRewards(address user) external override nonReentrant whenNotPaused returns (uint256) {
...
uint256 pendingReward = _calculatePendingRewards(user);
...
raacToken.safeTransfer(user, pendingReward);
...
}

Source: FeeCollector.sol#L202-L209

The issue is when rewards are claimed, the total fees shared across all users (totalDistributed) is stored where we are expecting just the number of tokens this user has received:

function claimRewards(address user) external override nonReentrant whenNotPaused returns (uint256) {
...
userRewards[user] = totalDistributed;
...
}
​
function _processDistributions(uint256 totalFees, uint256[4] memory shares) internal {
...
totalDistributed += shares[0];
...
}

Source: FeeCollector.sol#L206 and FeeCollector.sol#L415

Next time that user attempts to claimRewards, the _calculatePendingRewards helper will return 0 (since share <= userRewards[user]) and the transaction will revert.

Impact Explanation

Users will only be rewarded fairly the very first time they call claimRewards.

Then the user is blocked from claiming additional rewards until a significant amount of new fees have accumulated (when the user's share is greater than all user rewards at the time of their last claim).

All rewards that should have accumulated for the user during that time are NOT claimable. But after this point in time, new rewards begin to accumulate again. Once the user claims, the issue repeats - preventing future claims.

Overall resulting in significantly less rewards for the user than expected.

Likelihood Explanation

Very likely. This impacts any user that claims rewards without fully exiting the protocol (e.g. by withdrawing from veRAACToken).

Proof of Concept

The following git patch updates the FeeCollector unit tests to show the impact (lost reward revenue), followed by a test showing how much reward revenue was expected. Run with npm run test:unit:collectors.

diff --git a/test/unit/core/collectors/FeeCollector.test.js b/test/unit/core/collectors/FeeCollector.test.js
index e823432..cf05288 100644
--- a/test/unit/core/collectors/FeeCollector.test.js
+++ b/test/unit/core/collectors/FeeCollector.test.js
@@ -91,6 +91,48 @@ describe("FeeCollector", function () {
await veRAACToken.connect(user2).lock(ethers.parseEther("500"), ONE_YEAR);
});
+ describe.only("Bug", () => {
+ beforeEach(async () => {
+ await feeCollector.connect(owner).distributeCollectedFees();
+ await time.increase(WEEK);
+ })
+
+ async function systemCollectsMoreFees() {
+ const amount = ethers.parseEther("0.005");
+ await raacToken.mint(owner.address, amount);
+ await raacToken.connect(owner).approve(feeCollector.target, amount);
+ await feeCollector.connect(owner).collectFee(amount, 0);
+
+ await feeCollector.connect(owner).distributeCollectedFees();
+ await time.increase(WEEK);
+ }
+
+ it("The issue: second claim fails", async function () {
+ // First a user claims for ~0.0032 RAACToken
+ const tx = await feeCollector.connect(user1).claimRewards(user1.address);
+ await expect(tx).to.changeTokenBalances(raacToken, [user1], (changes) => changes[0] > 0.0032e18 && changes[0] < 0.0033e18);
+
+ // Then the system collects more fees:
+ await systemCollectsMoreFees();
+
+ // BUG: The user is not able to claim the additional rewards as expected
+ await expect(feeCollector.connect(user1).claimRewards(user1.address)).to.be.revertedWithCustomError(feeCollector, "InsufficientBalance");
+
+ // The expected claim below shows that the user lost out on 0.0015 RAACToken in this scenario
+ });
+
+ it("Expected total claim", async function () {
+ // Skip the first claim
+
+ // Then the system collects more fees (same as the test above):
+ await systemCollectsMoreFees();
+
+ // The user claims for ~0.0047 RAACToken total
+ const tx = await feeCollector.connect(user1).claimRewards(user1.address);
+ await expect(tx).to.changeTokenBalances(raacToken, [user1], (changes) => changes[0] > 0.0047e18 && changes[0] < 0.0048e18);
+ });
+ })
+
describe("Constructor & Initial Setup", function () {
it("should set the correct initial values", async function () {
expect(await feeCollector.raacToken()).to.equal(await raacToken.getAddress());

Recommendation

This patch updates the tracker to use the some of all safeTransfer calls to that user:

When applied, the POC test above fails (since the it no longer reverts).

diff --git a/contracts/core/collectors/FeeCollector.sol b/contracts/core/collectors/FeeCollector.sol
index 4e93083..c8b5cd9 100644
--- a/contracts/core/collectors/FeeCollector.sol
+++ b/contracts/core/collectors/FeeCollector.sol
@@ -203,7 +203,7 @@ contract FeeCollector is IFeeCollector, AccessControl, ReentrancyGuard, Pausable
if (pendingReward == 0) revert InsufficientBalance();
// Reset user rewards before transfer
- userRewards[user] = totalDistributed;
+ userRewards[user] += pendingReward;
// Transfer rewards
raacToken.safeTransfer(user, pendingReward);
Updates

Lead Judging Commences

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