TokenManager's flow limit logic is broken for ERC777 tokens

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-axelar/blob/2f9b234bb8222d5fbe934beafede56bfb4522641/contracts/its/token-manager/implementations/TokenManagerLockUnlock.sol#L60-L67
https://github.com/code-423n4/2023-07-axelar/blob/2f9b234bb8222d5fbe934beafede56bfb4522641/contracts/its/token-manager/implementations/TokenManagerLiquidityPool.sol#L94-L101

Vulnerability details

TokenManager implementations inherit from the FlowLimit contract that keeps track of flow in and flow out, and if these two values are too far away from each other, it reverts:

    function _addFlow(
        uint256 flowLimit,
        uint256 slotToAdd,
        uint256 slotToCompare,
        uint256 flowAmount
    ) internal {
        uint256 flowToAdd;
        uint256 flowToCompare;
        assembly {
            flowToAdd := sload(slotToAdd)
            flowToCompare := sload(slotToCompare)
        }
        if (flowToAdd + flowAmount > flowToCompare + flowLimit) revert FlowLimitExceeded();
        assembly {
            sstore(slotToAdd, add(flowToAdd, flowAmount))
        }
    }

Flow in and flow out are increased when some tokens are transferred from one blockchain to another.
There are 3 different kinds of TokenMaganer:

• Lock/Unlock
• Mint/Burn
• Liquidity Pool

Let's see how Lock/Unlock and Liquidity Pool implementations handle cases when they have to transfer tokens to users:

function _giveToken(address to, uint256 amount) internal override returns (uint256) {
        IERC20 token = IERC20(tokenAddress());
        uint256 balance = IERC20(token).balanceOf(to);

        SafeTokenTransfer.safeTransfer(token, to, amount);

        return IERC20(token).balanceOf(to) - balance;
    }

function _giveToken(address to, uint256 amount) internal override returns (uint256) {
        IERC20 token = IERC20(tokenAddress());
        uint256 balance = IERC20(token).balanceOf(to);

        SafeTokenTransferFrom.safeTransferFrom(token, liquidityPool(), to, amount);

        return IERC20(token).balanceOf(to) - balance;
    }

As can be seen, they return a value equal to a balance difference before and after token transfer. This returned value is subsequently used by the giveToken function in order to call _addFlowIn:

function giveToken(address destinationAddress, uint256 amount) external onlyService returns (uint256) {
        amount = _giveToken(destinationAddress, amount);
        _addFlowIn(amount);
        return amount;
    }

The problem arises when token is ERC777 token. In that case, token receiver can manipulate its balance in order to increase flow in more than it should be - see POC section for more details.

There is no mechanism that will disallow someone from creating TokenManager with ERC777 token as an underlying token, so it's definitely a possible scenario and the protocol would malfunction if it happens.

Note that it's not an issue like "users may deploy TokenManager for their malicious tokens that could even lie about balanceOf" - users may simply want to deploy TokenManager for some ERC777 token and bridge their ERC777 tokens and there is nothing unusual about it.

Impact

It is possible to manipulate flow in when there is TokenManager of type Lock/Unlock or Liquidity Pool and the underlying token is ERC777 token. It could be used to create DoS attacks as it won't be possible to transfer tokens from one blockchain to another when the flow limit is reached (it may be possible to send them from one blockchain, but it will be impossible to receive them on another one due to the revert in the _addFlow function).

In order to recover, flowLimit could be set to 0, but the feature was introduced in order to control flow in and flow out and setting flowLimit to 0 means that the protocol won't be able to control it anymore.

Here, the availability of the protocol is impacted, but an extra requirement is that there has to be TokenManager of Lock/Unlock or Liquidity Pool kind with ERC777 underlying token, so I'm submitting this issue as Medium.

Proof of Concept

Consider the following scenario:

1. TokenManager of type Lock/Unlock was deployed on blockchain X with underlying ERC777 token (like FLUX, for example) - let's call this token T.
2. Assume that blockchain X has low gas price (not strictly necessary, but will be helpful to visualise the issue).
3. Alice wants to move her tokens (T) from blockchain Y to X, so she calls sendToken from TokenManagerLockUnlock in order to start the process.
4. Bob sees that and he also calls sendToken but with some dust amount of token T, but he schedules that transaction from his smart contract called MaliciousContract.
5. Bob's transaction is handled first and finally TokenManagerLockUnlock::_giveToken is called in order to give that dust amount of T to Bob's contract (MaliciousContract).
6. _giveToken:

function _giveToken(address to, uint256 amount) internal override returns (uint256) {
        IERC20 token = IERC20(tokenAddress());
        uint256 balance = IERC20(token).balanceOf(to);

        SafeTokenTransfer.safeTransfer(token, to, amount);

        return IERC20(token).balanceOf(to) - balance;
    }

first records current balance of T of MaliciousContract, which happens to be 0 and calls transfer, which finally calls MaliciousContract::tokensReceived hook.

7. MaliciousContract::tokensReceived hook looks as follows:

function tokensReceived(
    address operator,
    address from,
    address to,
    uint256 amount,
    bytes calldata data,
    bytes calldata operatorData
) external
{
    if (msg.sender == <TOKEN_MANAGER_ADDRESS>)
        maliciousContractHelper.sendMeT();
    else
        return;
}

, where <TOKEN_MANAGER_ADDRESS> is the address of the relevant TokenManager and maliciousContractHelper is an instance of MaliciousContractHelper, that exposes sendMeT function which will send all tokens that it has to the MaliciousContract instance that called it.

8. maliciousContractHelper has a lot of tokens T, so when tokensReceived returns, T.balanceOf(MaliciousContract) will increase a lot despite the fact that only a dust amount of T was sent from TokenManager.
9. Execution will return to _giveToken and returned value will be huge, since IERC20(token).balanceOf(to) - balance will now be a big value, despite the fact that amount was close to 0.
10. Flow in amount will increase a lot, so that flowLimit is reached and Alice's transaction will not be processed.

In short, Bob has increased the flow in amount for TokenManager by sending to his contract a lot of money in ERC777 tokensReceived hook from his another contract. It didn't cost him much since he sent only tiny amount of T between blockchains - hence he could use almost all of his T tokens for the attack.

Of course Bob could perform this attack without waiting for Alice to submit her transaction - scenario presented above was just an example. In reality, Bob can do this at any moment.

It also seems possible to transfer tokens from the same blockchain to itself (by specifying wrong destinationChain in sendToken or just by specifying destinationChain = <CURRENT_CHAIN>), so Bob can have his tokens T only on one blockchain.

If gas price on that blockchain is low, Bob can perform that attack a lot of times - all he needs to do is to send tokens back to MaliciousContractHelper after each attack (so that it can send it to MaliciousContract as described above). Finally, he will reach flowLimit for TokenManager and will cause denial of service.

Tools Used

VS Code

Recommended Mitigation Steps

I would recommend doing one of the following:

• acknowledge the issue and warn users that the protocol doesn't support ERC777 tokens (possibly even check and if TokenManager with ERC777 underlying token is to be deployed - just revert)
• correct the value returned by _giveTokens to ensure that it doesn't exceed amount, as follows:

uint currentBalance = IERC20(token).balanceOf(to);
if (currentBalance - balance > amount)
    return amount;
return currentBalance - balance;

Assessed type

Other

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

deanamiel marked the issue as sponsor confirmed

[deanamiel]

Fixed so that the amount returned can never be higher than the initial amount.
Public PR link:
axelarnetwork/interchain-token-service#102

[c4-judge]

berndartmueller marked the issue as selected for report