code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

There are inconsistencies in the token transfer accounting between expressReceiveTokenWithData and _processSendTokenWithDataPayload which can be gamed by any user for fee-on-transfer tokens. Note that this is not an issue with the actual token transfer accounting for fee-on-transfer tokens, which is fine, but rather with the function call which happens after and utilizes the accounting information provided by the token transfer. Since it's clear that the protocol is intended to support fee-on-transfer tokens, as transfer functionality has been designed around supporting them, this is a non-trivial issue.

The inconsistency lies in that the amount value which is used in the function call in expressReceiveTokenWithData does not include the fee which has been taken out following the transfer. However, in _processSendTokenWithDataPayload, the amount value includes the fee having been subtracted out. This is a fundamental issue if/when the call to executeWithInterchainToken/expressExecuteWithInterchainToken updates some core logic based on this amount.

Proof of Concept

The expressReceiveTokenWithData function is defined as follows:

function expressReceiveTokenWithData(
    bytes32 tokenId,
    string memory sourceChain,
    bytes memory sourceAddress,
    address destinationAddress,
    uint256 amount,
    bytes calldata data,
    bytes32 commandId
) external {
    ...
    SafeTokenTransferFrom.safeTransferFrom(token, caller, destinationAddress, amount);

    _expressExecuteWithInterchainTokenToken(tokenId, destinationAddress, sourceChain, sourceAddress, data, amount);
    ...
}

The amount value which is fed into the _expressExecuteWithInterchainTokenToken call does not include the fee which is taken out after a transfer. This internal function is then simply a wrapper for the call to expressExecuteWithInterchainToken.

The _processSendTokenWithDataPayload function is defined as follows:

function _processSendTokenWithDataPayload(string calldata sourceChain, bytes calldata payload) internal {
    ...
    {
        address expressCaller = _popExpressReceiveTokenWithData(
            tokenId,
            sourceChain,
            sourceAddress,
            destinationAddress,
            amount,
            data,
            commandId
        );
        if (expressCaller != address(0)) {
            amount = tokenManager.giveToken(expressCaller, amount);
            return;
        }
    }
    amount = tokenManager.giveToken(destinationAddress, amount);
    IInterchainTokenExpressExecutable(destinationAddress).executeWithInterchainToken(sourceChain, sourceAddress, data, tokenId, amount);
    ...
}

As can be seen, when there is no expressCaller, the amount is the output of tokenManager.giveToken(..). This call returns the actual outputted amount of the transfer (based on subtracting the end balance from the beginning balance).

Therefore, there is a difference in the amount value between these two calls, which any user can decide to game based on what suits their personal interests (any user can be an expressCaller).

Tools Used

Manual review

Recommended Mitigation Steps

In the expressReceiveTokenWithData function, set the amount to be the difference in the balance of the destinationAddress before and after the safeTransferFrom. This is what should then be fed into _expressExecuteWithInterchainTokenToken.

Assessed type

ERC20

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 sponsor disputed

deanamiel commented 1 year ago

Express calls will not be supported for fee-on-transfer tokens, because of such issues.

berndartmueller commented 1 year ago

The submission lacks a demonstration of a specific attack scenario or the harm that can be caused. Ultimately, it depends on the internal logic of the expressExecuteWithInterchainToken function, which receives the "incorrect" amount parameter.

Looking at C4's judging criteria, medium severity is defined as:

Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

I see the leak of value not within the Axelar protocol. Instead, the integrator contract would be affected. As Axelar will not officially support such Fee-on-transfer tokens, I see the fault on the integrator side. Thus, I'm downgrading to QA (Low).

c4-judge commented 1 year ago

berndartmueller changed the severity to QA (Quality Assurance)