Enhance security related to burning

[SKYBITDev3]

Granting permission to the token manager to freely burn anyone's tokens via burn(account, amount) makes things easy to code and maintain, but there are risks.

There can be a way for an admin (or a hacker who is able to become admin) to freely burn any other holders' tokens. It's better to remove such a risk.

So as further improvement to my suggested interchain token code in https://github.com/axelarnetwork/interchain-token-service/issues/100#issue-1855546703, it'd be much more secure to replace burn(account, amount) with burnFrom(account, amount) from OpenZeppelin's ERC20Burnable extension.

burnFrom(account, amount) requires the owner of the tokens to approve or permit some other account (in this case the token manager) to spend some tokens.

So to increase security, the suggestion is to remove burn(account, amount) and BURNER_ROLE, and add ERC20Burnable which exports burnFrom(account, amount).

The interchain token sample code would then have code like this:

contract MyInterchainToken is ERC20, ERC20Burnable, AccessControl, ERC20Permit {
    bytes32 public constant MINTER_ROLE = keccak256("MINTER_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);
    }

//    Removed: function burn(address from, uint256 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);
    }

    /**
     * @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]

Since burnFrom(account, amount) requries approval, there would need to be an extra step beforehand in which the owner of the tokens responds to a permit or approval request. So when a user initiates an interchain transfer in a dapp, the permit or approval call in the web3 (e.g. ethers) code would cause a popup box in the crypto wallet asking for signature (if permit was possible) or an approval transaction. Only when that's passed, the token manager can burn and go on with the rest.

[SKYBITDev3]

Here's some of my script code for using permit or approve that you could use:

const { ethers, network } = require(`hardhat`)

async function main() {
  const [ownerWallet, spenderWallet] = await ethers.getSigners()
  console.log(`Using network: ${network.name} (${network.config.chainId}), RPC url: ${network.config.url}`)

  const tokenContractName = `TESTERC20`
  const contractAddress = `0xc45Eb76AB6A29B64FD17A0F49e214a1f0a20A59D` // localhost

  const contractOfOwner = await ethers.getContractAt(tokenContractName, contractAddress, ownerWallet)
  const contractOfSpender = await ethers.getContractAt(tokenContractName, contractAddress, spenderWallet)

  let txRec

  const totalSupply = ethers.formatUnits(await contractOfOwner.totalSupply())

  console.log(`Initially:`)
  console.log(`owner has ${ethers.formatUnits(await contractOfOwner.balanceOf(ownerWallet.address))} of ${totalSupply} tokens`)
  console.log(`spender has ${ethers.formatUnits(await contractOfOwner.balanceOf(spenderWallet.address))} of ${totalSupply} tokens`)

  console.log(`owner has allowed spender to spend ${ethers.formatUnits(await contractOfOwner.allowance(ownerWallet.address, spenderWallet.address))} tokens`)

  console.log(`owner has ${ethers.formatUnits(await ethers.provider.getBalance(ownerWallet.address), `ether`)} of native currency`)
  console.log(`spender has ${ethers.formatUnits(await ethers.provider.getBalance(spenderWallet.address), `ether`)} of native currency`)

  const amountAllowedToSpend = ethers.parseUnits(`2`, `ether`)

  if (tokenHasPermit(await contractOfOwner.getDeployedCode())) {
    const deadline = Math.floor(Date.now() / 1000 + 3600)
    const splitSigAsArray = await getPermitSignature(contractOfOwner, ownerWallet, spenderWallet, amountAllowedToSpend, deadline) // ownerWallet does the signing in here to permit spender to spend his tokens
    console.log(`Owner has given his signature`)
    console.log(`Spender is now calling token's permit function with owner's signature so that she can spend his tokens...`)
    txRec = await contractOfSpender.permit( // any account can call the permit function, as long as there's a signature (values in splitSigAsArray) that proves that owner has permitted his tokens to be spent by spender. Gas is paid by permit function caller, so approval process is free for owner if he didn't call permit.
      ownerWallet.address,
      spenderWallet.address,
      amountAllowedToSpend,
      deadline,
      ...splitSigAsArray
    )
  } else {
    console.log(`Owner is calling token's approve function...`)
    txRec = await contractOfOwner.approve(spenderWallet.address, amountAllowedToSpend) // only owner can call approve to let spender spend his tokens. contractOfSpender.approve doesn't work.
  }
  await txRec.wait()

  console.log(`After permit / approve:`)
  console.log(`owner has allowed spender to spend ${ethers.formatUnits(await contractOfOwner.allowance(ownerWallet.address, spenderWallet.address))} tokens`)

  console.log(`owner has ${ethers.formatUnits(await ethers.provider.getBalance(ownerWallet.address), `ether`)} of native currency`)
  console.log(`spender has ${ethers.formatUnits(await ethers.provider.getBalance(spenderWallet.address), `ether`)} of native currency`)

  txRec = await contractOfSpender.transferFrom(ownerWallet.address, spenderWallet.address, amountAllowedToSpend) // spender calls transferFrom to transfer tokens from owner
  await txRec.wait()
  console.log(`After transferFrom:`)

  console.log(`spender took ${ethers.formatUnits(amountAllowedToSpend)} tokens from owner`)

  console.log(`owner has ${ethers.formatUnits(await contractOfOwner.balanceOf(ownerWallet.address))} of ${totalSupply} tokens`)
  console.log(`spender has ${ethers.formatUnits(await contractOfOwner.balanceOf(spenderWallet.address))} of ${totalSupply} tokens`)

}

const getPermitSignature = async (tokenContract, ownerWallet, spenderWallet, value, deadline) => { // owner allows spender to spend value wei tokens before deadline
  const [name, nonce] = await Promise.all([
    tokenContract.name(),
    tokenContract.nonces(ownerWallet.address),
  ])

  const domain = {
    name,
    version: `1`,
    chainId: network.config.chainId,
    verifyingContract: tokenContract.target
  }
  // console.log(`domain: ${JSON.stringify(domain)}`)

  const permitTypes = {
    Permit: [{
      name: `owner`,
      type: `address`
    },
    {
      name: `spender`,
      type: `address`
    },
    {
      name: `value`,
      type: `uint256`
    },
    {
      name: `nonce`,
      type: `uint256`
    },
    {
      name: `deadline`,
      type: `uint256`
    },
    ],
  }
  const permitValues = {
    owner: ownerWallet.address,
    spender: spenderWallet.address,
    value,
    nonce,
    deadline,
  }
  // console.log(`permitValues: ${JSON.stringify(permitValues)}`)

  const signature = await ownerWallet.signTypedData(domain, permitTypes, permitValues) // owner signs the data
  const { v, r, s } = ethers.Signature.from(signature)

  // const recoveredAddress = ethers.verifyTypedData(
  //   domain,
  //   permitTypes,
  //   permitValues,
  //   { v, r, s }
  // )
  // console.log(`recoveredAddress: ${recoveredAddress}. Verification ${recoveredAddress === ownerWallet.address ? "passed" : "failed"}.`)

  return [v, r, s]
}

const tokenHasPermit = bytecode => bytecode.includes(`d505accf`) && bytecode.includes(`7ecebe00`) // permit & nonces selectors

main().catch(error => {
  console.error(error)
  process.exitCode = 1
})

Here's the output, showing no change in owner's native currency balance:

Using network: localhost (31337), RPC url: http://127.0.0.1:8545
Initially:
owner has 100.0 of 1000.0 tokens
spender has 0.0 of 1000.0 tokens
owner has allowed spender to spend 0.0 tokens
owner has 9999.9614911964478728 of native currency
spender has 10000.0 of native currency
Owner has given his signature
Spender is now calling token's permit function with owner's signature so that she can spend his tokens...
After permit / approve:
owner has allowed spender to spend 2.0 tokens
owner has 9999.9614911964478728 of native currency
spender has 9999.999880784353054822 of native currency
After transferFrom:
spender took 2.0 tokens from owner
owner has 98.0 of 1000.0 tokens
spender has 2.0 of 1000.0 tokens
Done in 8.48s.

[SKYBITDev3]

As burn(account, amount) is risky to include in a token, there'll be tokens that instead have burnFrom(account, amount). So the Interchain Token Service will need to be able to handle the different cases. e.g. if burn(account, amount) doesn't exist in the token contract then proceed to try permit (or approve if permit doesn't exist) and then burnFrom(account, amount).

[SKYBITDev3]

Please let me know what your plans are to resolve this issue.

[re1ro]

Hello @SKYBITDev3 ! Thank you for you interest in the project and such a detailed report. We are aware of the issue you are describing but at the same time we don't find it to be a security concern. Let me explain you why.

hacker who is able to become admin

As you already probably know security reports based on malicious actor taking over a service are not considered a vulnerability. Our tokens are minted and burned by the TokenManager that doesn't have an owner takes commands only from the TokenService itself. And the TokenService doesn't have an owner and can be upgraded only through voting on governance proposal on Axelar network. So there is no role on any of the contract that would be held by some wallet that could be stolen or hacked.

hacker is able to freely burn any other holders' tokens

If hacker takes over the service that would cause more serious problems than his ability to burn tokens. There is no benefit for hacked to burn your tokens. Instead hacked would just mint tokens and send them back to the token source chain. Or even simpler just send a command to the token source chain to give the hacker all the original token. And you would keep your token balance but the token would now worth nothing because the source token was stolen. So being able to mint or send cross-chain messages is much more dangerous that burning some token from an individual account.

burn vs burnFrom

As you can see adding approval + burnFrom doesn't improve any security properties of the system overall but would require users to spend additional gas every time they send tokens cross-chain. The design principle behind this is sending ERC20 cross-chain should be the same as sending it on the same chain. You wouldn't need to grand yourself an approval for doing do. Think about TokenManager to be an internal part of a cross-chain ERC20. We just separated it in different contract for cleaner architecture and responsibility separation.

Please feel free to ask more questions or bring up additional points if my explanation isn't sufficient. If not we will close this issue some time soon. Thank you!

[SKYBITDev3]

You should also consider the token issuer's side. After adding function burn(address from, uint256 amount) external into the token contract code, and also including a way to update who is allowed to execute it, it opens up the possibility that an admin of the token contract could grant himself the permission to call that function (e.g. to burn the tokens of someone he doesn't like). That would be impossible if such a function was not added.

Exchanges check the code of contracts as part of their due diligence when considering listing tokens. I recall Coinbase doesn't like to see functions that can affect the entire supply like pause. burn(address from, uint256 amount) is much worse, so I'd assume they'd hate it and not list tokens that have it, as technically those tokens of their customers could all be burned some day from the exchange's wallet (even if it's a cold wallet).

There is no benefit for hacker to burn your tokens

A hacker could use his power to burn as a ransom threat. e.g. he could say "give me 1M USDC otherwise I'll burn everyone's tokens", and he could prove that he really does have such power by starting to burn a few of someone's tokens.

It's a very big deal to be granting unlimited power over a token supply to an external entity - it has the power to burn the entire supply of every holders' tokens and can technically do that at any time, without user interaction or approval. With so much at stake, this matter shouldn't be taken lightly or quickly dismissed. Much thought and care is required, and the nervousness is justified.

I've focused on burning more than minting because burning may appear as a more negative impact to holders as they'd quickly notice in their wallet that they've lost what they had. With minting, holders may not immediately notice new mints unless they check etherscan or equivalent explorer. Usually they'd only be checking their wallet and when they see that they still have their tokens they'd feel OK. It's a bit like central banks creating huge amounts of fiat money from nothing... even though it's dilutionary, people don't protest about that in particular as they still have around the same amount of savings in their bank accounts as before. They may instead protest about prices increasing.

It shouldn't have to be all-or-nothing (grant complete power over token supply, or don't use the service). Instead of granting the power to burn the entire supply, it's better if permission can be granted to burn only the quantity of tokens that the holder wishes to transfer at the time. burnFrom makes that possible.

Remember when OpenSea originally had users approve their entire wallet balance? I've always thought that was crazy, as it places a lot of trust on an external entity. Then after many security incidents 1 or 2 years ago with millions of USD worth lost by users, they realized that was a mistake, and would now only request approval for amount required or let the user specify a limit. Wallet apps have also improved their interfaces so that users can easily set an allowance cap, rather than granting unlimited spending like before.

would require users to spend additional gas every time they send tokens cross-chain

No gas is spent for allowance approval if you use permit instead of approve because it's just a signature, not a transaction. See ERC20Permit and my code above.

[SKYBITDev3]

in Chainlink's Cross-Chain Interoperability Protocol (CCIP), approve is done to let the router take an amount of tokens:

const approvalTx = await tokenToSend.approve(routerAddress, amount);

at: https://github.com/smartcontractkit/ccip-starter-kit-hardhat/blob/73bf9823d0487217e535ce53da3f2eeb3e359c45/tasks/ccip-token-transfer.ts#L53

ccipSend in the router is called to do the interchain transfer:

const sendTx = await router.ccipSend(targetChainSelector, message);

in which token.safeTransferFrom is called (for which it was just approved) to transfer the amount of tokens to a pool: https://github.com/smartcontractkit/ccip/blob/db3b88ea0f8c5d94b0f5fe15bd5187dbaa2b2622/contracts/src/v0.8/ccip/Router.sol#L137

where it then gets burned: https://github.com/smartcontractkit/ccip/blob/db3b88ea0f8c5d94b0f5fe15bd5187dbaa2b2622/contracts/src/v0.8/ccip/pools/BurnMintTokenPool.sol#L32

The important step is the explicit approval of a finite amount - the router isn't allowed to simply take any or all tokens of the holder any time.

The Axelar Interchain Token Service should similarly have such an initial approval step. Maybe you can make it optional so that those who are security-concious can enable it and have burnFrom(account, amount) instead of burn(account, amount) in their token contract. It'd be better to provide such secure options than only offer one way.

[SKYBITDev3]

Exchanges check the code of contracts as part of their due diligence when considering listing tokens. I recall Coinbase doesn't like to see functions that can affect the entire supply

Here's an important quote from How Coinbase reviews tokens on Ethereum for technical security risks:

when single actors have the sole authority to execute a dangerous function, we will not recommend the asset for listing. Instead, we will work with asset issuers to help them apply mitigations to decentralize decision-making across multiple stakeholders of the project. A contract with dangerous superuser roles is only as secure as the protections on those roles.

When single actors hold the privilege to execute dangerous functions, there is nothing that can stop them from unilaterally executing that risky function and potentially impact millions of token holders; we do not believe this represents an open, blockchain-based financial system.

Also see Token Custody Risks: Defining Security in the Crypto World.

[re1ro]

Hey @SKYBITDev3 👋 I just wanted to remind you that tokenManager is not a role or any admin account but the TokenManager contract from that is creating the token in the first place. It burns token only if token is sent cross-chain or if user approved cross-chain send for another account.

It can't get hacked and can be upgraded only by governance proposal and just went through series of audits that we haven't published yet.

And back to the logical point: if hypothetical hacker can just send all your tokens to original chain, does it really matter if doesn't update in metamask, the value is gone anyway. So adding approve for burn would just increase the gas usage without improving the security of the system overall.

And if you think that your token implementation might be better for some cases and applications, you can deploy your custom token implementation cross-chain and grant the token manager minter and burner roles.

Your token would just need to inherit InterchainToken.sol to provide cross-chain methods as well. We could also add a different version of the TokenManager to use burnFrom instead of burn here to provide support for your use-case.

Custom tokens managers are deployed here

[SKYBITDev3]

you can deploy your custom token implementation cross-chain and grant the token manager minter and burner roles.

That's what my code in https://github.com/axelarnetwork/interchain-token-service/issues/100#issue-1855546703 shows:

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

But the risk with adding such a function with a role guard is that the admin of the contract (who can grant and revoke roles, who could be a team member of the token issuer) could one day grant herself admin and burn the entire token supply - every token (under the contract) that every holder had. Coinbase calls this "superuser risk" in an article I linked to above:

In smart contracts, operational risks typically manifest as superuser risks: when single actors have the sole authority to execute a dangerous function, we will not recommend the asset for listing.

Imagine if you added a transfer function into a token contract that made it possible for anyone with a transferer role to transfer anyone's tokens out to any address. Having the role guard (e.g. onlyRole(TRANSFERER_ROLE) modifier) might make it sound secure, but a lot of trust must then be given (by every token holder) to whoever had been granted the transferer role and also to the admin who can manage the roles. She could one day grant herself the transferer role and steal any or all tokens under the contract that are held by others (or only from the people whom she hates). Contracts need admins to manage the contract, but in the real world admins can some day turn rogue (e.g. due to disagreements about salary etc.).

The ERC20 token standard has transfer which only the holder can call, and transferFrom which any other account can call but only after approval of an allowance by the holder (via permit or approve function). A transfer function with a role guard would be considered to be insecure because of "superuser risk". These points about transfer are similar for "burn", with the only difference being the admin not gaining anything, just sabotaging (unless it's related to a ransom).

So a transfer function with role guard should not be added to a token contract, and instead everyone should just use the standard functions transfer and transferFrom. Similarly, a burn function with role guard should not be added to a token contract, and instead everyone should just stick with existing functions burn that only the holder can call and burnFrom that requires approval by the holder.

Token issuers are of course free to add such functions, but Axelar and other other highly reputed organizations should not be encouraging such practice. This means that you should implement alternatives that do not require token issuers to add such a burn function if they want to use Axelar's Interchain Token Service. Currently there are no alternatives.

[SKYBITDev3]

We could also add a different version of the TokenManager to use burnFrom instead of burn here to provide support for your use-case.

Yes, your code should by default check for burnFrom in the token, and if it has it (by inheriting ERC20Burnable) then get approval and call burnFrom. If it doesn't have burnFrom then you could try transferFrom, which is also secure due to approval requirement, and that's what Chainlink's Cross-Chain Interoperability Protocol does.

You could let the user choose whether or not they want approvals to be required when using the Interchain Token Service.

[SKYBITDev3]

Here's a list of 4 articles that I've come across by Coinbase that cover the "superuser" risk that I've been highlighting:

How Coinbase Protects Users From Risky Assets How Coinbase reviews tokens on Ethereum for technical security risks Security PSA: Protecting ERC-20 assets from malicious actors Token Custody Risks: Defining Security in the Crypto World

The third one in that list shows this code that it says is risky:

... which looks just like your code

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

...that you've published at: https://github.com/axelarnetwork/axelar-docs/blob/5c2cb34c424a0c28f2dafff24f04c08a0ca4f6e8/public/samples/interchain-token-iinterchaintoken.sol#L41-L43

[re1ro]

What are you describing is a hypothetical scenario of what developer can do when they deploy their custom token and connect it to ITS. It's a custom token deployed by a developer and they can deploy a good token or a bad token in the same manner as deploying just any ERC20 token. And the statement about the "superuser" is incorrect because token manager is a contract, it's not a user at all. So onlyOwner in the token example is granted for an ITS contact, not for some user account.

Again to remind you the default option for ITS is the Standard Token that doesn't have any of this functionality, that is trustless and can be deployed to any chain by anyone without any security concerns. Standard token is deployed automatically just by calling a method on the ITS and it's implementation is fixed and secure and can't be altered when calling deploy function. Our UI is going to list only Standard Interchain tokens and we might manually add some custom tokens that went through security audits (for example Coinbase might use it themselves to for bridging their USDC and it's up to them to follow their own recommendations you've listed above, we are not enforcing anything on developers).

The case you are describing is when you want to have your own version of the token with your custom functions in it and still connect it to the ITS. In this case you are in full control of what are you deploying and can make it as secure or unsecure as you want. You can use OZ contacts or not use them. You can require approval and burnFrom or you can save on gas. You can grand yourself an admin role or have no admin role at all. You have complete freedom of what you can do and ITS is not enforcing any constrains on you.

In case of custom token ITS is playing a role of an Interchain Token Protocol and it's up to developers to build anything they want on top of the protocol. The statement you are proposing is that we shouldn't allow developers to implement their own custom interchain tokens because they might build an insecure ones. And if we extend this statement than it can even sound like we shouldn't create GMP protocol because developers can write insecure cross-chain apps or we shouldn't have smart contracts at all because developers can deploy insecure ones. Which is obviously absurd.

Regardless we appreciate your persistentance and how much you care about security and ITS itself. Let's schedule a call next week to dissolve any of you remaining concerns and come to a conclusion on this 😄 I will send you an email with a calendly link soon, please pick a time that would work you. Looking forward to chat.

[SKYBITDev3]

I want to create a new ERC20 token with these conditions:

• It works well with Axelar Interchain Token Service (e.g. contract will have interchainTransfer function);
• When an interchainTransfer executes, tokens will be burned at source, minted at destination;
• Ownership of the token contract doesn't need to be transferred to any token manager, so that admin functions on the contract can be executed by token issuer at any time;
• There would be no way for an admin of the token contract to burn anyone elses' tokens.

What options are available that satisfies these conditions?

Let's schedule a call next week

OK, I'll check.

[re1ro]

@SKYBITDev3 I've sent an email to someone at SKYBIT but not sure if it was actually you or not 😊 Please use this link to schedule a call https://gist.github.com/re1ro/34ad5f310a4d4477c13cca61d29caf6c

[re1ro]

To address answer your question:

• To support interchainTransfer and interchainTransferFrom you would need your token to implement or inherit InterchainToken
• In order for those methods to work you would need to deploy and integrate the token manager by calling according methods.
• Token manager doens't need to have an ownership of the token, only permission to mint and burn. You can also make burn to require an approval, basically make it work like burnFrom. We might also add another type of token manager what would actually call burnFrom.
• It's up to you what permissions admin should have, I assume you would want only to have pause/unpause or something similar.

Or you could just inherit StandardizedToken.sol. Distributor role can be set to address(0x0) so there is not external supply control and no one can burn tokens.

If you actually don't need need any additional admin or custom functionality and StandardizedToken.sol completely satisfies your needs, you could just deploy it directly by calling these ITS methods. Passing distributer as address(0x0) would make it not mintable/burnable by admin/distributer. Only by the token manager contract. We can go into more details and all of your requirements on the call 😄

[SKYBITDev3]

• To support interchainTransfer and interchainTransferFrom you would need your token to implement or inherit InterchainToken

I import the interface:

import { IInterchainToken } from "@axelar-network/interchain-token-service/contracts/interfaces/IInterchainToken.sol";

• In order for those methods to work you would need to deploy and integrate the token manager by calling according methods.

I do that here:

    function deployTokenManager(bytes32 salt) public onlyRole(DEFAULT_ADMIN_ROLE) {
        bytes32 tokenId = its.deployCustomTokenManager(
            salt,
            ITokenManagerType.TokenManagerType.MINT_BURN,
            its.getParamsMintBurn(_msgSender().toBytes(), address(this))
        );
        setTokenManager(its.getTokenManagerAddress(tokenId));
    }

    function setTokenManager(address _tokenManagerAddress) public onlyRole(DEFAULT_ADMIN_ROLE) {
        tokenManager = ITokenManager(_tokenManagerAddress);
    }

* Token manager doens't need to have an ownership of the token, only permission to mint and burn.

But if an admin has the power to grant a token manager permission to freely burn, then she'd also have the power to grant herself permission to freely burn, and could go ahead with that any time in the future (e.g. after a salary dispute). This is "superuser risk" introduced by adding a new function like burn(address from, uint256 amount) public onlyRole(BURNER_ROLE) as Coinbase explained in their articles.

the statement about the "superuser" is incorrect because token manager is a contract

The "superuser" is the admin of the token contract who can giver herself access to burn other's tokens. Coinbase's warnings apply perfectly here.

• It's up to you what permissions admin should have, I assume you would want only to have pause/unpause or something similar.

There needs to be an admin that manages roles, that's normal. But if burn(address from, uint256 amount) public onlyRole(BURNER_ROLE) exists in a token contract, an admin could be tempted to grant herself BURNER_ROLE and call that function. To prevent that from ever happening, such a function should not be added to a token contract in the first place. That's why Coinbase calls such a burn function in particular risky. Without the risky function, the opportunity for an admin to exploit it wouldn't exist, which is the safest way.

You can also make burn to require an approval, basically make it work like burnFrom. We might also add another type of token manager what would actually call burnFrom.

Yes, I think burnFrom (or transferFrom then burn as done by ChainLink), and not add burn(address from, uint256 amount) public onlyRole(BURNER_ROLE), is the only properly secure solution, but I'm open to other ideas.

Or you could just inherit StandardizedToken.sol.

No, I'm using OpenZeppelin's ERC20Upgradeable.

ITS is not enforcing any constrains on you

But in order for the token to work with ITS currently, I'd need to add the function burn(address from, uint256 amount) public onlyRole(BURNER_ROLE) into the token contract, right, because the token manager will call it? That's like a constraint. So we need to make it work without such a risky function.

statement you are proposing is that we shouldn't allow developers to implement their own custom interchain tokens because they might build an insecure ones

No, I'm saying there needs to be improvements in the way ITS works with custom tokens to eliminate risks, particularly "superuser" risk on the token issuer's side. i.e. make it work without needing something like burn(address from, uint256 amount) public onlyRole(BURNER_ROLE) in the token contract.

[re1ro]

But if an admin has the power to grant a token manager permission to freely burn, then she'd also have the power to grant herself permission to freely burn, and could go ahead with that any time in the future (e.g. after a salary dispute)

What you are describing has nothing to do with ITS itself. It seems like you are trying to implement a custom token, raising security concerns, and trying to make ITS design responsible for it. A few points here:

• There should be no person in the company having full access to your contract admin functions.
• Admin is a role that should be assigned to a Multisig wallet. Use OpenZeppelin multisig and make sure that you distribute signing accounts securely among a trusted committee within your company or executive board.
• You should probably increase your current admin's salary tomorrow, buy her flowers, and tell her how much you appreciate her working for you.

This is "superuser risk" introduced by adding a new function like burn(address from, uint256 amount) public onlyRole(BURNER_ROLE) as Coinbase explained in their articles.

No, the "superuser risk" is coming from you trying to implement a token with an admin role in it. There should be no admin role in the token to avoid that risk. It has nothing to do with the Token Manager burn permission. You also make the admin able to change token managers, that is the "superuser risk".

The "superuser" is the admin of the token contract who can give herself access to burn other's tokens, an admin could be tempted to grant herself BURNER_ROLE and call that function. That's why Coinbase calls such a burn function in particular risky. Without the risky function, the opportunity for an admin to exploit it wouldn't exist, which is the safest way.

Exactly, there should be no admin or superuser at all to be able to follow Coinbase's recommendations. We don't have a superuser role in our Sample Token example or StandardizedToken. That's why Coinbase doesn't want to have an admin superuser to be able to do anything with the token. As I explained before, the mint function and MINTER_ROLE is even more dangerous because the admin can just mint herself tokens and sell them on Uniswap instead of just destroying them. Again, Coinbase doesn't have a problem with burn permission granted to a fully decentralized contract, they have a problem with your admin role.

No, I'm using OpenZeppelin's ERC20Upgradeable.

And here is the biggest "superuser risk" that makes your token insecure. Your admin can be just tempted to upgrade the token, mint herself tokens, sell them on Uniswap, and then burn everybody else's tokens. And after everything, she can also call a selfdestruct function and destroy the token completely. No one likes Upgradeable ERC20 in general, it's not very trustworthy and usually a red flag.

Yes, I think burnFrom (or transferFrom then burn as done by ChainLink), and not add burn(address from, uint256 amount) public onlyRole(BURNER_ROLE), is the only properly secure solution, but I'm open to other ideas.

Nothing stops you from implementing burn to work like burnFrom right now. As I've already mentioned, we are planning to add TokenManagerMintBurnFrom as well. And you still haven't proved the point of burn itself being insecure. What is the attack vector/scenario and example exploit code? If you get your own admin keys compromised and an attacker gains unauthorized access to your token, it's not an issue with the burn function itself.

But in order for the token to work with ITS currently, I'd need to add the function burn(address from, uint256 amount) public onlyRole(BURNER_ROLE) into the token contract, right, because the token manager will call it? That's like a constraint.

It's a feature, not a constraint. You either trust the security model of ITS or you don't.

You can actually skip implementing the burn function and still be able to send tokens cross-chain using interchainTransfer. But you will not be able to call sendToken and callContractWithInterchainToken on the token manager. So you would lose the ability to do cross-chain calls with your token attached. If it's something that you don't need, feel free to remove the burn function completely.

To summarize

Given all your concerns with implementing a secure custom interchain token, we would recommend you not to do so. You don't have any custom ERC20 functionality (like voting power or fee on transfer) except superuser rights and upgradability, which you don't actually want.

We would recommend you to use deployAndRegisterStandardizedToken on the ITS so your users won't have to establish trust in your custom implementation. StandardizedToken already went through a series of audits and has the trust of the Axelar user base. And it's already based on OpenZeppelin ERC20 if you examine it. So I don't see why you need to reimplement it.

The Token Manager still can have an Operator role that you can use to limit or pause your cross-chain flow. And we would recommend setting up OpenZeppelin multisig for the Operator role.

[SKYBITDev3]

• Admin is a role that should be assigned to a Multisig wallet

Yes, in practice we'd grant DEFAULT_ADMIN_ROLE to a (Gnosis multisig) Safe Wallet.

• You should probably increase your current admin's salary tomorrow, buy her flowers, and tell her how much you appreciate her working for you.

Hehehe maybe a dinner too... it could lead to something :) .

There should be no admin role in the token to avoid that risk

You can't say that for every token - administration may be needed depending on utility.

Token issuers have the right to maintain control of their token contract, and to not grant so much power to external entities. Axelar should still welcome their tokens onto its platforms, regardless of the idealistic and unrealistic notion that all tokens should be adminless.

You also make the admin able to change token managers, that is the "superuser risk"

Token issuers may want the ability to update contract parameters some time in the future due to changing circumstances. If, one day, functionality breaks (e.g. due to an address no longer being valid) then it'd be good if it can be quickly fixed by making an update. Otherwise you could be left with a defunct token, causing loss of confidence amongst holders, and (if you haven't shut the project) the messy task of having all holders do a token swap.

there should be no admin or superuser at all to be able to follow Coinbase's recommendations

They're not saying that there should be no admin at all, but to be careful not to add code that could be exploited, and also that it's better to use a multisig account instead of having only one person as admin.

the mint function and MINTER_ROLE is even more dangerous because the admin can just mint herself tokens

Burned tokens are more noticeable to holders - one day they see the tokens in their wallet, the next day they're gone.

No one likes Upgradeable ERC20 in general

That's untrue. Many platforms use upgradeable contracts, including Axelar. It makes things easier when fixing bugs or security issues or making improvements. It'd be too inconvenient if contract addresses change, especially if the contracts are being used by many people. OpenZeppelin have a suite of contracts and tools related to upgrading that they continue to develop, which would not exist if it really was true that "no one likes" upgradeable contracts: https://docs.openzeppelin.com/upgrades.

Yes, upgradeability increases risk, but granting upgrader role to a multisig account (as Coinbase also suggests) instead of a single person reduces the risk of a malicious upgrade.

But it's not really relevant - things should still work and be secure when the token connects with Axelar, regardless of upgradeability or there being an admin.

we are planning to add TokenManagerMintBurnFrom as well

That's great to hear, I'm eagerly looking forward to it, as I won't be adding burn into our new token.

And you still haven't proved the point of burn itself being insecure. What is the attack vector/scenario and example exploit code?

I thought you would have already understood from my previous posts and also the Coinbase articles. I'll now provide more detail then.

Having either of these functions in the token contract introduces risks for token holders: burn(address from, uint256 amount) public onlyOwner burn(address from, uint256 amount) public onlyRole(BURNER_ROLE)

They are almost the same. Only the contract owner can execute the first one, whilst only an account or contract that had been granted BURNER_ROLE can execute the second.

The first one had been explicitly highlighted by Coinbase as risky because the contract owner could one day decide to burn any holders' tokens simply by calling burn and passing the holder's address and quantity. Though I understand that it may not be applicable in ITS because if a token owner calls transferOwnership(tokenManager) then the owner can no longer do any burning, only the token manager contract instance can. But the (former) owner won't be able to run any other privileged function either, because he has completely lost ownership and control of the contract. I'd never suggest a token issuer to give up ownership, as it's irreversible - you can't take it back even if one day you need to.

The usual alternative to onlyOwner is role-based access control using onlyRole modifier, which does not require giving up contract ownership, so it makes more sense. There would be an admin who can grant roles. For operation with ITS, the token manager would be granted MINTER_ROLE and BURNER_ROLE so that it can freely execute mint and burn in the token.

The exploit could take these steps for example (assuming there is role-based burn):

1. One day the token admin demands a salary raise to 100M USDC, despite getting so many flowers and dinners.
2. Her demand is rejected.
3. She gets very angry about being called a gold digger, so she decides to do sabotage by burning the project's entire token supply.
4. From the blockchain explorer she gets the full list of addresses and quantities of token holdings, in descending quantity order.
5. She writes and runs a hardhat script like this:

   
   const tokenContract = await ethers.getContractAt(tokenContractName, tokenContractAddress)

const BURNER_ROLE = ethers.keccak256(ethers.toUtf8Bytes("BURNER_ROLE")) let txReceipt = tokenContract.grantRole(BURNER_ROLE, adminAddress) await txReceipt.wait() // now I'm allowed to burn any holder's tokens HAHAAHAA!

// BURN IT ALL! await tokenContract.burn(ethers.Typed.address(holderAddress1), quantity1) await tokenContract.burn(ethers.Typed.address(holderAddress2), quantity2) ... await tokenContract.burn(ethers.Typed.address(holderAddress1057457), quantity1057457) await tokenContract.burn(ethers.Typed.address(holderAddress1057458), quantity1057458)

20. The big holders (e.g. exchange wallets holding customers' tokens) notice first that their tokens have been burned, the rest later.
21. The project owners make an announcement deeply apologizing to their communities etc.

If instead there never was `burn(address from, uint256 amount) public onlyRole(BURNER_ROLE)` in the contract then she wouldn't have had any opportunity to do such sabotage, greatly reducing superuser risk. Reducing superuser risk would also depend on other functions and factors, but not adding such a burn function is a big one.

A multisig account being the admin would also reduce some risk, but not by as much as just removing `burn`, because people can collude (which has happened e.g. recently with PEPE).

The list of combinations of *adding `burn`* with *using a multisig admin*, **ordered from unsafe to safest**, would be:
`burn(address from, uint256 amount) public onlyRole(BURNER_ROLE)` added|multisig admin
--|--
true|false
true|true
false|false
false|true

> You don't have any custom ERC20 functionality

That's not true. There's also code for GSN. There's also a `withdraw` function in case any tokens are accidentally sent into the contract, which still happens in the crypto world. That function alone requires privileged (admin) access so that the tokens can be sent back. This goes against your idealistic advice "there should be no admin role in the token", but we care for our users and wouldn't want them to lose funds in our contracts.

[SKYBITDev3]

I love Axelar because I share your cross-chain vision. I think how ITS supports custom tokens is a great idea and I'm extremely supportive and want it to succeed (which is why I spend valuable time and effort to make suggestions). I hope Axelar continues to be inclusive and flexible in accommodating many kinds of custom tokens, regardless of whether the tokens are controlled by admins or are upgradeable, whilst also being mindful of the security concerns on both Axelar's and the users' sides.

I'm very much looking forward to trying your token manager that will call burnFrom.