howlbot-integration[bot] commented 4 months ago

Lines of code

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

Vulnerability details

Impact

Using msg.value in a loop allows the contract to be completely drained of Ethereum.

Proof of Concept

There is the private function _transferOutAndCallV5() that could be called either by transferOutAndCallV5() or batchTransferOutAndCallV5(). It takes the TransferOutAndCallData struct and uses it to create a swap from Ethereum to a token or from a token to another token. If, the fromAsset variable in the struct is address(0), then a swap from Ethereum to a token will occur. This is the if statement we would end up in that scenario (skipped the event emission):

if (aggregationPayload.fromAsset == address(0)) {
      (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 (!sendSuccess) {
          payable(address(msg.sender)).transfer(msg.value);
        }
      }
}

The issue here is that this is one of the ways this function gets called:

function batchTransferOutAndCallV5(
    TransferOutAndCallData[] calldata aggregationPayloads
  ) external payable nonReentrant {
    for (uint i = 0; i < aggregationPayloads.length; ++i) {
      _transferOutAndCallV5(aggregationPayloads[i]);
    }
  }

This uses msg.value in a loop essentially allowing for a user to send 1 Ether and if the loop is iterated 10 times, then that would count as 10 Ethereum as msg.value is the same for each iteration.

User can, for example, just specify himself as the target, send 1 Ethereum to the function, iterate it 10 times and steal 10 Ethereum of the contract.

While there is no documented way for Ethereum to be in the contract, it is still possible and I explained 4 ways for that to happen in my previous issue report regarding another way to completely drain the contract.

Also, if there is no ETH available in the contract, that is still unintended behavior that would lead to reverts in a lot of cases.

Tools Used

Manual Review

Recommended Mitigation Steps

Avoid using msg.value in a loop

Assessed type

Loop

c4-judge commented 4 months ago

trust1995 changed the severity to 2 (Med Risk)