In some cases TokenManager.giveToken will revert and cause DOS

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-axelar/blob/main/contracts/its/token-manager/TokenManager.sol#L161-L165

Vulnerability details

Impact

InterchainTokenService._processSendTokenPayload/_processSendTokenWithDataPayload is used to process token sending commands. Both will eventually call tokenManager.giveToken to send the token to the receiver. If the token is ERC777, the giveToken may be manipulated by the receiver, which can make the subsequent giveToken revert from other users. This will consume the relayer's gas, which is paid by the user in the source chain.

Proof of Concept

For existing ERC20, TokenManager can be TokenManagerLockUnlock or TokenManagerLiquidityPool. TokenManager.giveToken first calls _giveToken from the child contract to send the token to the receiver, and then calls _addFlowIn to add a flow in amount.

File: contracts\its\token-manager\TokenManager.sol
161:     function giveToken(address destinationAddress, uint256 amount) external onlyService returns (uint256) {
162:         amount = _giveToken(destinationAddress, amount);
163:         _addFlowIn(amount);
164:         return amount;
165:     }

TokenManagerLockUnlock._giveToken and TokenManagerLiquidityPool._giveToken have similar logic. They use the AfterBalance(to)-BeforeBalance(to) mode to return the amount of token transferred. This method is no problem for erc20 without callback. But if the token is ERC777 and to is a malicious contract, the return value of _giveToken can be manipulated.

File: contracts\its\token-manager\implementations\TokenManagerLockUnlock.sol
60:     function _giveToken(address to, uint256 amount) internal override returns (uint256) {
61:         IERC20 token = IERC20(tokenAddress());
62:         uint256 balance = IERC20(token).balanceOf(to);
63: 
64:         SafeTokenTransfer.safeTransfer(token, to, amount);//@audit if token is ERC777, there is callback to 'to' inside this function
65: 
66:         return IERC20(token).balanceOf(to) - balance;
67:     }

The attack flow is as follows:

TokenManager.giveToken	//amount=1e18
  TokenManagerLockUnlock._giveToken
    balance = IERC20(token).balanceOf(to);	//assuming that to's balance is 0.
    SafeTokenTransfer.safeTransfer(token, to, amount);
      to.tokensReceived
        token.transterFrom(EOA, address(this), 100000e18); //EOA is wallet from attacker.
    return IERC20(token).balanceOf(to) - balance; //return value is 100001e18.
   _addFlowIn(amount)	//here amount is 100001e18.

If the current flow limit is not 0, then if the amount returned by _giveToken just meets this condition, that is, flowToAdd + flowAmount is close to flowToCompare + flowLimit. flowAmount is the amount returned by _giveToken. In this way, the subsequent _addFlowIn will revert because the condition is not met.

However, _addFlowIn internally reads FlowInSlot and FlowOutSlot by epoch, and the interval between each epoch is EPOCH_TIME (six hours). 4 epochs a day. The attacker can send tokens (very small amount) across chains every 6 hours without paying gas, then the relayer will not call such a transaction. In this way, when a new epoch arrives, the attacker calls InterchainTokenService.execute to trigger _processSendTokenPayload. Make subsequent calls to revert by attack flow mentioned above.

Tools Used

Manual Review

Recommended Mitigation Steps

For TokenManagerLockUnlock:

File: contracts\its\token-manager\implementations\TokenManagerLockUnlock.sol
60:     function _giveToken(address to, uint256 amount) internal override returns (uint256) {
61:         IERC20 token = IERC20(tokenAddress());
62:-        uint256 balance = IERC20(token).balanceOf(to);
62:+	    uint256 balance = IERC20(token).balanceOf(address(this));
63: 
64:         SafeTokenTransfer.safeTransfer(token, to, amount);
65: 
66:-        return IERC20(token).balanceOf(to) - balance;
66:+        return balance - IERC20(token).balanceOf(address(this);
67:     }

For TokenManagerLiquidityPool:

File: contracts\its\token-manager\implementations\TokenManagerLiquidityPool.sol
094:     function _giveToken(address to, uint256 amount) internal override returns (uint256) {
095:         IERC20 token = IERC20(tokenAddress());
096:-        uint256 balance = IERC20(token).balanceOf(to);
096:+        uint256 balance = IERC20(token).balanceOf(liquidityPool());
097: 
098:         SafeTokenTransferFrom.safeTransferFrom(token, liquidityPool(), to, amount);
099: 
100:-        return IERC20(token).balanceOf(to) - balance;
100:+        return balance - IERC20(token).balanceOf(liquidityPool());
101:     }

Assessed type

DoS

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #317

[c4-judge]

berndartmueller marked the issue as not a duplicate

[c4-judge]

berndartmueller marked the issue as duplicate of #345

[c4-judge]

berndartmueller marked the issue as satisfactory

[c4-judge]

berndartmueller marked the issue as not a duplicate

[c4-judge]

berndartmueller marked the issue as duplicate of #332