User can loss hsi funds in `_transferOutAndCallV5` function

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

The is _transferOutAndCallV5 functions is called by the by a THORChain vault when a user want to make a swap through the THORChainn.

See _transferOutAndCallV5 function:

function _transferOutAndCallV5(
    TransferOutAndCallData calldata aggregationPayload
  ) private {
   ...
    } 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
        ) <--------
      );

      emit TransferOutAndCallV5(
        msg.sender,
        aggregationPayload.target,
        aggregationPayload.fromAmount,
        aggregationPayload.toAsset,
        aggregationPayload.recipient,
        aggregationPayload.amountOutMin,
        aggregationPayload.memo,
        aggregationPayload.payload,
        aggregationPayload.originAddress
      );
    }
  }

[Link]

This function is sending the tokens first to the aggregationPayload.target see first arrow, after checking that the transfer was successful the function then perform the call to make the swap if the aggregator fails (see second arrow above), the transaction should not revert.

The problem is that if the aggregator fails during the swap the funds are never refunded to the user and the event is emitted as is everything was successfully.

Impact

If the swap in the aggregator contract fail the protocol is never refund the user.

Proof of Concept

As we can see:

function _transferOutAndCallV5(
    TransferOutAndCallData calldata aggregationPayload
  ) private {
   ...
    } 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
        ) <--------
      );

      emit TransferOutAndCallV5(
        msg.sender,
        aggregationPayload.target,
        aggregationPayload.fromAmount,
        aggregationPayload.toAsset,
        aggregationPayload.recipient,
        aggregationPayload.amountOutMin,
        aggregationPayload.memo,
        aggregationPayload.payload,
        aggregationPayload.originAddress
      );
    }
  }

[Link]

In the first arrow we see that the tokens are been sent to the aggregator, then in case that the swapOutV5 in the aggregator fail (see second arrow) the tokens are not been refunded to the user instead it stay in the aggregator and the the event is emitted as if the swap was successfully.

Tools Used

Manual

Recommended Mitigation Steps

One of the recommendation that i can think about is taking out the token from the aggregator contract with a safeTransferFrom function with previously given allowance from the aggregator to the router. Indeed this add some complex so it has to be reviewed carefully.

Assessed type

Other

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid

[c4-judge]

trust1995 marked the issue as unsatisfactory: Out of scope