code-423n4 / 2024-06-thorchain-findings

6 stars 3 forks source link

The function `THORChain_Router::_transferOutAndCallV5` is sending the ether to the `target` instead of the `recipient`. #66

Closed howlbot-integration[bot] closed 3 months ago

howlbot-integration[bot] commented 3 months ago

Lines of code

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

Vulnerability details

Description

In the function THORChain_Router::_transferOutAndCallV5, a call is performed on the target if fromAsset is address(0), here and here.

Based on the developers comments, the call should be performed on the recipient and not the target. // If can't swap, just send the recipient the gas asset

Impact

This would result, to send the funds to the wrong address.

Recommended Mitigation

[...]
-      (bool swapOutSuccess, ) = aggregationPayload.target.call{
+      (bool swapOutSuccess, ) = aggregationPayload.recipient.call{
        value: msg.value
      }(
[...]
      if (!swapOutSuccess) {
-        bool sendSuccess = payable(aggregationPayload.target).send(msg.value); // If can't swap, just send the recipient the gas asset
+        bool sendSuccess = payable(aggregationPayload.recipient).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.
        }
      }
[...]

Assessed type

ETH-Transfer

c4-judge commented 3 months ago

trust1995 marked the issue as unsatisfactory: Invalid

c4-judge commented 2 months ago

trust1995 marked the issue as unsatisfactory: Out of scope