Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/main/ethereum/contracts/THORChain_Router.sol#L330 https://github.com/code-423n4/2024-06-thorchain/blob/main/ethereum/contracts/THORChain_Router.sol#L330 https://github.com/code-423n4/2024-06-thorchain/blob/main/ethereum/contracts/THORChain_Router.sol#L231 https://github.com/code-423n4/2024-06-thorchain/blob/main/ethereum/contracts/THORChain_Router.sol#L206

Vulnerability details

Impact

The system must emit only correct events for the smartcontract_log_parser to process only valid actions. In the THORChain_Router::transferOut, THORChain_Router::_transferOutV5, THORChain_Router::transferOutAndCall and THORChain_Router::_transferOutAndCallV5 events are emitted even if the sending of the funds is not successful

smartcontract_log_parser will process incorrect events, leading to a protocol's loss of funds and this is very bad according to the documentation

Proof of Concept

Please add a new test file called for example 7_Incorrect_Event_Emission.js and paste the code from below. The code asserts that an event is emitted even if the send has failed and the funds are refunded to the msg.sender

const Router = artifacts.require('THORChain_Router');
const BigNumber = require('bignumber.js');
const { expect } = require('chai');
function BN2Str(BN) {
  return new BigNumber(BN).toFixed();
}

var ROUTER, ASGARD, USER1;

const ETH = '0x0000000000000000000000000000000000000000';
const _1 = '1000000000000000000';

describe('Incorrect emit of events', function () {
  let accounts;

  before(async function () {
    accounts = await web3.eth.getAccounts();
    ROUTER = await Router.new();
    USER1 = accounts[0];
    ASGARD = accounts[3];
  });

  it('Should emit emit on failed send', async function () {
    /* 
      Make a transfer call which will try to send the amount to the router,
      but will not be successful and return the ethers to the USER1 
    */
    const tx = await ROUTER.transferOut(ROUTER.address, ETH, _1, 'MEMO', {
      from: USER1,
      value: _1,
    });

    /* Get ending balances of */
    const endingBalanceOfUser1 = await web3.eth.getBalance(USER1);
    const endingBalanceOfRouter = await web3.eth.getBalance(ROUTER.address);

    /* Log ending balances */
    console.log('\nEnding Balance USER1:', BN2Str(endingBalanceOfUser1));
    console.log('Ending Balance ROUTER:', BN2Str(endingBalanceOfRouter));

    /* Assert that transfer out event was emitted even if the transfer failed */
    expect(tx.logs[0].event).to.equal('TransferOut');
    expect(tx.logs[0].args.to).to.equal(ROUTER.address);
    expect(tx.logs[0].args.asset).to.equal(ETH);
    expect(tx.logs[0].args.memo).to.equal('MEMO');
    expect(BN2Str(tx.logs[0].args.amount)).to.equal(_1);
    expect(BN2Str(endingBalanceOfRouter)).to.be.equal('0');
  });
});

Tools Used

Manual Review

Recommended Mitigation Steps

Emit events only if the actions are successful

Assessed type

Other

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

[H-03] Incorrect event emissions, tricking the `smartcontract_log_parser` #52