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

[howlbot-integration[bot]]

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

[the-eridanus]

The new whitelist code is live on Stagenet (our testing network that uses external mainnet blockchains to test system load) - and we have not seen increased daemon load. Even if there was increased daemon load, I don't think this would be a security issue, hence adding "sponsor disputed"

[trust1995]

This submission does not fall under the acceptable security issues types that are accepted within the validity rules, specifically a flaw in the codebase needs to be demonstrated.

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid

[dontonka]

The new whitelist code is live on Stagenet (our testing network that uses external mainnet blockchains to test system load) - and we have not seen increased daemon load.

That's good sign. Do you have a metric indicating how much time GetTxInItem is being called when whitelist VS non-whitelist for example only for ETH? I would be curious to understand what is the real magnitud of increase.

Even if there was increased daemon load, I don't think this would be a security issue, hence adding "sponsor disputed"

The concern raised here is the DoS aspect of Bifrost which will be in a vulnerable position and will be wasting a lot of CPU/RAM processing transaction/events he doesn't care about. That might not be a problem today, but if you support another blockchain that generate much more transactions/logs, that might become an issue, which is the point of my report, to open your eyes on the current inefficiency aspect of your current router's event observation mechanism, which is pooling, while it should be listening ideally.

specifically a flaw in the codebase needs to be demonstrated.

The flaw is not being demostrated with a coded PoC indeed, but we can all understand how inefficient is the current implementation and how it places Bifrost in a bad position.

Overall I think my report is bringing some values and showcase how DoS impact could be a problem, how to mitigate it in an efficient way, but indeed without prooving the DoS impact today might not qualify as a security issue, so as such I think it could make more sense to be downgraded to QA, but if the judge feels it's not even the case, I'll ultimately respect his decision.

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)

[c4-judge]

trust1995 marked the issue as grade-c

[c4-judge]

trust1995 marked the issue as grade-b