looping through the whiteList aggregator even though disableWhitelist=1

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/bifrost/pkg/chainclients/ethereum/ethereum_block_scanner.go#L902-L928 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/bifrost/pkg/chainclients/ethereum/ethereum_block_scanner.go#L671-L684

Vulnerability details

Impact

WhiteList contract are included even though we disable th only whiteList contract feature.

Proof of Concept

While checking to find whether the contract is valid ,we are passing includewhiteList parameter as true even though disableWhitelist is set to 1.

Lets see the case e.isToValidContractAddress(destination, true): her ein this snippet when we disabled the whitelist

https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/bifrost/pkg/chainclients/ethereum/ethereum_block_scanner.go#L902-L928

func (e *ETHScanner) fromTxToTxIn(tx *etypes.Transaction) (*stypes.TxInItem, error) {
    ....

    disableWhitelist, err := e.bridge.GetMimir(constants.EVMDisableContractWhitelist.String()) not initialised in constant
    if err != nil {
        e.logger.Err(err).Msgf("fail to get %s", constants.EVMDisableContractWhitelist.String())
        disableWhitelist = 0
    }

    if disableWhitelist == 1 {
        // parse tx without whitelist
        destination := tx.To()
        isToVault, _ := e.pubkeyMgr.IsValidPoolAddress(destination.String(), e.cfg.ChainID)

        switch {
        case isToVault:
            // Tx to a vault
            return e.getTxInFromTransaction(tx, receipt)
        case e.isToValidContractAddress(destination, true): //@audit isToValidContractAddress(destination, false) since we are disabling the whitelist .
            // Deposit directly to router
            return e.getTxInFromSmartContract(tx, receipt, 0)
        case evm.IsSmartContractCall(tx, receipt):
            // Tx to a different contract, attempt to parse with max allowable logs
            return e.getTxInFromSmartContract(tx, receipt, int64(e.cfg.MaxContractTxLogs))
        default:
            // Tx to a non-contract or vault address
            return e.getTxInFromTransaction(tx, receipt)
        }
    } else {
        // parse tx with whitelist
        if e.isToValidContractAddress(tx.To(), true) {
            return e.getTxInFromSmartContract(tx, receipt, 0)
        }
        return e.getTxInFromTransaction(tx, receipt)
    }
}

https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/bifrost/pkg/chainclients/ethereum/ethereum_block_scanner.go#L671-L684

func (e *ETHScanner) isToValidContractAddress(addr *ecommon.Address, includeWhiteList bool){
        contractAddresses := e.pubkeyMgr.GetContracts(common.ETHChain)
    if includeWhiteList {
        contractAddresses = append(contractAddresses, whitelistSmartContractAddress...)
    }
    // combine the whitelist smart contract address
    for _, item := range contractAddresses {
        if strings.EqualFold(item.String(), addr.String()) {
            return true
        }
    }
    return false
}

includeWhiteList is set as true while calling isToValidContractAddress that results in looping through the whitelisted contract.

Tools Used

Manual Review

Recommended Mitigation Steps

call isToValidContractAddress(destination, false) instead of passing true

Assessed type

Context

[the-eridanus]

Seems like a good point. We can probably remove that call all together

[trust1995]

@the-eridanus could you explain the worst case impact of the submission? To me it seems worst case scenario is inefficient code, which does not qualify for Med.

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)

[c4-judge]

trust1995 marked the issue as grade-c

[dontonka]

I would like to clarify some facts about this report since it was Sponsor Confirmed (cc: @the-eridanus) in case they plan to apply this in the future, but I totally agree with the judge to downgrade this to QA.

• The only benefit of removing this would be a minuscule saving in terms of CPU/RAM footprint.
• Plus, removing this will be a change in behavior in terms of logs supported by existing whitelisted contracts, as with the whitelist call the logs are unlimited, while otherwise it will be capped to the max log (planned to 50?), which has the potential to break existing aggregator that are working today with an higher amount of logs than the planned cap.

So overall there is some risk in applying this change, but if the team do their due diligences and feel confident with removing the call alltogether, I don't think this justify a Medium severity but rather Low severity, so I'm aligned with the judge here.

Nevertheless, if we compare this report to mine which has been disputed and set unsatisfactory, while showing a much more impactfull CPU/RAM impacts by the incoming changes, I'm having difficulties the reason about how the current issue was qualified as a QA while mine being deemed unsatisfactory, but I'm not the judge, and don't want to be XD, but I still wanted to express my disagreement in that regards.

[the-eridanus]

@the-eridanus could you explain the worst case impact of the submission? To me it seems worst case scenario is inefficient code, which does not qualify for Med.

agreed, the worst case is a very slight performance hit, but it's not really of concern.

[trust1995]

@dontonka I don't mind re-marking yours as QA, by the way a single issue QA report is usually not eligible for rewards, do keep that in mind. Note that you also submitted as High, so there are grounds to disqualify based on inflated severity, which I didn't.

@the-eridanus for the record, sponsor confirmed without "disagree-with-severity" telegraphs agreement with the severity level, which was wrong here.