Bifrost risk of DoS due to the increase in transactions and events to process

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/main/bifrost/pkg/chainclients/shared/evm/smartcontract_log_parser.go#L166
https://github.com/code-423n4/2024-06-thorchain/blob/main/bifrost/pkg/chainclients/shared/evm/utils.go#L12-L21

Vulnerability details

Description

By removing the whitelisting, Bifrost will be exposed to a dramatic increase in traffic, which translate in much more CPU / RAM and might be hard to keep up the load, which could translate in transactions delay/failures and crash from the Bifrost daemon ifself, so a risk of DOSing the chain, as without Bifrost working properly, THORChain doesn't work, which seems to warrant High severity.

I'm gonna do the case for the ETH mainnet only, but then we must also consider the other blockchain being supported so BSC and AVAX at least. Bifrost doesn't seem currently arquitected to run on multiple machines for a single operator, and even in K8s Bifrost will always have a single pod to represent a node operator as far as I understand.

Daily ETH transaction is sitting at around 1.2M. If we account that about 1M transactions failure occur per month, that means on a daily basis about 33k transaction fails, so about 3% are failures, while 97% are success.

THORChain Router on ETH is receiving about 2000 tx per day which generates one event per transaction usually. This means the Bifrost will be calling ETHScanner::getTxInFromSmartContract that amount of time in the moment (and processing usually only a single event in GetTxInItem). Here is the issue. Once the whitelisting is removed, ALL the successful smart contract call on ETH that have emitted at least one event will now be calling this function.

func IsSmartContractCall(tx *etypes.Transaction, receipt *etypes.Receipt) bool {
	data := tx.Data()
	if len(data) < 4 {
		return false
	}
	if len(receipt.Logs) == 0 {
		return false
	}
	return true
}

This translate in the function ETHScanner::getTxInFromSmartContract (which include SmartContractLogParser::GetTxInItem) being called much more often, we are talking here about a 581x increase in terms of load. That is for transaction only, then many will have multiple events (but those will be cut early in the loop). This is the worst case scenario as considering all the successfull transaction to have emitted at least one event, which might not be always the case.
((1.2M * 0.97) / 2000) - 1 = 581x

Since the whole goal is to observe only the event emitted by the router, most of the processing here will be simply wasted, as of the 1.2M calls, only 2k would have such events (so 0.15% only).

func (scp *SmartContractLogParser) GetTxInItem(logs []*etypes.Log, txInItem *types.TxInItem) (bool, error) {
	if len(logs) == 0 {
		scp.logger.Info().Msg("tx logs are empty return nil")
		return false, nil
	} else if int(scp.maxLogs) > 0 && len(logs) > int(scp.maxLogs) {
		scp.logger.Info().Msgf("tx logs are too many, ignore")
		return false, nil
	}
	isVaultTransfer := false
	for _, item := range logs {
		// only events produced by THORChain router is processed
		if !scp.addressValidator(&item.Address, false) {    <-------------------------------------- ONLY interested in Router generated event
			continue
		}
		earlyExit := false
		switch item.Topics[0].String() {
		case depositEvent:
		
		...

Impact

Risk of Bifrost being DoS depending how much transactions supported EVM are generating. For example, someone that would like to DoS Bifrost could go on the cheapest supported chain, create a smart contract that do simply post dummy events to just hit the limit of event supported (which is configured to be 50), and do this as much as possible. Or a very popular dApp on the supported chain could inadvertently DoS Bifrost if their smart contract become very popular and emit a lot of events but not beyong the threshold.

Recommended Mitigation Steps

While the current Bifrost implementation is working with fine whitelisting, there is a realistic chance that it would not be working as smooth without whitelisting for the amount of load increase explained in this report.
There is a much more efficient way to retreive logs from a specific contract and this is using FilterQuery.

While my report is not prooving for sure that Bifrost would be DoS due to load increase (which means also THORChain itself, as without Bifrost, THORChain will not function properly), but we can understand that there is a real concern about it, it is not future proof and the current way of observing transaction on connected blockchain is inefficient, and I would highly recommend todo benchmark before going live with the current implementation, and probably implement a better observing mechanism as proposed here.

type FilterQuery struct {
	BlockHash *common.Hash     // used by eth_getLogs, return logs only from block with this hash
	FromBlock *big.Int         // beginning of the queried range, nil means genesis block
	ToBlock   *big.Int         // end of the range, nil means latest block
	Addresses []common.Address // restricts matches to events created by specific contracts

	// The Topic list restricts matches to particular event topics. Each event has a list
	// of topics. Topics matches a prefix of that list. An empty element slice matches any
	// topic. Non-empty elements represent an alternative that matches any of the
	// contained topics.
	//
	// Examples:
	// {} or nil          matches any topic list
	// {{A}}              matches topic A in first position
	// {{}, {B}}          matches any topic in first position AND B in second position
	// {{A}, {B}}         matches topic A in first position AND B in second position
	// {{A, B}, {C, D}}   matches topic (A OR B) in first position AND (C OR D) in second position
	Topics [][]common.Hash
}

Assessed type

DoS