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

Division before multiplication in `FeeCollector::_calculateDistribution` results in precision loss in fee distribution

kiteweb3

Summary

In FeeCollector::_calculateDistribution function, the code performs division before multiplication when calculating weights and shares. This order of operations can lead to truncation and precision loss due to Solidity's integer division behavior that results in wrong fee distribution calculations and unfair distribution of fees.

Vulnerability Details

function _calculateDistribution(uint256 totalFees) internal view returns (uint256[4] memory shares) {
uint256 totalCollected;
​
for (uint8 i = 0; i < 8; i++) {
uint256 feeAmount = _getFeeAmountByType(i);
if (feeAmount == 0) continue;
​
FeeType memory feeType = feeTypes[i];
totalCollected += feeAmount;
@> uint256 weight = (feeAmount * BASIS_POINTS) / totalFees;
@> shares[0] += (weight * feeType.veRAACShare) / BASIS_POINTS;
@> shares[1] += (weight * feeType.burnShare) / BASIS_POINTS;
@> shares[2] += (weight * feeType.repairShare) / BASIS_POINTS;
@> shares[3] += (weight * feeType.treasuryShare) / BASIS_POINTS;
}
​
if (totalCollected != totalFees) revert InvalidFeeAmount();
​
shares[0] = (totalFees * shares[0]) / BASIS_POINTS;
shares[1] = (totalFees * shares[1]) / BASIS_POINTS;
shares[2] = (totalFees * shares[2]) / BASIS_POINTS;
shares[3] = (totalFees * shares[3]) / BASIS_POINTS;
​
uint256 remainder = totalFees - (shares[0] + shares[1] + shares[2] + shares[3]);
if (remainder > 0) shares[3] += remainder;
}

Impact

Add Foundry to the project using this procedure

Create a file named FeeCollector.t.sol and copy / paste this:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;
​
import {Test, console2} from "forge-std/Test.sol";
​
import {FeeCollector} from "../contracts/core/collectors/FeeCollector.sol";
import {RAACToken} from "../contracts/core/tokens/RAACToken.sol";
import {veRAACToken} from "../contracts/core/tokens/veRAACToken.sol";
​
contract FeeCollectorTest is Test {
FeeCollector public feeCollector;
RAACToken public raacToken;
veRAACToken public veRaacToken;
​
address owner = makeAddr("owner");
address treasury = makeAddr("treasury");
address repairFund = makeAddr("repairFund");
​
uint256 SWAP_TAX_RATE = 100; // 1%
uint256 BURN_TAX_RATE = 50; // 0.5%
​
function setUp() public {
raacToken = new RAACToken(owner, SWAP_TAX_RATE, BURN_TAX_RATE);
veRaacToken = new veRAACToken(address(raacToken));
feeCollector = new FeeCollector(address(raacToken), address(veRaacToken), treasury, repairFund, owner);
​
console2.log("raacToken: ", address(raacToken));
console2.log("veRaacToken: ", address(veRaacToken));
console2.log("feeCollector: ", address(feeCollector));
}
​
function testFuzz_precisionLossInDistribution(uint256 largeTotalFees) public {
largeTotalFees = bound(largeTotalFees, 1000 * 1e18, 10000 * 1e18);
​
uint256 smallFeeAmount = 100 * 1e18;
uint256 BASIS_POINTS = 10000;
uint256 veRAACShare = 8000;
​
vm.startPrank(owner);
​
// First calculate using the current implementation's approach
uint256 currentWeight = (smallFeeAmount * BASIS_POINTS) / largeTotalFees;
uint256 currentVeRAACShare = (currentWeight * veRAACShare) / BASIS_POINTS;
​
// Now calculate the correct way (multiplication before division)
uint256 correctVeRAACShare = ((smallFeeAmount * BASIS_POINTS * veRAACShare) / largeTotalFees) / BASIS_POINTS;
​
// Compare results
assertEq(currentVeRAACShare, correctVeRAACShare, "Precision loss should be demonstrated");
​
console2.log("Current implementation result:", currentVeRAACShare);
console2.log("Correct calculation result:", correctVeRAACShare);
​
vm.stopPrank();
}
​
function test_precisionLossInDistributionCheck() public {
uint256 smallFeeAmount = 1e20;
uint256 largeTotalFees = 7606554941953871744457;
uint256 BASIS_POINTS = 10000;
uint256 veRAACShare = 8000;
​
vm.startPrank(owner);
​
// First calculate using the current implementation's approach
uint256 currentWeight = (smallFeeAmount * BASIS_POINTS) / largeTotalFees;
uint256 currentVeRAACShare = (currentWeight * veRAACShare) / BASIS_POINTS;
​
// Now calculate the correct way (multiplication before division)
uint256 correctVeRAACShare = ((smallFeeAmount * BASIS_POINTS * veRAACShare) / largeTotalFees) / BASIS_POINTS;
​
// Compare results
assertNotEq(currentVeRAACShare, correctVeRAACShare, "Precision loss should be demonstrated");
​
// Log the differences
console2.log("Small fee amount:", smallFeeAmount);
console2.log("Large fee amount:", largeTotalFees);
console2.log("Current implementation result:", currentVeRAACShare);
console2.log("Correct calculation result:", correctVeRAACShare);
​
vm.stopPrank();
}
}

