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

RAAC Token fees on transfer do not use FeeCollector collectFee, therefore bypassing accounting logic

0xtimefliez

Description

The RAAC Token is designed to be able to collect fees and burn a certain amount of transferred assets. The collected fees from every transaction, if taxes are enabled within the contract, will transfer the funds directly into the FeeCollector contract. The issue arising here is, that the contract lacks functionality to effectively deal with directly transferred funds.

Vulnerability Details

As you can see in the following RAACToken::_update function:

function _update(
address from,
address to,
uint256 amount
) internal virtual override {
uint256 baseTax = swapTaxRate + burnTaxRate;
if (baseTax == 0 || from == address(0) || to == address(0) || whitelistAddress[from] || whitelistAddress[to] || feeCollector == address(0)) {
super._update(from, to, amount);
return;
}
uint256 totalTax = amount.percentMul(baseTax);
uint256 burnAmount = totalTax * burnTaxRate / baseTax;
@> super._update(from, feeCollector, totalTax - burnAmount);
super._update(from, address(0), burnAmount);
super._update(from, to, amount - totalTax);
}

In the case of a transfer between users, while taxes are enabled and a fee collector is set, the update function will directly transfer the funds into the FeeCollector contract.
Looking now into the collector contract:

function collectFee(uint256 amount, uint8 feeType) external override nonReentrant whenNotPaused returns (bool) {
if (amount == 0 || amount > MAX_FEE_AMOUNT) revert InvalidFeeAmount();
if (feeType > 7) revert InvalidFeeType();
raacToken.safeTransferFrom(msg.sender, address(this), amount);
@> _updateCollectedFees(amount, feeType);
emit FeeCollected(feeType, amount);
return true;
}

we can already see within this function, that the RAAC Token is not using the intended functionality to deposit the tokens into the Fee Collector.

Going a bit further down within the contract we will than see the following:

function distributeCollectedFees() external override nonReentrant whenNotPaused {
if (!hasRole(DISTRIBUTOR_ROLE, msg.sender)) revert UnauthorizedCaller();
1.@> uint256 totalFees = _calculateTotalFees();
if (totalFees == 0) revert InsufficientBalance();
uint256[4] memory shares = _calculateDistribution(totalFees);
2.@> _processDistributions(totalFees, shares);
delete collectedFees;
emit FeeDistributed(shares[0], shares[1], shares[2], shares[3]);
}
​
/// *** SNIP *** ///
​
function _calculateTotalFees() internal view returns (uint256) {
1.2@> return collectedFees.protocolFees +
collectedFees.lendingFees +
collectedFees.performanceFees +
collectedFees.insuranceFees +
collectedFees.mintRedeemFees +
collectedFees.vaultFees +
collectedFees.swapTaxes +
collectedFees.nftRoyalties;
}
​

The total amount of fees collected is reliant on FeeCollector::_updateCollectedFees written by the FeeCollector::collectFee function. The total amount for distribution is calculated within FeeCollector::_calculateTotalFees which simply returns state being written by the collectFee function. The total amount calculated here will directly be passed into FeeCollector::_processDistributions. Therefore the contract lacks the functionality to effectively deal with directly transferred RAAC Tokens, since they are bypassing important state updates to effectively be distributed.

Impact

A certainly mitigating factor for this is though, that the FeeCollector possesses a rescue function, so funds, which would be send into the contract bypassing the accounting are not lost, however it requires manual intervention from the admin side to free those funds. Furthermore those funds are not accounted for within the type of fee they are meant for and this will still be an impact, since it is in the implementation as it is, impossible to tell in which category of fees funds fall, which did not go through the accounting. As an example, those fees could easily be mistaken as donations and easily sent into the wrong fee bucket leaving potentially holes in the balance somewhere else.
Also, the emergency withdraw can only send RAAC Tokens into the treasury and on top of that, would remove the whole balanceOf() from the contract, it should pretty much only be used sparely.

The likelihood rating for me is a clear high, since it happens during normal operations, while the impact is a strong low, funds aren't lost, but almost certainly misrouted and would require an emergency withdraw. Therefore I conclude the total severity as a Medium in this case.

Tools Used

Manual Review

Recommended Mitigation

Use collectFee within the RAAC Token Contract to deposit the fees into the Fee Collector like so:

+import {IFeeCollector} from "../../interfaces/core/collectors/IFeeCollector.sol";
function _update(
address from,
address to,
uint256 amount
) internal virtual override {
uint256 baseTax = swapTaxRate + burnTaxRate;
// Skip tax for whitelisted addresses or when fee collector disabled
if (baseTax == 0 || from == address(0) || to == address(0) || whitelistAddress[from] || whitelistAddress[to] || feeCollector == address(0)) {
super._update(from, to, amount);
return;
}
// All other cases where tax is applied
uint256 totalTax = amount.percentMul(baseTax);
uint256 burnAmount = totalTax * burnTaxRate / baseTax;
- super._update(from, feeCollector, totalTax - burnAmount);
super._update(from, address(0), burnAmount);
super._update(from, to, amount - totalTax);
+ uint256 feeCollectorAmount = totalTax - burnAmount;
+ if(feeCollectorAmount > 0){
+ require(whitelistAddress[feeCollector], "Fee collector is not whitelisted"); // Important to avoid an infinite loop
+ super._update(from, address(this), feeCollectorAmount);
+ approve(feeCollector, feeCollectorAmount);
+ IFeeCollector(feeCollector).collectFee(feeCollectorAmount, 0);
+ }
}

If the above solution seems suitable, please don't forget to tie the whitelisting of the Fee Collector

a) into the constructor

b) enable/disable events

otherwise funds will not be transferable, but every transaction would revert with an OOG.

Alternative solution would be a nested if-else-block.

Updates

Lead Judging Commences

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

RAACToken::burn sends tax directly to FeeCollector without using collectFee(), causing tokens to bypass accounting and remain undistributed. `collectFee` is not used anywhere.

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

RAACToken::burn sends tax directly to FeeCollector without using collectFee(), causing tokens to bypass accounting and remain undistributed. `collectFee` is not used anywhere.

Support

FAQs

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