ERC20 token smart contract check

Question

I updated my smart contract code according to the explanations you gave and the instructions you gave. Please, if possible, check the rewritten code again and give your general opinion, is this smart contract safe, complete and in terms of the writing is clean and professional?

Thankful ...

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

// Import necessary OpenZeppelin libraries for ERC20 functionality
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Burnable.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Snapshot.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol";
import "@openzeppelin/contracts/security/Pausable.sol";
import "@openzeppelin/contracts/access/AccessControl.sol";

/**
 * @title TokenExam
 * @dev A comprehensive ERC20 token contract with additional features and access control.
 */
contract TokenExam is ERC20, ERC20Burnable, ERC20Snapshot, AccessControl, Pausable, ERC20Permit{
    address public owner;

    bytes32 public constant ACCESS_CONTROLLER_ROLE = keccak256("ACCESS_CONTROLLER_ROLE");
    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
    bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
    bytes32 public constant FREEZER_ROLE = keccak256("FREEZER_ROLE");
    bytes32 public constant SNAPSHOT_ROLE = keccak256("SNAPSHOT_ROLE");
    
    mapping(string => bool) private frozenFeatures;

    event FeatureFrozen(string feature, address freezer);
    event FeatureUnfrozen(string feature, address freezer);
    event Mint(address to  , uint256 amount);
    event Burn(address account  , uint256 amount);

    /**
     * @dev Contract constructor
     */
    constructor() ERC20("TokenExam", "TEX") ERC20Permit("TokenExam") {
        owner = msg.sender;

        _grantRole(DEFAULT_ADMIN_ROLE, owner);
        _grantRole(ACCESS_CONTROLLER_ROLE , owner);
        _grantRole(MINTER_ROLE, owner);
        _grantRole(PAUSER_ROLE, owner);
        _grantRole(FREEZER_ROLE, owner);
        _grantRole(SNAPSHOT_ROLE, owner);
        _mint(owner, 1000000000 * 10 ** decimals());
    }

    /**
     * @dev Modifier to check if a feature is not frozen.
     * @param feature The feature to check.
     */
    modifier whenNotFrozen(string memory feature) {
        require(!frozenFeatures[feature], "Feature is frozen !");
        _;
    }

    /**
     * @dev Modifier to check if an account has enough allowance.
     * @param account The account to check allowance from.
     * @param amount The required allowance.
     */
    modifier onlyAllowed(address account, uint256 amount) {
        require(
            allowance(account , msg.sender) >= amount,
            "Not enough allowance !"
        );
        _;
    }

    /**
     * @dev Pauses the contract operations, restricted to PAUSER_ROLE.
     */
    function pause() public onlyRole(PAUSER_ROLE) {
        _pause();
    }

    /**
     * @dev Unpauses the contract operations, restricted to PAUSER_ROLE.
     */
    function unpause() public onlyRole(PAUSER_ROLE) {
        _unpause();
    }

    /**
     * @dev Freezes a feature, restricted to FREEZER_ROLE.
     * @param feature The feature to freeze.
     */
    function freeze(string memory feature) public onlyRole(FREEZER_ROLE) {
        require(!frozenFeatures[feature], "Feature is already frozen !");

        frozenFeatures[feature] = true;
        emit FeatureFrozen(feature, msg.sender);
    }

    /**
     * @dev Unfreezes a feature, restricted to FREEZER_ROLE.
     * @param feature The feature to unfreeze.
     */
    function unfreeze(string memory feature) public onlyRole(FREEZER_ROLE) {
        require(frozenFeatures[feature], "Feature is not frozen !");

        frozenFeatures[feature] = false;
        emit FeatureUnfrozen(feature, msg.sender);
    }

    /**
     * @dev Mints new tokens and assigns them to an address, restricted to MINTER_ROLE.
     * @param to The address to receive the minted tokens.
     * @param amount The amount of tokens to mint.
     */
    function mint(address to, uint256 amount) 
        public
        whenNotPaused
        whenNotFrozen("mint") 
        onlyRole(MINTER_ROLE) 
    {
        _mint(to, amount);
        emit Mint(to , amount);
    }

    /**
     * @dev Burns a specified amount of tokens, restricted to burners.
     * @param amount The amount of tokens to burn.
     */
    function burn(uint256 amount)
        public
        whenNotPaused
        whenNotFrozen("burn")
        override
    {
        super.burn(amount);
        emit Burn(msg.sender , amount);
    }

    /**
     * @dev Burns a specified amount of tokens from an account, restricted to burners.
     * @param account The account from which tokens will be burned.
     * @param amount The amount of tokens to burn.
     */
    function burnFrom(address account , uint256 amount)
        public
        whenNotPaused
        whenNotFrozen("burn")
        onlyAllowed(account , amount)
        override
    {
        super.burnFrom(account, amount);
        emit Burn(account , amount);
    }

    /**
     * @dev Transfers tokens to a specified address, restricted to non-frozen transfers.
     * @param to The address to which tokens will be transferred.
     * @param amount The amount of tokens to transfer.
     * @return A boolean indicating the success of the transfer.
     */
    function transfer(address to, uint256 amount)
        public 
        whenNotPaused
        whenNotFrozen("transfer") 
        override
        returns (bool) 
    {
        super.transfer(to, amount);
        return true;
    }

    /**
     * @dev Transfers tokens from one address to another, restricted to non-frozen transfers.
     * @param from The address from which tokens will be transferred.
     * @param to The address to which tokens will be transferred.
     * @param amount The amount of tokens to transfer.
     * @return A boolean indicating the success of the transfer.
     */
    function transferFrom(address from, address to, uint256 amount)
        public 
        whenNotPaused
        whenNotFrozen("transferFrom") 
        override
        returns (bool) 
    {
        super.transferFrom(from, to, amount);
        return true;
    }

    /**
     * @dev Transfers tokens from the sender to a specified address with a permit, restricted to non-frozen transfers.
     * @param to The address to which tokens will be transferred.
     * @param amount The amount of tokens to transfer.
     * @param deadline The deadline for the permit.
     * @param v The v signature parameter.
     * @param r The r signature parameter.
     * @param s The s signature parameter.
     */
    function transferWithPermit(
        address to,
        uint256 amount,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) 
    external
    whenNotPaused
    whenNotFrozen("transferWithPermit") 
    {
        permit(msg.sender, address(this), amount, deadline, v, r, s);
        transfer(to, amount);
    }

     /**
     * @dev Transfers tokens from a specified owner's account to another address with a permit, restricted to non-frozen transfers.
     * @param tokenOwner The owner of the tokens.
     * @param from The address from which tokens will be transferred.
     * @param to The address to which tokens will be transferred.
     * @param amount The amount of tokens to transfer.
     * @param deadline The deadline for the permit.
     * @param v The v signature parameter.
     * @param r The r signature parameter.
     * @param s The s signature parameter.
     */
    function transferFromWithPermit(
        address tokenOwner,
        address from,
        address to,
        uint256 amount,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) 
    external 
    whenNotPaused
    whenNotFrozen("transferFromWithPermit") 
    {
        permit(msg.sender, from, amount, deadline, v, r, s);
        transferFrom(tokenOwner , to, amount);
    }

    /**
     * @dev Approves the allowance of a spender for a specific amount of tokens, restricted to non-frozen approvals.
     * @param spender The address to which allowance will be given.
     * @param amount The amount of allowance to grant.
     * @return A boolean indicating the success of the approval.
     */
    function approve(address spender, uint256 amount) 
        public 
        whenNotPaused
        whenNotFrozen("approve") 
        override
        returns (bool) 
    {
        super.approve(spender, amount);
        return true;
    }

    /**
     * @dev Creates a snapshot of the current token balances, restricted to SNAPSHOT_ROLE holders.
     */
    function snapshot() 
        public 
        whenNotPaused
        whenNotFrozen("snapshot") 
        onlyRole(SNAPSHOT_ROLE) 
    {
        _snapshot();
    }

    /**
     * @dev Grants a role to an account, restricted to ACCESS_CONTROLLER_ROLE holders.
     * @param role The role to grant.
     * @param account The address to which the role will be granted.
     */
    function grantRole(string memory role , address account)
        public
        whenNotFrozen("grantRole")
        onlyRole(ACCESS_CONTROLLER_ROLE)
    {
        bytes32 roleHash = keccak256(bytes(role));

        require(!hasRole(roleHash , account) , "Role already assigned to the account !");
        _grantRole(roleHash , account);
    }

    /**
     * @dev Revokes a role from an account, restricted to ACCESS_CONTROLLER_ROLE holders.
     * @param role The role to revoke.
     * @param account The address from which the role will be revoked.
     */
    function revokeRole(string memory role , address account)
        public
        whenNotFrozen("grantRole")
        onlyRole(ACCESS_CONTROLLER_ROLE)
    {
        bytes32 roleHash = keccak256(bytes(role));
        
        require(hasRole(roleHash , account) , "Role not assigned to the account !");
        _revokeRole(roleHash , account);
    }

    // The following functions are overrides required by Solidity.

    /**
     * @dev Hook that is called before any token transfer, including minting and burning.
     * It includes checks related to the ERC20 standard and ERC20Snapshot extension.
     * @param from The address from which tokens are transferred.
     * @param to The address to which tokens are transferred.
     * @param amount The amount of tokens being transferred.
     */
    function _beforeTokenTransfer(address from, address to, uint256 amount)
        internal
        override(ERC20, ERC20Snapshot)
    {
        super._beforeTokenTransfer(from, to, amount);
    }
}

