We shouldn't have to write risky code to use the Interchain Token Service

SKYBITDev3 · September 23, 2023, 6:00pm

It appears that Axelar staff are not taking the security issue that I had highlighted last month:

github com/axelarnetwork/interchain-token-service/issues/101

(This forum platform doesn’t let me post links, I get the error: “Sorry you cannot post a link to that host.”)

The issue is big enough that Coinbase has published at least 4 articles that cover this kind of issue (“superuser risk”) within the past year:

How Coinbase Protects Users From Risky Assets
coinbase com/blog/how-coinbase-protects-users-from-risky-assets

How Coinbase reviews tokens on Ethereum for technical security risks
coinbase com/blog/how-coinbase-reviews-tokens-on-ethereum-for-technical-security-risks

Security PSA: Protecting ERC-20 assets from malicious actors
coinbase com/blog/security-psa-protecting-erc-20-assets-from-malicious-actors

Token Custody Risks: Defining Security in the Crypto World
coinbase com/blog/token-custody-risks-defining-security-in-the-crypto-world

In the third one I’ve listed above, they even explicitly show this code:

… which looks almost exactly like the sample code that Axelar has published:

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

Source: github com/axelarnetwork/axelar-docs/blob/5c2cb34c424a0c28f2dafff24f04c08a0ca4f6e8/public/samples/interchain-token-iinterchaintoken.sol#L41-L43

I’m aware that in the case of the Interchain Token Service, the suggestion is to transfer ownership of the contract to the token manager so that it can freely burn and mint the tokens. But that would mean token issuers would completely lose control of the contract, so they’d be against such a crazy suggestion. So in a separate GitHub issue about that I suggested replacing onlyOwner with onlyRole (function burn(address from, uint256 amount) external onlyRole(BURNER_ROLE)) , assigning the token manager burner role and minter role. But that is still insecure because an admin of the contract could one day assign the burner role to herself and burn everyone’s tokens.

The proper way to do it is instead of having token issuers add such a risky burn function, request approval from the token holder (by calling permit or approve) and call burnFrom or transferFrom when executing an interchain transfer. That’s how Chainlink’s Cross-Chain Interoperability Protocol (CCIP) does it, as I’ve explained with code references in the GitHub issue.

The risk for Axelar is the possibility that one or more tokens that contains your suggested burn function gets compromised by an admin of the token, causing loss of others’ tokens. The blame would go back to Axelar for condoning such risky code and dismissing the concerns that are being raised in this present time, causing reputational damage, loss of AXL token value, etc… I really hope that would never happen.

Luckily the Interchain Token Service is not yet live on mainnets, so @sergey please correct this and any other critical issues while you can do so more easily than if it was in production.

Kiryl · September 24, 2023, 1:54pm

This report is cross-posted from the original issue on github:

github.com/axelarnetwork/interchain-token-service

Enhance security related to burning

opened 06:26AM - 21 Aug 23 UTC

SKYBITDev3

Granting permission to the token manager to freely burn anyone's tokens via `bur…n(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)](https://docs.openzeppelin.com/contracts/4.x/api/token/erc20#ERC20Burnable-burnFrom-address-uint256-) 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: <pre> contract MyInterchainToken is ERC20, <b>ERC20Burnable</b>, 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); } <b>// Removed: function burn(address from, uint256 amount)</b> 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 ); } } </pre>

The report was qualified as invalid due to several reasons:

Mentioned code is not a part of the Interchain Token Service itself but rather a simplified example code from the documentation about how to create custom tokens.
Any of the “superuser” risks described above are not applicable as onlyOwner role in this example is granted to the ITS TokenManager contact and not any EVM user account.
There is nothing stopping developers from implementing approve + burnFrom functionality in their custom interchain tokens if they choose so.

For complete Interchain Token implementation you can always use StandardizedToken as a reference.

SKYBITDev3 · September 24, 2023, 7:05pm

If onlyOwner is replaced with onlyRole(BURNER_ROLE) (if the token issuer doesn’‘t want to give up ownership) then whoever manages roles in the token contract (e.g. token contract admin) is the “superuser”, and could grant herself BURNER_ROLE, then go onto burn others’ tokens. That is definitely a risk, so the warnings that both I and Coinbase have made are valid in this case.

It’s the token manager that does the burning. Currently it calls the risky function burn. Which means we are forced to include that risky function in order to be able to use ITS. I’m saying that there needs to be a way to do it without the risky function, e.g. have token manager call burnFrom or transferFrom (after a permit or approve). Token developers can imoprt ERC20Burnable to get burnFrom but it’s up to Axelar to make the token manager use it.

Developers will copy example code that Axelar has published, so it should be good quality and without risky code.

Kiryl · September 25, 2023, 4:42am

The purpose of Sample Token example is to showcase integration point with the token manager. Not give developers a copy-paste solution. In our example onlyOwner is only for mint + burn role granted to the Token Manager. There is no ownership. It’s not a contract admin and there is no account holding this role. Token Manager can’t transfer that permission to anyone else.

If we include the whole Open Zeppeling framework in our example code it will make understanding base ITS concepts even harder. It is developer responsibility to know what are they doing or just use a Standard Interchain Token provided by the service. Giving a different example for a copy-paste will not prevent unexperienced developers from introducing security problems.

From our github conversation the problem is with your custom token implementation where you also have ADMIN_ROLE that can do too many dangerous things. This is the “superuser” risk that you are introducing in your token. Our simple example doesn’t have that “risky code” in it.

approve + burnFrom in our example will not improve the security model anyhow, but will make users pay extra gas for every cross-chain transaction. If you have self-sabotaging admins in your token, then yeah you can make your burn function to work like burnFrom and require approve. I bet it won’t help with anything as admin can still destroy your token in many ways without any burn function.

Please stop posting the same self-contradicting statements, they do not make sense at this point. If you still have concerns or need help with implementing your custom token please schedule a call with me using the provided link.