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#L324

Vulnerability details

The intention of the _transferOutAndCallV5 internal function at the error line was to transfer the ETH to the recipient if the swapping failed but wrong parameter was used as payable(aggregationPayload.target) instead of payable(aggregationPayload.recipient).

function _transferOutAndCallV5(TransferOutAndCallData calldata aggregationPayload) private {

  // ..

  if (!swapOutSuccess) {
    bool sendSuccess = payable(aggregationPayload.target).send(msg.value); // If can't swap, just send the recipient the gas asset //@audit-h target should be recipient
    if (!sendSuccess) {
      payable(address(msg.sender)).transfer(msg.value); // For failure, bounce back to vault & continue.
    }
  }

  // ..

}

Impact

Callers fund will always stay in the target(aggregator) contract when swapOutV5 failed

Tools Used

Manual review

Recommended Mitigation Steps

Add and remove the indicated lines below

function _transferOutAndCallV5(TransferOutAndCallData calldata aggregationPayload) private {

  // ..

  if (!swapOutSuccess) {
  -  bool sendSuccess = payable(aggregationPayload.target).send(msg.value);
  +  bool sendSuccess = payable(aggregationPayload.recipient).send(msg.value);
    if (!sendSuccess) {
      payable(address(msg.sender)).transfer(msg.value); // For failure, bounce back to vault & continue.
    }
  }

  // ..

}

Assessed type

Error

c4-judge commented 4 months ago

trust1995 marked the issue as unsatisfactory: Invalid