Axelar Network v2 contest - horsefacts's results

Low

Proxy setup may fail silently

Since low level calls do not check for contract existence, and return true if a contract does not exist, it's possible for Axelar proxy contracts that delegatecall to a setup function to fail silently if their implementationAddress does not exist.

util/Proxy#init:

        // solhint-disable-next-line avoid-low-level-calls
        (bool success, ) = implementationAddress.delegatecall(
            //0x9ded06df is the setup selector.
            abi.encodeWithSelector(0x9ded06df, params)
        );
        if (!success) revert SetupFailed();

xc20 Proxy#constructor:

        // solhint-disable-next-line avoid-low-level-calls
        (bool success, ) = implementationAddress.delegatecall(
            //0x9ded06df is the setup selector.
            abi.encodeWithSelector(0x9ded06df, params)
        );
        if (!success) revert SetupFailed();

Suggestion: Check address.code.length for implementationAddress. For example:

        if (implementationAddress.code.length == 0) revert ImplementationDoesNotExist();
        // solhint-disable-next-line avoid-low-level-calls
        (bool success, ) = implementationAddress.delegatecall(
            //0x9ded06df is the setup selector.
            abi.encodeWithSelector(0x9ded06df, params)
        );
        if (!success) revert SetupFailed();

Gateway may be incompatible with rebasing tokens

In addition to fee-on-transfer tokens (see this finding from the previous C4 contest), the Axelar gateway is not fully compatible with ERC20 tokens that increase an account's underlying balance, like Lido stETH and Aave aTokens. Due to rebases, the gateway contract may accrue a balance greater than the token amount minted on a destination chain, and these excess amounts could remain locked in the gateway. There is not really a reasonable technical solution here: instead it's probably best to document this incompatibility for end users of the protocol if token creation becomes permissionless, or avoid these tokens if it is permissioned.

Fixed chain ID in domain separator

The ERC20Permit contract generates and permanently stores an EIP-712 domain separator using block.chainid at construction time:

ERC20Permit#constructor

    constructor(string memory name) {
        DOMAIN_SEPARATOR = keccak256(
            abi.encode(DOMAIN_TYPE_SIGNATURE_HASH, keccak256(bytes(name)), keccak256(bytes('1')), block.chainid, address(this))
        );
    }

ERC20Permit#constructor

    constructor(string memory name) {
        DOMAIN_SEPARATOR = keccak256(
            abi.encode(DOMAIN_TYPE_SIGNATURE_HASH, keccak256(bytes(name)), keccak256(bytes('1')), block.chainid, address(this))
        );
    }

Since chainid is set permanently, there is a risk of replay attacks in the event of a hard fork: since contracts on both forks would use the same stored chainid, signatures on one chain would be replayable on the fork. This is a pretty low probablity scenario, but as a cross-chain protocol supporting many EVM chains, Axelar is probably at higher risk of exposure to a hard fork than a typical single chain application.

Suggestion: cache chainId and DOMAIN_SEPARATOR at construction time. Use the cached DOMAIN_SEPARATOR unless chainID has changed. If so, regenerate DOMAIN_SEPARATOR.

QA

Extract a SafeTransfer library

A similar ERC20 _safeTransfer function is defined twice, in both DepositBase and AxelarGasService. Consider extracting this function to a shared library. (Note that the version in AxelarGasService includes a zero amount check that should be moved outside the function if you do implement this refactor).

DepositBase#_safeTransfer:

    function _safeTransfer(
        address tokenAddress,
        address receiver,
        uint256 amount
    ) internal {
        // solhint-disable-next-line avoid-low-level-calls
        (bool success, bytes memory returnData) = tokenAddress.call(abi.encodeWithSelector(IERC20.transfer.selector, receiver, amount));
        bool transferred = success && (returnData.length == uint256(0) || abi.decode(returnData, (bool)));

        if (!transferred || tokenAddress.code.length == 0) revert TokenTransferFailed();
    }

AxelarGasService#_safeTransfer

    function _safeTransfer(
        address tokenAddress,
        address receiver,
        uint256 amount
    ) internal {
        if (amount == 0) revert NothingReceived();

        // solhint-disable-next-line avoid-low-level-calls
        (bool success, bytes memory returnData) = tokenAddress.call(abi.encodeWithSelector(IERC20.transfer.selector, receiver, amount));
        bool transferred = success && (returnData.length == uint256(0) || abi.decode(returnData, (bool)));

        if (!transferred || tokenAddress.code.length == 0) revert TransferFailed();
    }

Note that a similar function exists in the XC20 contract as well, although it may make sense to keep it isolated:

XC20Wrapper.sol#_safeTransfer

    function _safeTransfer(
        address tokenAddress,
        address receiver,
        uint256 amount
    ) internal {
        (bool success, bytes memory returnData) = tokenAddress.call(abi.encodeWithSelector(IERC20.transfer.selector, receiver, amount));
        bool transferred = success && (returnData.length == uint256(0) || abi.decode(returnData, (bool)));

        if (!transferred || tokenAddress.code.length == 0) revert('TransferFailed()');
    }