`aggregationPayload.fromAmount` of an ERC20 token can be lost when calling `_transferOutAndCallV5` function for such token if target aggregator's `swapOutV5` function call reverts

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/5b91b5c6683a222b0ce046533515e301c9699d74/chain/ethereum/contracts/THORChain_Router.sol#L304-L389 https://github.com/code-423n4/2024-06-thorchain/blob/0e02d3c221d7e546a73f25183e29500eaa63f4cf/chain/ethereum/contracts/THORChain_Aggregator.sol#L144-L216

Vulnerability details

Impact

Calling the following _transferOutAndCallV5 function for an ERC20 token executes aggregationPayload.fromAsset.call(abi.encodeWithSignature("transfer(address,uint256)", aggregationPayload.target, aggregationPayload.fromAmount)), which transfers aggregationPayload.fromAmount of such token to the target aggregator. The _transferOutAndCallV5 function then calls the target aggregator's swapOutV5 function below but such swapOutV5 function call can revert, such as when the swapRouter.swapExactTokensForETH function call cannot satisfy aggregationPayload.amountOutMin. In this situation, calling the _transferOutAndCallV5 function would not revert even though _dexAggSuccess would be false. Since aggregationPayload.fromAmount of such token has been transferred to the target aggregator, failing to use these transferred tokens in the target aggregator's swapOutV5 function call and not reverting the _transferOutAndCallV5 function call cause the vault to send these transferred tokens and the recipient to receive nothing in return. As a result, the recipient loses aggregationPayload.fromAmount of such token that she or he deserves.

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

  function _transferOutAndCallV5(
    TransferOutAndCallData calldata aggregationPayload
  ) private {
    if (aggregationPayload.fromAsset == address(0)) {
      ...
    } else {
      _vaultAllowance[msg.sender][
        aggregationPayload.fromAsset
      ] -= aggregationPayload.fromAmount; // Reduce allowance

      // send ERC20 to aggregator contract so it can do its thing
      (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"
      );

      // add test case if aggregator fails, it should not revert the whole transaction (transferOutAndCallV5 call succeeds)
      // call swapOutV5 with erc20. if the aggregator fails, the transaction should not revert
      (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
        )
      );
      ...
    }
  }

https://github.com/code-423n4/2024-06-thorchain/blob/0e02d3c221d7e546a73f25183e29500eaa63f4cf/chain/ethereum/contracts/THORChain_Aggregator.sol#L144-L216

  function swapOutV5(
    address fromAsset,
    uint256 fromAmount,
    address toAsset,
    address recipient,
    uint256 amountOutMin,
    bytes memory payload,
    string memory originAddress
  ) public payable nonReentrant {
    address[] memory path = new address[](2);
    ...
    if (fromAsset == address(0)) {
      ...
    } else {
      // received an ERC20, swap to ETH or do as you please
      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
          )
        );
        require(aggSuccess, "swapExactTokensForETH failed");
      } else {
        // do something with your payload like parse to an aggregator specific structure
        // UniV2 transfers from 'msg.sender', so we need a low-level `.call` to change the msg.sender from the target's perspective
        (bool success, ) = 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
          )
        );
        require(success, "swapExactTokensForETH failed");
      }
    }
  }

Proof of Concept

The following steps can occur for the described scenario.

1. A vault calls the _transferOutAndCallV5 function for an ERC20 token.
2. Since the swapRouter.swapExactTokensForETH function call does not satisfy aggregationPayload.amountOutMin, the target aggregator's swapOutV5 function call reverts.
3. Because calling the _transferOutAndCallV5 function sends aggregationPayload.fromAmount of such token to the target aggregator and does not revert even though the target aggregator's swapOutV5 function call reverts, the vault sends these transferred tokens but the recipient receives nothing in return.
4. The recipient loses aggregationPayload.fromAmount of such token that she or he deserves.

Tools Used

Manual Review

Recommended Mitigation Steps

The _transferOutAndCallV5 function can be updated to revert if _dexAggSuccess returned by the target aggregator's swapOutV5 function call is false.

Assessed type

Token-Transfer

[the-eridanus]

This is an intentional design decision. The aggregator should have a rescue function available in the case that the swapOut fails - this should not be the responsibility of the Router. Adding "sponsor disputed"

[trust1995]

Sponsor's comments seem appropriate, will invalidate.

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid

[0xGreed]

I believe the issue should be valid.

Even though there is a rescue function on the aggregator (e.g. rescueFunds()), not only the contract owner must be aware that there are some funds that need to be rescued (which might not be immediate), but an attacker still has the ability to frontrun the owner transaction to execute swapOutV5() and drain the tokens that have been transferred previously.

The swap on the aggregator works in a two-steps process (token.transfer() and aggregator.swapOutV5()) which must be done atomically in order to prevent the loss of funds. In case these steps are splitted, a malicious actor can insert the same swapOutV5() transaction in between but with a different recipient to steal funds.

[trust1995]

@the-eridanus can you offer comments on that?

[trust1995]

This is invalid similar to other issues which assume behavior of the OOS aggregator contract.