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

`RAACToken::feeCollector` receives less fee due to incorrect accounting in `RAACToken::_update`

0xlouistsai

Summary

The RAACToken::_update function does not correctly account for burn tax distribution, leading to the feeCollector receiving a lower-than-expected amount. The issue arises due to incorrect handling of tax calculations and improper sequencing of _update calls.

Vulnerability Details

According to documenation:

Implements a burn function with a separate burn tax mechanism

The RAACToken::burn function is expected to apply a burn tax based on the user’s desired amount multiplied by the burnTaxRate. However, the way _update processes the tax distribution causes the feeCollector to receive less than the expected burn tax amount.

The RAACToken::burn function is expected to apply a burn tax based on the user’s desired amount multiplied by the burnTaxRate. However, the way _update processes the tax distribution causes the feeCollector to receive less than the expected burn tax amount.

Consider the following scenario:

  1. A user holds 100e18 RAAC tokens and intends to burn them.

  2. The burnTaxRate is set to 0.5% (default value).

  3. According to expectations, the feeCollector should receive 100e18 * 0.5% = 5e17 RAAC tokens

  4. The RAACToken::burn function calls _transfer(msg.sender, feeCollector, taxAmount). This invokes _update(from, to, amount) with (user, feeCollector, taxAmount).

  5. Inside _update, the if-condition fails to check to == feeCollector, leading to an additional tax application:

  6. This results in:

    • Additional amount * burnTaxRate being burned to the zero address.

    • Only the remaining amount being sent to feeCollector, reducing its expected fee.

Below is a test case demonstrating the issue:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;
​
import {Test, console} from "forge-std/Test.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "src/core/tokens/RAACToken.sol";
​
contract RAACTokenDoubleBurnFeeTest is Test {
RAACToken internal raac;
​
address internal minter;
address internal user;
address internal feeCollector;
​
function setUp() public {
address initialOwner = address(this);
uint256 initialSwapTaxRate = 0; // No swap tax for this test
uint256 initialBurnTaxRate = 50; // 0.5% burn tax rate
raac = new RAACToken(initialOwner, initialSwapTaxRate, initialBurnTaxRate);
​
minter = makeAddr("minter");
feeCollector = makeAddr("feeCollector");
user = makeAddr("user");
​
raac.setMinter(minter);
raac.setFeeCollector(feeCollector);
​
// Mint tokens to the user
vm.prank(minter);
raac.mint(user, 100e18);
}
​
function testExploit() public {
// User attempts to burn 100 RAAC tokens
vm.prank(user);
raac.burn(100e18);
​
// Expected fee received by the feeCollector
console.log(raac.burnTaxRate() * 100e18 / 1e4); // Expected: 500000000000000000
// Actual fee received by the feeCollector
console.log(raac.balanceOf(feeCollector)); // Actual: 497500000000000000
}
}

Problematic code section: https://github.com/Cyfrin/2025-02-raac/blob/89ccb062e2b175374d40d824263a4c0b601bcb7f/contracts/core/tokens/RAACToken.sol#L185

function _update(
address from,
address to,
uint256 amount
) internal virtual override {
uint256 baseTax = swapTaxRate + burnTaxRate;
// @audit-issue Do not consider the to == feeCollector condition
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); // swap tax
super._update(from, address(0), burnAmount); // burn tax
super._update(from, to, amount - totalTax); // actual receive
}

Impact

  • FeeCollector Receives Less Fees: The incorrect tax accounting results in the feeCollector receiving fewer tokens than expected.

  • Incorrect Burn Mechanism: The function unintentionally redistributes the burn tax incorrectly, deviating from expected behavior.

Tools Used

Manual Review

Recommendations

To fix this issue, update _update to correctly check when to == feeCollector and prevent double taxation:

function _update(
address from,
address to,
uint256 amount
) internal virtual override {
uint256 baseTax = swapTaxRate + burnTaxRate;
// Ensure feeCollector is not taxed again
- if (baseTax == 0 || from == address(0) || to == address(0) || whitelistAddress[from] || whitelistAddress[to] || feeCollector == address(0)) {
+ if (to == feeCollector || 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); // swap tax
super._update(from, address(0), burnAmount); // burn tax
super._update(from, to, amount - totalTax); // actual receive
}

By implementing this fix, the feeCollector will receive the expected burn tax, ensuring proper accounting and preventing unintended losses.

Updates

Lead Judging Commences

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

RAACToken::burn applies burn tax twice when transferring to feeCollector, causing excess tokens to be burned and reduced fees to be collected

This is by design, sponsor's words: Yes, burnt amount, done by whitelisted contract or not always occur the tax. The feeCollector is intended to always be whitelisted and the address(0) is included in the _transfer as a bypass of the tax amount, so upon burn->_burn->_update it would have not applied (and would also do another burn...). For this reason, to always apply such tax, the burn function include the calculation (the 2 lines that applies) and a direct transfer to feeCollector a little bit later. This is done purposefully

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

RAACToken::burn applies burn tax twice when transferring to feeCollector, causing excess tokens to be burned and reduced fees to be collected

This is by design, sponsor's words: Yes, burnt amount, done by whitelisted contract or not always occur the tax. The feeCollector is intended to always be whitelisted and the address(0) is included in the _transfer as a bypass of the tax amount, so upon burn->_burn->_update it would have not applied (and would also do another burn...). For this reason, to always apply such tax, the burn function include the calculation (the 2 lines that applies) and a direct transfer to feeCollector a little bit later. This is done purposefully

Appeal created

inallhonesty Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

RAACToken::burn applies burn tax twice when transferring to feeCollector, causing excess tokens to be burned and reduced fees to be collected

This is by design, sponsor's words: Yes, burnt amount, done by whitelisted contract or not always occur the tax. The feeCollector is intended to always be whitelisted and the address(0) is included in the _transfer as a bypass of the tax amount, so upon burn->_burn->_update it would have not applied (and would also do another burn...). For this reason, to always apply such tax, the burn function include the calculation (the 2 lines that applies) and a direct transfer to feeCollector a little bit later. This is done purposefully

Support

FAQs

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

Give us feedback!