code-423n4 / 2023-07-axelar-findings

2 stars 0 forks source link

```InterchainTokenService#registerCanonicalToken``` GatewayToken check can be bypassed in several ways #424

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-axelar/blob/main/contracts/cgp/AxelarGateway.sol#L385 https://github.com/code-423n4/2023-07-axelar/blob/main/contracts/its/interchain-token-service/InterchainTokenService.sol#L309

Vulnerability details

Impact

Allows any developer to register a token that is already registered with the gateway.

Vulnerability Details

In order to register a token as canonical, the registerCanonicalToken function is called and passed an address. The symbol of the passed token is checked to make sure it hasn't already been registered on the gateway before being deployed.

    function registerCanonicalToken(address tokenAddress) external payable notPaused returns (bytes32 tokenId) {
        (, string memory tokenSymbol, ) = _validateToken(tokenAddress);

        //@audit
        if (gateway.tokenAddresses(tokenSymbol) == tokenAddress) revert GatewayToken();

         tokenId = getCanonicalTokenId(tokenAddress);
        _deployTokenManager(tokenId, TokenManagerType.LOCK_UNLOCK, abi.encode(address(this).toBytes(), tokenAddress));
    }

The check can be bypassed in 2 different ways:

  1. By simply calling registerCanonicalToken before AxelarGateway#deployToken is called. Whether done by frontrunning or not. This can be done since the deployment is done with create2
  2. If the tokenSymbol registered to the gateway doesn't match the actual token at the address. This works for external tokens deployed on AxelarGateway#deployToken because the function doesn't check whether the passed symbol actually is the symbol of the token at the address.

    
    function deployToken(bytes calldata params, bytes32) external onlySelf {
        (string memory name, string memory symbol, uint8 decimals, uint256 cap, address tokenAddress, uint256 mintLimit) = abi.decode(
            params,
            (string, string, uint8, uint256, address, uint256)
        );
    
        //...
    
        if (tokenAddress == address(0)) {
    
            //..
    
        } else {
            if (tokenAddress.code.length == uint256(0)) revert TokenContractDoesNotExist(tokenAddress);
    
            // Mark that this symbol is an external token, which is needed to differentiate between operations on mint and burn.
            _setTokenType(symbol, TokenType.External);
    
            //@audit here should verify that symbol == ERC20(tokenAddress).symbol()
        }
    
        _setTokenAddress(symbol, tokenAddress);
        _setTokenMintLimit(symbol, mintLimit);
    
        emit TokenDeployed(symbol, tokenAddress);
## Proof of Concept
POC for (1)
Place the following test inside tokenService.js "Register Canonical Token" tests.
    it('Canonical token registeration bypass through frontrunning', async () => {
        //Call register canonical token first, can be done by frontrunning
        await expect(service.registerCanonicalToken(token.address));
        //then register the token on the gateway
        await (await gateway.setTokenAddress(tokenSymbol, token.address)).wait();
    });


## Tools Used
Manual Review
## Recommended Mitigation Steps
Inside AxelarGateway, you should both verify that the token isn't already deployed as a canonical token and the deployToken should verify symbol == ERC20(tokenAddress).symbol(). 

## Assessed type

Invalid Validation
c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as primary issue

c4-sponsor commented 1 year ago

deanamiel marked the issue as disagree with severity

deanamiel commented 1 year ago

Corrected Severity: QA The first case proposed could be possible, although the Axelar Gateway is not planning to add tokens any time soon, and it will migrate existing ones to the ITS in the future. All existing tokens have symbols that match the gateway symbols. Even if a gateway token is registered, there would not be any issues, except migrating tokens would become more difficult in the future.

berndartmueller commented 1 year ago

Agree with sponsor. Downgrading to QA (Low).

c4-judge commented 1 year ago

berndartmueller changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

berndartmueller marked the issue as grade-c