Users will be denied from using a particular protocol functionality under a certain case

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L304-L389

Vulnerability details

Impact

Users will be denied from using the swapping functionality under a certain case

Proof of Concept

Users can call depositWithExpiry() with a memo of their choice signalizing the intent of their deposit, for example swapping. Then, the function responsible for handling their intent gets called after scanning the event and confirming the validity of the transaction. However, there is a particular scenario that would DoS the users from using the swapping functionality and lock their tokens until the owner calls rescueFunds() in THORChain_Aggregator.sol.

This is part of the _transferOutAndCallV5() function responsible for swapping (just the else statement that is responsible for token to token swap, also ignored the event emission):

_vaultAllowance[msg.sender][aggregationPayload.fromAsset] -= aggregationPayload.fromAmount;

      (bool transferSuccess, bytes memory data) = aggregationPayload.fromAsset.call(
          abi.encodeWithSignature(
            "transfer(address,uint256)",
            aggregationPayload.target,
            aggregationPayload.fromAmount
          )
        );

      require(
        transferSuccess && (data.length == 0 || abi.decode(data, (bool))),
        "Failed to transfer token before dex agg call"
      );

      (bool _dexAggSuccess, ) = aggregationPayload.target.call{value: 0}(
        abi.encodeWithSignature(
          "swapOutV5(address,uint256,address,address,uint256,bytes,string)",
          aggregationPayload.fromAsset,
          aggregationPayload.fromAmount,
          aggregationPayload.toAsset,
          aggregationPayload.recipient,
          aggregationPayload.amountOutMin,
          aggregationPayload.payload,
          aggregationPayload.originAddress
        )
      );
}

First, the vault allowance gets decremented and the tokens are transferred to the target contract. Then, swapOutV5() gets called on the target contract. Here is a part the current implementation for it:

      path[0] = fromAsset;
      path[1] = WETH;
      safeApprove(fromAsset, address(swapRouter), 0); // best practice
      safeApprove(fromAsset, address(swapRouter), fromAmount);

      if (payload.length == 0) {
        // no payload, process without wasting gas
        // UniV2 transfers from 'msg.sender', so we need a low-level `.call` to change the msg.sender from the target's perspective
        (bool aggSuccess, ) = address(swapRouter).call(
          abi.encodeWithSignature(
            "swapExactTokensForETH(uint256,uint256,address[],address,uint256)",
            fromAmount,
            amountOutMin,
            path,
            recipient,
            type(uint).max // Assuming using the maximum uint256 value as the deadline
          )
        );

It swaps the tokens for ETH. The issue is that the path array is set in a way that assumes that there is a direct pair between WETH and the token. If there isn't, the path array should have intermediary tokens to swap between in order to achieve the intended swap to WETH. Source: https://docs.uniswap.org/contracts/v2/reference/smart-contracts/router-02#swapexacttokensforeth

The current implementation will cause the transaction to revert if a whitelisted token does not have a Uniswap pool with WETH or if it is not supported by Uniswap at all. Such tokens exist, one of them is 0xAa6E8127831c9DE45ae56bB1b0d4D4Da6e5665BD, the token is whitelisted (https://gitlab.com/thorchain/thornode/-/blob/develop/common/tokenlist/ethtokens/eth_mainnet_latest.json?ref_type=heads) but is not supported by Uniswap and doesn't have a pool with WETH. This causes a DoS for users and locks their tokens in THORChain_Aggregator.sol until an owner calls rescueFunds() and rescues their tokens.

Tools Used

Manual Review

Recommended Mitigation Steps

Remove tokens that do not have a pool between them and WETH and tokens that are not available on Uniswap. Another option is to not hardcode the path array but that wouldn't be trivial to do.

Assessed type

Token-Transfer

[the-eridanus]

Adding label: sponsor acknowledged. This issue is valid in that it is a possible scenario, but the issue would be an integration (e.g. interface or wallet) + aggregator error. If the interface or wallet allows the user to create a swap that leads to a swapOut call with a token that doesn't have the appropriate Uniswap pool, this would be a mistake of the integrator, not the protocol. The responsibility is on the aggregator/integration to ensure proper use of the system.

[trust1995]

@the-eridanus what would be the alternative way out in case user is using whitelisted tokens without a path?

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as selected for report

[c4-judge]

trust1995 changed the severity to 2 (Med Risk)

[AsmundTC]

There is a large category of actions that can be taken above the protocol level that the protocol cannot either technically or practically guard against that would lead to loss of funds. Even if every frontend made sure to check that a trading pair was valid against the target aggregator before building a txn, there is nothing to stop an end user from crafting such a transaction by hand and submitting. An even more direct example doesn't involve any aggregators or contract calls: a user swaps to any infinite number of "dead" addresses that swallow funds: =:ETH.ETH:0x...dEaD

[trust1995]

The frontend is of no interest in contract security. The contracts should do the minimum necessary to warn OR prevent users from using them in a harmful way.

Can you explain whether user using whitelisted tokens with the router without a swap pair considered a reckless action? Is there a documentation or other warning that we're missing?

[TradMod]

Hey @trust1995 Judge Sahab!

The issue is that the path array is set in a way that assumes that there is a direct pair between WETH and the token

I believe this is a OOS issue. It points a vulnerability which exists in the Aggregator contract, which was not in-scope of this contest. Also the THORChain_Aggregator contract is not a real contract, it just an example implementation, I confirmed this with the sponsors during the contest. Pls @the-eridanus sir, confirm that here too. (I asked that in the private thread, I can post the screenshots If you want, Judge Sahab)

Even if this issue is considered as In-scope issue, I believe it is informational or a low issue at best, as the sponsor explained:

the issue would be an integration (e.g. interface or wallet) + aggregator error

[trust1995]

Thanks @ShaheenRehman for pointing this out.

The issue is that the path array is set in a way that assumes that there is a direct pair between WETH and the token. If there isn't, the path array should have intermediary tokens to swap between in order to achieve the intended swap to WETH.

This clearly shows the root cause is in the OOS contract, making it invalid.

[c4-judge]

trust1995 marked the issue as unsatisfactory: Out of scope

[samuraii77]

Hello, while I agree that the rules state that contracts that compose of OOS contracts and the root cause for the issue is in the OOS contract should be invalid, it also states that a judge can make his own decision based on the exact context of the issue. Thus, I believe this issue should be classified as a medium due to the impact and likelihood of it occurring. It will occur every single time the scenario explained in my issue above occurs and the impact is clear and debatably, even high, as the funds will be locked for an unknown amount of time. Please reconsider the severity of this issue. @trust1995

[the-eridanus]

Hey @trust1995 Judge Sahab!

The issue is that the path array is set in a way that assumes that there is a direct pair between WETH and the token

I believe this is a OOS issue. It points a vulnerability which exists in the Aggregator contract, which was not in-scope of this contest. Also the THORChain_Aggregator contract is not a real contract, it just an example implementation, I confirmed this with the sponsors during the contest. Pls @the-eridanus sir, confirm that here too. (I asked that in the private thread, I can post the screenshots If you want, Judge Sahab)

Even if this issue is considered as In-scope issue, I believe it is informational or a low issue at best, as the sponsor explained:

the issue would be an integration (e.g. interface or wallet) + aggregator error

Correct, THORChain_Aggregator is just a hypothetical example and not in scope. This issue is not valid because as stated this would be an integration error, not something that needs to be changed at the protocol side.