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 6 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 6 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 5 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.