Can you review my code of a simple coin contract with tax on transfer?

Smart Contracts Review Wanted
1

Hey guys! I am very new to Solidity. Making my frst steps... Can you please look through this coin contract? It's a simple coin, with a permanent 3% tax on any transfer except for to/from owners wallet... Any weak spots or improvements? Thanks in advance!

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.9.0;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Burnable.sol";
import "@openzeppelin/contracts/access/Ownable.sol";

contract Test6 is ERC20, ERC20Burnable, Ownable {
    constructor() ERC20("Test6", "Test6") {
        _mint(msg.sender, 1000000000 * 10 ** decimals());
    }

    function _transfer(
        address from,
        address to,
        uint256 amount
    ) internal virtual override {
        require(balanceOf(from) >= amount, "Error: transfer amount exceeds balance");

        if (from == owner() || to == owner()) {
            //Owner's wallet sends and receives coins without Tax
            super._transfer(from, to, amount); 
        } else {
            uint256 tax = amount * 3 / 100;
            uint256 amountAfterTaxDeducted = amount - tax;

            super._transfer(from, to, amountAfterTaxDeducted); 
            super._transfer(from, owner(), tax);
        }

    }


}
1 Like
2
  1. The part "owner's wallet sends without tax" seems logically wrong.
  2. The part "owner's wallet receives without tax" seems logically redundant.

Point 1 means that the owner actually loses the desired tax, since it goes to the recipient.
Point 2 would be handled correctly in the else clause (if you removed it from the if clause).

You might wanna change the whole thing to something as simple as:

uint256 tax = amount * 3 / 100;
uint256 amountAfterTaxDeducted = amount - tax;

super._transfer(from, to, amountAfterTaxDeducted); 
if (from != owner())
    super._transfer(from, owner(), tax);

You can also keep the if (to == owner()) part, in order to ensure a single transfer instead of two transfers whenever the recipient is the owner:

if (to == owner()) {
    super._transfer(from, to, amount); 
} else {
    uint256 tax = amount * 3 / 100;
    uint256 amountAfterTaxDeducted = amount - tax;

    super._transfer(from, to, amountAfterTaxDeducted); 
    if (from != owner())
        super._transfer(from, owner(), tax);
}

Side note with regards to:

require(balanceOf(from) >= amount, "Error: transfer amount exceeds balance");

I'm pretty sure that it is verified in super._transfer, so you might wanna get rid of it here.

2 Likes
3

Actually it's not wrong. It makes owner's life much easier... Imagine you need to send 1000 coins from owner's wallet to pay for marketing / team etc. If owner sends without tax you just send 1000 and that's all. But if all the transactions are taxable you have to calculate how much to send in order for the recipient gets exactly that 1000...

1 Like
4

I think your current code is right.

2 Likes