Use OpenZeppelin's AccessControl instead of Distributor and Ownable

[SKYBITDev3]

OpenZeppelin developed AccessControl and I was wondering why you didn't use it. There was no need to have Distributor at all.

Note also that transferOwnership(address(tokenManager) in the sample code is crazy!

Please see my code below with bolded parts for emphasis, and update your code at https://remix.ethereum.org/axelarnetwork/axelar-docs/blob/main/public/samples/interchain-token-iinterchaintoken.sol .

contract MyInterchainToken is ERC20, AccessControl, ERC20Permit {
    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
    bytes32 public constant BURNER_ROLE = keccak256("BURNER_ROLE");

    using AddressBytesUtils for address;

    ITokenManager public tokenManager;
    IInterchainTokenService public service =
        IInterchainTokenService(0xF786e21509A9D50a9aFD033B5940A2b7D872C208);

    address public creator;

    constructor(address adminAddress) ERC20("MyInterchainToken", "MITKN") ERC20Permit("MyInterchainToken") {
        _grantRole(DEFAULT_ADMIN_ROLE, adminAddress);

        // Mint 1,000 tokens to the creator
        creator = adminAddress;
        _mint(creator, 1000 * 10**decimals());

        // Register this token (could also be done 1-time smart contract invocation)
        // Not a good practice beacuse it can't go to non-EVM chains
        deployTokenManager("");
    }

    function mint(address to, uint256 amount) public onlyRole(MINTER_ROLE) {
        _mint(to, amount);
    }

    function burn(address from, uint256 amount) external onlyRole(BURNER_ROLE) {
        _burn(from, amount);
    }

    function deployTokenManager(bytes32 salt) internal {
        bytes memory params = service.getParamsMintBurn(
            msg.sender.toBytes(),
            address(this)
        );
        bytes32 tokenId = service.deployCustomTokenManager(
            salt,
            ITokenManagerType.TokenManagerType.MINT_BURN,
            params
        );
        // tokenManager = ITokenManager(service.getTokenManagerAddress(tokenId));
        // transferOwnership(address(tokenManager)); // THIS SHOULD NEVER BE DONE!
        setTokenManager(service.getTokenManagerAddress(tokenId));
    }

    function myCustomFunction() public {
        // Just an example custom feature
        // Give owner 1,000 more
        _mint(creator, 1000 * 10**18);
    }

    /*
     * Deploy this token, then register it with the Interchain Token Service
     * You'll be given a TokenManager which you can set here, allowing the
     * local send methods to function.
     */
    function setTokenManager(address _tokenManager) public onlyRole(DEFAULT_ADMIN_ROLE) {
        tokenManager = ITokenManager(_tokenManager);
        _grantRole(MINTER_ROLE, _tokenManager);
        _grantRole(BURNER_ROLE, tokenManager);
    }

    /**
     * @notice Implementation of the interchainTransfer method
     * @dev We chose to either pass `metadata` as raw data on a remote contract call, or, if no data is passed, just do a transfer.
     * A different implementation could have `metadata` that tells this function which function to use or that it is used for anything else as well.
     * @param destinationChain The destination chain identifier.
     * @param recipient The bytes representation of the address of the recipient.
     * @param amount The amount of token to be transfered.
     * @param metadata Either empty, to just facilitate an interchain transfer, or the data can be passed for an interchain contract call with transfer as per semantics defined by the token service.
     */
    function interchainTransfer(
        string calldata destinationChain,
        bytes calldata recipient,
        uint256 amount,
        bytes calldata metadata
    ) external payable {
        address sender = msg.sender;

        // Metadata semantics are defined by the token service and thus should be passed as-is.
        tokenManager.transmitInterchainTransfer{value: msg.value}(
            sender,
            destinationChain,
            recipient,
            amount,
            metadata
        );
    }

    /**
     * @notice Implementation of the interchainTransferFrom method
     * @dev We chose to either pass `metadata` as raw data on a remote contract call, or, if no data is passed, just do a transfer.
     * A different implementation could have `metadata` that tells this function which function to use or that it is used for anything else as well.
     * @param sender the sender of the tokens. They need to have approved `msg.sender` before this is called.
     * @param destinationChain the string representation of the destination chain.
     * @param recipient the bytes representation of the address of the recipient.
     * @param amount the amount of token to be transfered.
     * @param metadata either empty, to just facilitate a cross-chain transfer, or the data to be passed to a cross-chain contract call and transfer.
     */
    function interchainTransferFrom(
        address sender,
        string calldata destinationChain,
        bytes calldata recipient,
        uint256 amount,
        bytes calldata metadata
    ) external payable {
        uint256 _allowance = allowance(sender, msg.sender);

        if (_allowance != type(uint256).max) {
            _approve(sender, msg.sender, _allowance - amount);
        }

        tokenManager.transmitInterchainTransfer{value: msg.value}(
            sender,
            destinationChain,
            recipient,
            amount,
            metadata
        );
    }
}

[SKYBITDev3]

After the contract is deployed, OpenZeppelin's AccessControl offers functions to manage the roles, e.g.:

Using JavaScript ethers:

  const BURNER_ROLE = ethers.keccak256(ethers.toUtf8Bytes("BURNER_ROLE"))

  await contract.revokeRole(BURNER_ROLE, addressToRevokeRole)

  await contract.hasRole(BURNER_ROLE, addressToCheck)

  await contract.grantRole(BURNER_ROLE, addressToGrantRole)

[SKYBITDev3]

Note that adminAddress is passed into the constructor because when using a factory (e.g. Constant Address Deployer) to deploy the contract, msg.sender may be the factory's (or a temporary proxy's) address. It wouldn't make sense to be granting the factory admin role and initial supply of tokens!

[SKYBITDev3]

AccessControl with BURNER_ROLE was suggested here as a better alternative to completely relinquishing ownership and control of the token contract via transferOwnership (that your sample code still shows at https://remix.ethereum.org/axelarnetwork/axelar-docs/blob/main/public/samples/interchain-token-iinterchaintoken.sol).

But it still has risks, so I've made a more secure suggestion that removes BURNER_ROLE in #101

[re1ro]

We are not using OZ contacts as they have worse upgradability properties because of using plainly defined storage slots, and using uint256[40] private __gap; at the end of the contact to leave some empty storage slots. So that is a pretty risky work around and every contract upgrade requiring changes to the storage slots becomes very risky as it's still possible to get it wrong.

Another point is AccessControl can have several accounts with the same BURNER_ROLE and it breaks some security assumptions for the ITS.

Hypothetically anyone could still implement their custom token with using AccessControl and it would work just fine, there is nothing preventing this from the ITS side. Our custom token implementation is just an example how to integrate with ITS, that's it. Custom tokens are still requiring users to trust the developer as developer has full control over the functionality and implementation. ITS is providing trustless Standard Token for every use case that is not requiring custom token functionality and Standard token is the default option.