Issues with Tax & Burn in OpenZepplin V5.0 ERC20.sol

I have created an ERC20 token using OpenZepplin 5.0. We have reflection and burn features enabled in the token.

The ERC20.sol suggests:

  1. Include any tax/ burn logic in _transfer.
    We don't want to touch the ERC20.sol of OZ. However since the function cannot be overridden in our token contract, we only are left with option to add it in ERC20.sol

  2. _Update should be overridden for adding any extra logic. Following is our code. We observed that when burn is called in this function, the _update is called again. Due to this we cannot add nonreentrant keyword to the method as the process will fail.

  3. We had to add following since the burn was going in a recursive call till the burn fee is zero by calling _update in a loop.

 if (recipient == address(0)) 
        {
            // This is a burn operation, handle accordingly
            super._transfer(sender, recipient, amount);
        } 

Is there any better way of doing this?

  1. Above code works perfectly fine as expected. However when we run the tests several tests fail unless the _update function has only super._update. Even a single require method before super call in _update will result in test case failures.

Pls help with above.

:1234: The code for the function is below...

 function _update(address sender, address recipient, uint256 amount)
        internal override(ERC20, ERC20Pausable)   
        notBlacklisted(sender) notBlacklisted(recipient) whenNotPaused {
        require(amount > 0, "KSCxToken: Transfer amount must be greater than zero");
        if (recipient == address(0)) 
        {
            // This is a burn operation, handle accordingly
            super._transfer(sender, recipient, amount);
        } 
        else 
        {
            //AntiWhale
            if (recipient != owner() && recipient != address(0)) 
            {
                uint256 maxTransferAmount = (totalSupply() * maxTransferAmountRate) / MAX_RATE;
                require(amount <= maxTransferAmount, "KSCxToken: Transfer amount exceeds the maxTransferAmount");

                uint256 recipientBalance = balanceOf(recipient);
                uint256 maxWalletBalance = (totalSupply() * maxWalletBalanceRate) / MAX_RATE;
                require(recipientBalance + amount <= maxWalletBalance, "KSCxToken: Exceeds maximum wallet token balance");
            }
            //Tax Fee & Burn
            if (sender == owner() || recipient == owner() || 
            isExcludedFromFeeAndBurn[sender] || 
            isExcludedFromFeeAndBurn[recipient] ||
            recipient == feeDestination) 
            {
                super._transfer(sender, recipient, amount); // Transfer without fee or burn
            } 
            else 
            {
                // Calculate fee and burn amounts
                uint256 feeAmount = (amount * transactionFee * 10**decimals()) / (100 * 10**decimals());
                uint256 burnAmount = (amount * burnRate * 10**decimals()) / (100 * 10**decimals());
                uint256 amountAfterFeeAndBurn = amount - feeAmount - burnAmount;
            
                if(amountAfterFeeAndBurn > 0){
                    super._transfer(sender, recipient, amountAfterFeeAndBurn);// Transfer the net amount
                }
            
                if (feeAmount > 0) {
                    super._transfer(sender, feeDestination, feeAmount); // Ensure this uses _transfer not _update to avoid recursion
                }
            
                if (burnAmount > 0) {
                     _burn(sender, burnAmount); // Burn the specified amount
                }
            }
        }

:computer: Environment

Remix, Solidity 0.8.20, OZ V5.0 libraries, bsc test chain, ERC20 Burnable Pausable

Your implementation is a total chaos, but here are a few points:


Point 1:

Other than in the constructor, you should never use 10**decimals() inside your contract's code.
All amounts should always be denoted in the full-resolution (aka "wei units") of the associated token.

In your specific case:

uint256 feeAmount = (amount * transactionFee * 10**decimals()) / (100 * 10**decimals());
uint256 burnAmount = (amount * burnRate * 10**decimals()) / (100 * 10**decimals());

That expression is actually cancelled out, as it appears on both the numerator and the denominator.
However, it may still yield an overflow during intermediate calculations, resulting in the entire transaction being reverted without a "good" reason (due to the cancel-out issue explained above).

In any case, it is the offchain's responsibility (app, website, script, etc) to convert every amount:

  1. From token-resolution to wei-resolution before passing it as input from the user to the contract
  2. From wei-resolution to token-resolution before passing it as output from the contract to the user

In short, the decimals attribute of a token should be used almost exclusively for offchain readability, since it is rather difficult to conceive, for example, the magnitude of a 25-digit number.


Point 2:

Implementing abstract contract ERC20Pausable is ERC20, Pausable looks completely redundant.

You may as well just declare your contract to inherit ERC20 and Pausable, and then avoid stuff like override(ERC20,ERC20Pausable).

This is mostly a semantic issue of course, since you've already figured out that you should use this override in order to avoid the compilation error.

But why complicate things if you can make them simpler?


Point 3:

In your 3rd bullet:

It is not really clear what makes you think that super._transfer doesn't yield the same recursive call.
In fact, in probably does.
But in either case, this seems like exactly where you should be calling super._update instead.


Point 4:

In your 4th bullet:

It is not really clear what you've tried and what you haven't.

A call to super._update appears in several execution flows within your contract's _update function.
In some of them, there are several lines of code before the actual call.
In others, there is only a single require statement, as you've mentioned.
And in all of them, there are several modifiers being executed prior to everything.
Specifically - notBlacklisted(sender) and notBlacklisted(recipient).
So what makes you so sure that it is not one of these checks which causes your transaction to revert?

Also, understandably, you've tried to do some code-reuse, by putting these two checks in a single place in the code. But note that these checks are usually better off conducted in each user-facing (public or external) function, while leaving the internal functionality of the contract oblivious to this logic.

If we divide 10/100 for tax calculation, it will give 0 instead of 0.1 For decimal calculation, we are using this.

Tested it...Logically you are right. But when we tested the code, it seems super._transfer seems to fix the recursion caused by super._update. This is true only for burn call.

Point 4:

Its a simple reflection. We do 3 transfers. one to tax wallet, one to recipient and one burn.

This statement implies a fundamental lack of understanding of very basic math principles.

The calculation:

uint256 feeAmount = (amount * transactionFee * 10**decimals()) / (100 * 10**decimals());
uint256 burnAmount = (amount * burnRate * 10**decimals()) / (100 * 10**decimals());

Is mathematically equivalent to:

uint256 feeAmount = amount * transactionFee / 100;
uint256 burnAmount = amount * burnRate / 100;

But with a higher probability of reverting the transaction due to overflow in an intermediate calculation.

In other words, the output (feeAmount and burnAmount) is the same regardless of whether or not you multiply by 10**decimals(), but the transaction is more likely to revert on overflow when you apply that multiplication.

As with regards to "we divide 10/100 for tax calculation" - you don't "divide 10/100" anywhere!


As for the rest (point 3 and 4) - kudos for not explaining properly any of the doubts raised in there.

I recommend that you start off with something much simpler, which would allow you to:

  1. Investigate more easily
  2. Test and verify more thoroughly
  3. Describe here more clearly (if any issues still remain)