Answer 1

I can spot something, you shouldnt be marking internal functions as whenNotFrozen(), just stick to public functions since it's impossible to call internal functions directly, you're just making redundant checks that waste gas. (For example, marking _beforeTokenTransfer whenNotFrozen is functionally equivalent to making transfer/transferFrom whenNotFrozen)

Also, there is no reason to override a function if you don't change its behavior. I don't think it generates any additional bytecode but i personally find it messy. You could straight up delete them.

Answer 2

First of all, I noticed that you are using OpenZeppelin contracts as imports, which is a good choice since they are well-tested and audited. However, you should also make sure that you are using the latest version of the contracts and that they are compatible with your Solidity compiler version. For example, the ERC20Permit contract was introduced in OpenZeppelin 4.0, which requires Solidity 0.8 or higher.

Secondly, I suggest that you add some comments and documentation to your code to make it more readable and understandable. You can use the NatSpec format to document your functions and events, as well as the contract itself. This will help other developers and users to interact with your contract more easily.

Thirdly, I recommend that you add some modifiers to your functions to check for some common conditions, such as whether the caller is the owner of the contract, whether the amount of tokens to be transferred or minted is positive, whether the recipient or spender address is valid, etc. This will help you avoid some potential errors and vulnerabilities in your contract.

Fourthly, I noticed that you have a function called transferWithPermit that allows the user to transfer tokens without having to approve the spender first. This is a useful feature that can save gas and improve user experience. However, you should also be careful about the security implications of this function. For example, you should make sure that the permit signature is valid and not expired, that the nonce of the permit matches the nonce of the token owner, that the amount of tokens to be transferred does not exceed the allowance given by the permit, etc. You can use the OpenZeppelin ERC20Permit contract as a reference for implementing this function correctly.

Fifthly, I suggest that you add some events to your contract to emit information about important state changes, such as when a feature is frozen or unfrozen. This will help users and developers to monitor and track the activities of your contract more easily.