Summary
The FeeCollector::distributeCollectedFees, FeeCollector::_processDistributions violate the CEI pattern.
While the functions is protected with nonReentrant, following CEI remains a best practice to prevent unforeseen vulnerabilities.
Vulnerability Details
The CEI pattern dictates that a contract should:
However, in FeeCollector::distributeCollectedFees, the external FeeCollector::_processDistributions happens before updating delete collectedFees:
function distributeCollectedFees() external override nonReentrant whenNotPaused {
if (!hasRole(DISTRIBUTOR_ROLE, msg.sender)) revert UnauthorizedCaller();
uint256 totalFees = _calculateTotalFees();
if (totalFees == 0) revert InsufficientBalance();
uint256[4] memory shares = _calculateDistribution(totalFees);
_processDistributions(totalFees, shares);
delete collectedFees;
emit FeeDistributed(shares[0], shares[1], shares[2], shares[3]);
}
And in FeeCollector::_processDistributions, the external TimeWeightedAverage.createPeriod happens before updating totalDistributed += shares[0]:
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]);
}
https://github.com/Cyfrin/2025-02-raac/blob/89ccb062e2b175374d40d824263a4c0b601bcb7f/contracts/core/collectors/FeeCollector.sol#L180-L192
https://github.com/Cyfrin/2025-02-raac/blob/89ccb062e2b175374d40d824263a4c0b601bcb7f/contracts/core/collectors/FeeCollector.sol#L401-L424
Impact
Deviation from best practices, which could lead to overlooked vulnerabilities.
Tools Used
Manual
Recommendations
Reorder the function logic to update the contract’s state before interacting with external contracts:
function distributeCollectedFees() external override nonReentrant whenNotPaused {
if (!hasRole(DISTRIBUTOR_ROLE, msg.sender)) revert UnauthorizedCaller();
uint256 totalFees = _calculateTotalFees();
if (totalFees == 0) revert InsufficientBalance();
uint256[4] memory shares = _calculateDistribution(totalFees);
// Effects (State Updates)
+ delete collectedFees;
// Interactions (External Calls)
_processDistributions(totalFees, shares);
- delete collectedFees;
emit FeeDistributed(shares[0], shares[1], shares[2], shares[3]);
}
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) {
+ totalDistributed += shares[0];
TimeWeightedAverage.createPeriod(
distributionPeriod,
block.timestamp + 1,
7 days,
shares[0],
totalVeRAACSupply
);
- totalDistributed += shares[0];
} else {
shares[3] += shares[0]; // Add to treasury if no veRAAC holders
}
}
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]);
}
