Lines of code

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

Vulnerability details

Impact

  function _transferOutAndCallV5(
    TransferOutAndCallData calldata aggregationPayload
  ) private {

...snip...

    } 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
        )
      );

...snip...

In the _transferOutAndCallV5 function, the _dexAggSuccess return value from the call to aggregator's swapOutV5 is not checked.

Note that the _vaultAllowance is reduced and the aggregationPayload.fromAsset is transferred to the aggregator before the swap call.

Thus, if the swap in aggregator fails (e.g. the amountOut is less than the specified amountOutMin), the funds are left in the aggregator, and the recipient receives nothing.

Moreover, in the above scenario, an attacker can directly use the aggregator's swapOutV5 function to move the left funds out, effectively stealing the funds meant for aggregationPayload.recipient.

Tools Used

Manual Review

Recommended Mitigation Steps

Check the _dexAggSuccess return value, and revert the transaction if _dexAggSuccess is false.

Assessed type

call/delegatecall

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