howlbot-integration[bot] commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/bifrost/pkg/chainclients/shared/evm/smartcontract_log_parser.go#L66-L73

Vulnerability details

Proof of Concept

First note that this has been stated in the readMe: https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/README.md#L182-L188

### ERC20 token behaviors in scope

| Question                                                                                                                                                   | Answer |
| ---------------------------------------------------------------------------------------------------------------------------------------------------------- | ------ |
| [Balance changes outside of transfers](https://github.com/d-xo/weird-erc20?tab=readme-ov-file#balance-modifications-outside-of-transfers-rebasingairdrops) | Yes    |

This means that we should take into account rebasing tokens, now from the readMe we can see that the way Thorchain is supposed to work is that it watches the router and if there is any emitted verified log, to quote unqoute the readMe:

" The GetTxInItem is run for each obvserve transaction on EVM chains. The function iterates through each log emitted by the transaction, and determines if any valid logs were emitted by the THORChain router.

If it determines that a log was emitted by the THORChain router, it parses that log and forwards the appropriate details to the THORChain Layer 1 for processing. This is an incredibly sensitive and crucial piece of the codebase, as if it can be tricked, this means malicious contracts can trick Bifrost into informing THORChain of something that didn’t actually happen, leading to loss of funds. In fact, in July of 2021, THORChain’s old router + Bifrost were hacked in this exact way.

"

However, considering protocol integrates with tokens whose balances change outside transfers, this would mean that where as the GetTxInItem would assume a transfer that was made for any of these tokens and emmiited to be a valid transaction, there is a high chance that by the time the transaction is to be executed on the Layer 1 for processing, these balances would have changed.

This would then allow for an attacker to easily game the protocol in a way, for example the LIDO stETH token normally rebases around 12pm, so an attacker can always watch the mempool on the rebase from LIDO around the time and in the case where the rebase is going to skim off or heavily increase the balances they can position themselves to make the most out of the transaction by frontrunning the rebase update with a transfer in the THORChain router, and by the time this log is verified by GetTxInItem to be processed on the layer 1 chain the balance would have already changed for the asset, effectively tricking the system.

Impact

Tokens that have their balances changing outside withdrawals can be used to trick the protocol.

Note that this is very similar to the first Attack ideas hinted in the readMe, since in this case the attacker would be interacting with the Router and tricking the smartcontract_log_parser, and therefore the network, since the DepositEvent that's been emitted by the THORChain router would include non-accurate data in real time in regards to the real deposited amount.

Note that in our case here from the router, the snippets below essentially get called when the deposit is to be finalized https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/ethereum/contracts/THORChain_Router.sol#L142-L160

  // Deposit an asset with a memo. ETH is forwarded, ERC-20 stays in ROUTER
  function _deposit(
    address payable vault,
    address asset,
    uint amount,
    string memory memo
  ) private nonReentrant {
    uint safeAmount;
    if (asset == address(0)) {
      safeAmount = msg.value;
      bool success = vault.send(safeAmount);
      require(success);
    } else {
      require(msg.value == 0, "unexpected eth"); // protect user from accidentally locking up eth
      safeAmount = safeTransferFrom(asset, amount); // Transfer asset
      _vaultAllowance[vault][asset] += safeAmount; // Credit to chosen vault
    }
    emit Deposit(vault, asset, safeAmount, memo);//@audit
  }

Recommended Mitigation Steps

Do not support these tokens or integrate a functionality that ensures that the amount transferred has not been changed when it's been processed on the L1.