Fund stuck forever in vault in case of multiple deposits to different vaults

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/main/bifrost/pkg/chainclients/shared/evm/smartcontract_log_parser.go#L182-L212

Vulnerability details

Description

While removing whitelisting will open the door for more composability with THORChain, the current SmartContractLogParser::GetTxInItem implementation seems too rigid to leverage such composability, at least in regard to the Deposit event, which seems to warrant High severity as user funds loss could occurs.

Imagine a dApp that integrates with THORChainRouter (as possible in the future with no whitelisting) decides to make multiple deposits in a single transaction which would be as follow:

EOA --> DepositAggregatorDapp::depositInBatch --> THORChainRouter::depositWithExpiry(Vault1, Asset1, ...)
                                              \-> THORChainRouter::depositWithExpiry(Vault1, Asset2, ...)
                                              \-> THORChainRouter::depositWithExpiry(Vault2, Asset2, ...)
                                              \-> THORChainRouter::depositWithExpiry(Vault3, Asset3, ...)

1. This would fall into the new path, as DepositAggregatorDapp is a smart contract call, but not directly the router. And since those events are generated by the router, it would pass the main gate.

		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))

2. Inside GetTxInItem, the first deposit event would be processed properly (depositWithExpiry(Vault1, Asset1, ...), but the 2nd event processed would fail the following condition, which would cause the function to abort and return an error. This is due to the fact that in a same transaction, there would be multiple deposit event that goes to different vaults, which granted doesn't make sense in a whitelisted world, but seems to make sense in a non-whitelisted world. This would means that those deposits event would not be accounted for, thus not sent to THORChain, which is effectively is a user funds loss for the user as those funds would be stuck there forever, like a donation.

			if len(txInItem.To) > 0 && !strings.EqualFold(txInItem.To, depositEvt.To.String()) {
				return false, fmt.Errorf("multiple events in the same transaction, have different to addresses , ignore")
			}

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) { //@audit: does an attacker SM delegating to router would generate event from Router or smart contract attack?
			continue
		}
		earlyExit := false
		switch item.Topics[0].String() {
		case depositEvent:
			// router contract , deposit function has re-entrance protection
			depositEvt, err := scp.parseDeposit(*item)
			if err != nil {
				scp.logger.Err(err).Msg("fail to parse deposit event")
				continue
			}
			if len(depositEvt.Amount.Bits()) == 0 {
				scp.logger.Info().Msg("deposit amount is 0, ignore")
				continue
			}
			if len(txInItem.To) > 0 && !strings.EqualFold(txInItem.To, depositEvt.To.String()) {
				return false, fmt.Errorf("multiple events in the same transaction, have different to addresses , ignore")
			}
			if len(txInItem.Memo) > 0 && !strings.EqualFold(txInItem.Memo, depositEvt.Memo) {
				return false, fmt.Errorf("multiple events in the same transaction , have different memo , ignore")
			}
			asset, err := scp.assetResolver(depositEvt.Asset.String())
			if err != nil {
				scp.logger.Err(err).Str("token address", depositEvt.Amount.String()).Msg("failed to get asset from token address")
				continue
			}
			if asset.IsEmpty() {
				continue
			}
			txInItem.To = depositEvt.To.String()
			txInItem.Memo = depositEvt.Memo
			decimals := scp.decimalResolver(depositEvt.Asset.String())
			txInItem.Coins = append(txInItem.Coins,
				common.NewCoin(asset, scp.amtConverter(depositEvt.Asset.String(), depositEvt.Amount)).WithDecimals(decimals))
			isVaultTransfer = false
			
			...

Impact

User deposits could be stuck forever into the THORChain vault(s) in case a non-whitelisted smart contract do multiple deposits to different vault in the same transaction.

Tools Used

Manual review

Recommended Mitigation Steps

The current event processing rules in GetTxInItem might need to evolve with the fact of removing the whitelisting. While in this report I showcased how an aggregator/integrator could break the Deposit event, other events would need also to be reviewed. Seems like a refactoring would be needed in GetTxInItem in order to support the use case described in my report, which seems realistic with increased composability.

Assessed type

Invalid Validation