Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L185-L207 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L209-L238 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L261-L293

Vulnerability details

Description

In the THORChain_Router.sol contract, various functions utilize events to display transaction details to users.

Impact

There is a potential issue where the displayed information might be incorrect. Specifically, in the transferOut() , _transferOutV5() , and transferOutAndCall() functions, the contract includes logic to return funds to the sender if the initial transfer to the receiver fails.

If the call to transfer funds to the receiver fails due to any reason, the receiver does not receive the intended amount of ETH.
The funds are then transferred back to the sender's account.
As a result, the actual transfer of funds to the receiver does not occur, and the funds remain in the sender's account. However, the event emitted by the contract incorrectly indicates that the complete amount has been transferred to the receiver.
POC

https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L206 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L231-L237 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L284-L292

Test Case

Here is commands to run the POC:

forge test --mt test_EmitsWrongEvents -vvvv

Here is the proof of code:

//SPDX-License-Identifier: MIT
pragma solidity 0.8.22;

import {THORChain_Router} from "../contracts/THORChain_Router.sol";
import {MockERC20} from "forge-std/mocks/MockERC20.sol";
import {ERC20Token} from "../contracts/Token.sol";
import {Test, console} from "forge-std/Test.sol";
contract BugTest is Test {
    THORChain_Router public router;
    MockERC20 public token;
    address public Alice = makeAddr("Alice");
    address public Vault = makeAddr("Vault");

    function setUp() public {
        token = new MockERC20();
        token.initialize("Test", "TEST", 18);
        router = new THORChain_Router();
    }
    function test_EmitsWrongEvents() public {
            ReceiverContract receiver = new ReceiverContract();
            vm.deal(Alice, 10 ether);
            vm.startPrank(Alice);
            router.transferOut{value: 10 ether}(payable(address(receiver)), address(0), 0, "swap");
            vm.stopPrank();
            // As the receiver contract does not receive the ether, but the 
           function emits wrong events
           assertEq(address(receiver).balance, 0 ether);
            assertEq(address(Alice).balance, 10 ether);
        }  
  }
contract ReceiverContract {
    receive() external payable {
        revert("I'm a revert");
    }
}

Result

 │   ├─ [144] ReceiverContract::receive{value: 10000000000000000000}()
    │   │   └─ ← [Revert] revert: I'm a revert
    │   ├─ [0] 0x0000000000000000000000000000000000000000::fallback{value: 10000000000000000000}()
    │   │   └─ ← [Stop] 
    │   ├─ emit TransferOut(vault: 0x0000000000000000000000000000000000000000, to: ReceiverContract: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], asset: 0x0000000000000000000000000000000000000000, amount: 10000000000000000000 [1e19], memo: "swap")

Tools Used

Foundry .

Recommended Mitigation Steps

Here is the recommended mitigation:

 function transferOut(
    address payable to,
    address asset,
    uint amount,
    string memory memo
  ) public payable nonReentrant {
    uint safeAmount;
    if (asset == address(0)) {
      safeAmount = msg.value;
      bool success = to.send(safeAmount); // Send ETH.
      if (!success) {
     + safeAmount = 0; 
     +  payable(address(msg.sender)).transfer(msg.value); // For failure, bounce back to vault & continue.
    }
    } else {
      _vaultAllowance[msg.sender][asset] -= amount; // Reduce allowance
      (bool success, bytes memory data) = asset.call(
        abi.encodeWithSignature("transfer(address,uint256)", to, amount)
      );
      require(success && (data.length == 0 || abi.decode(data, (bool))));
      safeAmount = amount;
    }
    emit TransferOut(msg.sender, to, asset, safeAmount, memo);
  }

Assessed type

Other

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

Incorrect Information Displayed by Events in THORChain_Router.sol #69