ERC777 and similar token implementations allow stealing of funds when transferring tokens

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-axelar/blob/2f9b234bb8222d5fbe934beafede56bfb4522641/contracts/its/token-manager/implementations/TokenManagerLiquidityPool.sol#L82-L85 https://github.com/code-423n4/2023-07-axelar/blob/2f9b234bb8222d5fbe934beafede56bfb4522641/contracts/its/token-manager/implementations/TokenManagerLockUnlock.sol#L48-L51

Vulnerability details

Impact

A malicious actor can trick a TokenManager into thinking that a bigger amount of tokens was transferred. On the destination chain, the malicious actor will be able to receive more tokens than they sent on the source chain.

Proof of Concept

TokenManagerLockUnlock and TokenManagerLiquidityPool are TokenManager implementations that transfer tokens from/to users when sending tokens cross-chain. The low-level _takeToken function (TokenManagerLiquidityPool._takeToken, TokenManagerLockUnlock._takeToken) is used to take tokens from a user on the source chain before emitting a cross-chain message, e.g. via the TokenManager.sendToken function. The function computes the difference in the balance of the liquidity pool or the token manager before and after the transfer, to track the actual amount of tokens transferred. The amount is then passed in the cross-chain message to tell the InterchainTokenService contract on the destination chain how much tokens to give to the recipient.

The _takeToken function, however, is not protected from re-entrancies, which opens up the following attack scenario:

1. A malicious contract initiates transferring of 100 ERC777 tokens by calling TokenManager.sendToken.
2. The _takeToken function calls transferFrom on the ERC777 token contract, which calls the tokensToSend hook on the malicious contract (the sender).
3. In the hook, the malicious contract makes another call to TokenManager.sendToken and sends 100 more tokens.
4. In the nested _takeToken call, the balance change will equal 100 since, in ERC777, the balance state is updated only after the tokensToSend hook, so only the re-entered token transfer will be counted.
5. The re-entered call to TokenManager.sendToken will result in 100 tokens transferred cross-chain.
6. In the first _takeToken call, the balance change will equal 200 because the balance of the receiver will increase twice during the transferFrom call: once for the first call and once for the re-entered call.
7. As a result, the malicious contract will transfer 100+100 = 200 tokens, but the TokenManager contract will emit two cross-chain messages: one will transfer 100 tokens (the re-entered call) and the other will transfer 200 tokens (the first call). This will let the malicious actor to receive 300 tokens on the destination chain, while spending only 200 tokens on the source chain.

Since the protocol is expected to support different implementations of ERC20 tokens, including custom ones, the attack scenario is valid for any token implementation that uses hooks during transfers.

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding a re-entrancy protection to the TokenManagerLiquidityPool._takeToken and TokenManagerLockUnlock._takeToken functions, for example by using the ReentrancyGuard from OpenZeppelin.

Assessed type

Reentrancy

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

deanamiel marked the issue as sponsor confirmed

[deanamiel]

We have added a separate token manager for fee on transfer tokens, which is protected from reentrancy. Here is a link to the public PR: https://github.com/axelarnetwork/interchain-token-service/pull/96

[c4-judge]

berndartmueller marked the issue as selected for report