howlbot-integration[bot] commented 4 months ago

Lines of code

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

Vulnerability details

Impact

The transferOutAndCall function, when aggregationPayload.fromAsset == address(0) emits a TransferOutAndCallV5 event regardless whether the swapOut was successfull or not.

This broadcasts false information on-chain and could have severe consequences since THORChain uses broadcasted events as input.

Proof of Concept

In the _transferOutAndCallV5 function, we can observe the following steps.

The function attempts to perform a swapOut.
If this fails, it attempts to send the ETH directly to the sender.
If this fails, it sends the ETH back to the vault.

    if (aggregationPayload.fromAsset == address(0)) {
      // call swapOutV5 with ether
      (bool swapOutSuccess, ) = aggregationPayload.target.call{
        value: msg.value
      }(
        abi.encodeWithSignature(
          "swapOutV5(address,uint256,address,address,uint256,bytes,string)",
          aggregationPayload.fromAsset,
          aggregationPayload.fromAmount,
          aggregationPayload.toAsset,
          aggregationPayload.recipient,
          aggregationPayload.amountOutMin,
          aggregationPayload.payload,
          aggregationPayload.originAddress
        )
      );
      if (!swapOutSuccess) {
        bool sendSuccess = payable(aggregationPayload.target).send(msg.value); // If can't swap, just send the recipient the gas asset
        if (!sendSuccess) {
          payable(address(msg.sender)).transfer(msg.value); // For failure, bounce back to vault & continue.
        }
      }

However, just below this we find:

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

The event emission is not conditional on the success of any of the above steps. If the swap fails and sending ETH to the target fails as well, the function will still emit an event as if a swap has happened.

Tools Used

Manual Review

Recommended Mitigation Steps

Include the event emission in the conditional for swap success. No success, no emission.

Assessed type

Other

c4-judge commented 4 months ago

trust1995 changed the severity to 3 (High Risk)