Review for a draft upgradeable and pausable token

Smart Contracts Review Wanted
,
1

Hello, I am working towards deploying an upgradeable and pausable ERC-20 token contract and would appreciate any constructive criticism of the current implementation (71 lines). The token also has an access control mechanism, which is fairly self-explanatory.

One aspect I would particularly appreciate comments on is the initialisation. In particular, the contract uses two init functions, which can apparently (see readme in OZ's upgradeable contracts github) cause insecurities. I believe that the use of the two init calls in lines 17 and 18 do not create such an insecurity and would value any dissenting view.

pragma solidity ^0.8.0;
  
import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20PausableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol";

contract SimpleToken is Initializable, ERC20PausableUpgradeable, AccessControlUpgradeable {

    bytes32 public constant USER_ROLE = keccak256("USER_ROLE");
    bytes32 public constant COMPLIANCE_ROLE = keccak256("COMPLIANCE_ROLE");

    mapping(address => bool) private _frozen;

    function initialize() public initializer {
        _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
        _setRoleAdmin(USER_ROLE, COMPLIANCE_ROLE);
        __ERC20_init("SimpleToken","SMT");
        __ERC20Pausable_init();
    }

    function mint(address to, uint256 amount) public {
        require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "Must be ADMIN.");
        require(hasRole(USER_ROLE, to), "Recipient must be USER.");
        _mint(to, amount);
    }

    function burn(address from, uint256 amount) public {
        require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "Must be ADMIN.");
        _burn(from, amount);
    }

    function transfer(address to, uint256 value) public override returns (bool) {
        require(hasRole(USER_ROLE, to), "Recipient must be USER.");
        require(!_frozen[msg.sender], "Must be unfrozen.");
        require(!_frozen[to], "Recipient address must be unfrozen.");
        return super.transfer(to, value);
    }

    function transferFrom(address from, address to, uint256 value) public override returns (bool) {
        require(hasRole(USER_ROLE, to), "Recipient must be a USER.");
        require(hasRole(USER_ROLE, msg.sender), "Must be USER.");
        require(!_frozen[msg.sender], "Must be unfrozen.");
        require(!_frozen[to], "Recipient must be unfrozen.");
        require(!_frozen[from], "Sender must be unfrozen.");
        return super.transferFrom(from, to, value);
    }

    function freeze(address account) public {
        require(hasRole(COMPLIANCE_ROLE, msg.sender), "Must hold COMPLIANCE role.");
        _frozen[account] = true;
    }

    function unfreeze(address account) public {
        require(hasRole(COMPLIANCE_ROLE, msg.sender), "Must hold COMPLIANCE role. ");
        _frozen[account] = false;
    }

    function isAccountFrozen(address account) public view returns (bool) {
        return _frozen[account];
    }

    function pause() public whenNotPaused {
        require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "Must be ADMIN.");
        _pause();
    }
    
    function unpause() public whenPaused {
        require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "Must be ADMIN.");
        _unpause();
    }
}
1 Like
2

calling multiple "sub-initializers" within your initialize() function is fine. You just need to make sure that you call all the initializers of the inherited contracts (compare with Openzeppelins audit of Forta, the initialize() code mentioned is here)

1 Like
3

Thanks for the response and reference to that OZ audit! Here is how I interpret your suggestion:

  1. We must call all initializers of inherited contracts, recursively all the way up the inheritance tree.

  2. SimpleToken inherits Initializable, which has no initializer, so we are OK there. (In what follows, I will ignore inheritance of Initializable, since it has no initializer.)

  3. SimpleToken inherits ERC20PausableUpgradeable, which has an initializer __ERC20Pausable_init() that we call.

  4. ERC20PausableUpgradeable inherits ERC20Upgradeable, and PausableUpgradeable. ERC20Upgradeable has an initializer that we call with __ERC20_init("SimpleToken","SMT") and inherits ContextUpgradeable, IERC20Upgradeable, and IERC20MetadataUpgradeable. ContextUpgradeable has the initializer below (do I need to call this and can I, given that it is external?):

function initialize() external initializer {
        __init(true);
    }

The other two inheritances are interfaces without initializers. PausableUpgradeable has an initializer, but this was already called by the __ERC20Pausable_init() function.

  1. SimpleToken inherits AccessControlUpgradeable, which has initializer __AccessControl_init(). We should call this in SimpleToken's initializer. (Is this a formality/style issue since __AccessControl_init() is an empty function?)

Summarising: we may need to add the ContextUpgradeable initializer and should add the AccessControl initializer.

Final question: what, if anything, would concretely go wrong with the contract as currently written?

1 Like
4

I am not a professional myself (but aspiring to be), so take everything with a grain of salt :slight_smile:

  1. yes, though I think most of them do recursive initializes by default

2.-3. had no question

  1. I only quickly glanced over it, but I dont see that particular initializer in ContextUpgradeable.sol, maybe there would be no danger in leaving it uninitialized, depends on what that __init(true); does

  2. if its empty there should be no danger in leaving it uninitialized. Makes me wonder why it is there in the first place (maybe meant to be overridden?). I dont know if its best practice to call it anyway though

Final q.: just by quickly glancing over it, I dont see any obvious mistakes

1 Like
5

Thanks very much! I hope my questions help both of us learn the quirks of these contracts.

After further investigation, here are the changes I will make to the contract:

  • Call the initializers __AccessControl_init() and __Context_init(). (The init function I mentioned in point (4) above is irrelevant - I accidentally was looking at a different ContextUpgradeable.sol, not the one in my inheritance tree.)

  • Add a dummy constructor so that the implementation contract is initialized immediately upon deployment. See this link, which describes this as a best practice.

  • I also reordered the code within the initialize() function so that all init calls are first and _setupRole(...) and _setRoleAdmin(...) are at the end, because it seems odd to call these before __AccessControl_init().

So now, the start of the contract looks like this:

pragma solidity ^0.8.0;
  

import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20PausableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol";

contract SimpleToken is Initializable, ERC20PausableUpgradeable, AccessControlUpgradeable {

    bytes32 public constant USER_ROLE = keccak256("USER_ROLE");
    bytes32 public constant COMPLIANCE_ROLE = keccak256("COMPLIANCE_ROLE");

    mapping(address => bool) private _frozen;

    /// @custom:oz-upgrades-unsafe-allow constructor
    constructor() {
        _disableInitializers();
    }

    function initialize() public initializer {
        __ERC20_init("SimpleToken","SMT");
        __ERC20Pausable_init();
        __AccessControl_init();
        __Context_init();

        _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
        _setRoleAdmin(USER_ROLE, COMPLIANCE_ROLE);
    }
1 Like