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#L323-L328

Vulnerability details

Impact

The eth after swap is supposed to send to the Recipient Address if the aggregator fails to receive.

Proof of Concept

It is clearly mentioned in the DOC that if the aggregator contract fails/revert when the eth is sent to it , it should repay the amount to the recipient address. But in the _transferOutAndCallV5 we are sending eth to aggregator contract itself even though it has already failed.

"Destination addresses should only be user-controlled addresses, not smart contract addresses.;"

function _transferOutAndCallV5(
    TransferOutAndCallData calldata aggregationPayload
  ) private {
    if (aggregationPayload.fromAsset == address(0)) {
      // call swapOutV5 with ether
      (bool swapOutSuccess, ) = aggregationPayload.target.call{
        value: msg.value
      }(
       ...
        )
      );
      if (!swapOutSuccess) {
        bool sendSuccess = payable(aggregationPayload.target).send(msg.value); // @audit should send to recipients address
        if (!sendSuccess) {
          payable(address(msg.sender)).transfer(msg.value); // For failure, bounce back to vault & continue.
        }
      }
    } 
      emit TransferOutAndCallV5(
       ...
      );
    }
}

Here instead the amount is passed to aggregationPayload.target instead of aggregationPayload.recipient.

Tools Used

Manual review

Recommended Mitigation Steps

Change aggregationPayload.target to aggregationPayload.recipient

Assessed type

ETH-Transfer

c4-judge commented 4 months ago

trust1995 marked the issue as unsatisfactory: Invalid