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

`_calculateDistribution()` distributes too much to treasury due to rounding error

charlescheerful

Summary

At FeeColletor::_calculateDistribution() there can be rounding errors to 0. Leading to some of the rewards receivers not receiving them.

Vulnerability Details

At _calculateDistribution(), if the fees collected from one action are 10_000 times smaller that the total fees collected, the treasury will get 100% of those fees due to rounding error. This means, imagine that from feeType == 1 you accrue 100 tokens, and in total, including the rest of feeTypes you accrue 1_000_000 tokens. The treasury will get the 100 tokens that were meant to distributed to different actors.

All value set on the remainder goes to treasury because it is accounted to in shares[3] which is the value later transferred to treasury on _processDistributions():

function distributeCollectedFees() external override nonReentrant whenNotPaused {
// more code...
@> 👁️ When distritbuting this functions are called to allocate the fees
@> uint256[4] memory shares = _calculateDistribution(totalFees);
@> _processDistributions(totalFees, shares);
delete collectedFees;
emit FeeDistributed(shares[0], shares[1], shares[2], shares[3]);
}
​
function _calculateDistribution(uint256 totalFees) internal view returns (uint256[4] memory shares) {
// more codee...
​
@> // 👁️ Iterate through the different 7 feeTypes
for (uint8 i = 0; i < 8; i++) {
uint256 feeAmount = _getFeeAmountByType(i);
if (feeAmount == 0) continue;
​
FeeType memory feeType = feeTypes[i];
totalCollected += feeAmount;
@> // 👁️ Here, if `feeAmount` is BASIS_POINTS (10K), smaller than `totalFees`, it rounds down
@> // Because it is rounded down, it is not added to shares, as weight is 0 and multiplying.
@> // This means that the amount will end up on `reminder` due to logic at the end of the func.
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;
}
​
@> // 👁️ Here shares[] is converted to token units instead of %
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;
​
@> // 👁️ Here is the `remainder` logic, if there is anything left, it goes to treasury.
uint256 remainder = totalFees - (shares[0] + shares[1] + shares[2] + shares[3]);
@> if (remainder > 0) shares[3] += remainder;
}
​
function _processDistributions(uint256 totalFees, uint256[4] memory shares) internal {
// more codee...
@> // 👁️ Index 3 goes to treasury
@> if (shares[3] > 0) raacToken.safeTransfer(treasury, shares[3]);
}

Impact

Treasury can and will get more fees than it should. This is unfair as it leads to some fee receivers, like veRAAC holders, receiving less fees than they should.

POC

You can play with the following code on RemixIDE to see that the logic does the described behavior.

  • Input 1: _calculateDistribution(100e18, 1000000e18) -> Round down to 0, all would go to treasury.

This is a simplified version of the original function, here.
Yet the logic is exactly the same, just deleted the for loop and focused on 1 feetype logic.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
contract DistributionTest {
function _calculateDistribution(uint256 feeAmountOneType, uint256 totalFees) public pure returns (uint256,uint256,uint256) {
uint256 BASIS_POINTS = 10_000;
uint256 weight = (feeAmountOneType * BASIS_POINTS) / totalFees;
// Let's imagine a 20% realistic value as can bee see in on _initializeFeeTypes()
uint256 shares = (weight * 2_000) / BASIS_POINTS;
uint256 amount = totalFees * shares / BASIS_POINTS;
uint256 remainder = totalFees - amount;
return(shares, amount, remainder);
}
}

Recommendations

Increase the precision of the BASIS_POINTS. You can use RAYs as done in other parts of the code, these have 27 decimals and that is quite a lot of precision for token amounts arising from fees.

If you increase the precision the rounding to 0 error will disappear, as the rounding will be more precise. Making everyone receive their fair share.

Updates

Lead Judging Commences

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