Run forge test --match-test testFuzz_precisionLossInDistribution -vv

Logs:
raacToken: 0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
veRaacToken: 0x2e234DAe75C793f67A35089C9d99245E1C58470b
feeCollector: 0xF62849F9A0B5Bf2913b396098F7c7019b51A820a
Bound result 7606554941953871744457
​
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 16.44ms (6.96ms CPU time)
​
Ran 1 test suite in 189.88ms (16.44ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)
​
Failing tests:
Encountered 1 failing test in test/FeeCollector.t.sol:FeeCollectorTest
[FAIL: Precision loss should be demonstrated: 104 != 105; counterexample: calldata=0x2f8273cb0000000000000001ca1f27d1773513b94171aea0745adeb2bf83183ae9a86460 args=[11233111431342404087002701263833437960838443992302798529632 [1.123e58]]] testFuzz_precisionLossInDistribution(uint256) (runs: 4, μ: 12241, ~: 12241)

The test fails. For largeTotalFees = 7606554941953871744457 the calculated shares are different.

We can prove this running the other test: forge test --match-test test_precisionLossInDistributionCheck -vv

Logs:
raacToken: 0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
veRaacToken: 0x2e234DAe75C793f67A35089C9d99245E1C58470b
feeCollector: 0xF62849F9A0B5Bf2913b396098F7c7019b51A820a
Small fee amount: 100000000000000000000
Large fee amount: 7606554941953871744457
Current implementation result: 104
Correct calculation result: 105
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.60ms (1.20ms CPU time)

The test shows a precision loss for the current implementation that performs a division before a multiplication.

Tools Used

Manual review

Recommendations

Rewrite the calculations to perform multiplication before division.

function _calculateDistribution(uint256 totalFees) internal view returns (uint256[4] memory shares) {
uint256 totalCollected;
​
for (uint8 i = 0; i < 8; i++) {
uint256 feeAmount = _getFeeAmountByType(i);
if (feeAmount == 0) continue;
​
FeeType memory feeType = feeTypes[i];
totalCollected += feeAmount;
- uint256 weight = (feeAmount * BASIS_POINTS) / totalFees;
- shares[0] += (weight * feeType.veRAACShare) / BASIS_POINTS;
- shares[1] += (weight * feeType.burnShare) / BASIS_POINTS;
- shares[2] += (weight * feeType.repairShare) / BASIS_POINTS;
- shares[3] += (weight * feeType.treasuryShare) / BASIS_POINTS;
​
+ shares[0] += (feeAmount * feeType.veRAACShare) / totalFees;
+ shares[1] += (feeAmount * feeType.veRAACShare) / totalFees;
+ shares[2] += (feeAmount * feeType.veRAACShare) / totalFees;
+ shares[3] +=(feeAmount * feeType.veRAACShare) / totalFees;
}
​
if (totalCollected != totalFees) revert InvalidFeeAmount();
​
shares[0] = (totalFees * shares[0]) / BASIS_POINTS;
shares[1] = (totalFees * shares[1]) / BASIS_POINTS;
shares[2] = (totalFees * shares[2]) / BASIS_POINTS;
shares[3] = (totalFees * shares[3]) / BASIS_POINTS;
​
uint256 remainder = totalFees - (shares[0] + shares[1] + shares[2] + shares[3]);
if (remainder > 0) shares[3] += remainder;
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 4 months ago
Submission Judgement Published
Validated
Assigned finding tags:

FeeCollector distributes too much to treasury when fee amounts are small relative to total due to precision loss in (feeAmount * BASIS_POINTS) / totalFees

inallhonesty Lead Judge 4 months ago
Submission Judgement Published
Validated
Assigned finding tags:

FeeCollector distributes too much to treasury when fee amounts are small relative to total due to precision loss in (feeAmount * BASIS_POINTS) / totalFees

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.