EVM RPC errors may lead to missed inbound transactions

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/node/zetaclient/evm_client.go#L846
https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/node/zetaclient/evm_client.go#L883

Vulnerability details

Impact

Inbound transactions may be missed and are not voted upon by observers, potentially leading to a loss of funds.

Proof of Concept

ZetaChain observers watch external EVM chains via the ExternalChainWatcher function that internally calls the observeInTX function on each ob.GetCoreParams().InTxTicker ticker.

The observeInTX function performs multiple tasks:

1. Query for zeta sent (ZetaSent) logs
2. Query for ERC-20 deposited logs
3. Query tx's that are sent to the TSS address

The queried blocks are bound by the range of startBlock and toBlock , which are set in lines 809-810. The startBlock is the previously processed toBlock (i.e., retrieved via ob.GetLastBlockHeightScanned()), incremented by 1.

At the end of the function, in line 988, the toBlock is set as the new lastBlockHeightScanned.

However, in lines 846 and 883, an early return statement would skip the current tasks and would still proceed to store the toBlock as the new lastBlockHeightScanned, even though the blocks (and their logs) have not been fully processed.

This can be the case if the used RPC has temporary issues and the FilterZetaSent or FilterDeposited functions return an error.

774: func (ob *EVMChainClient) observeInTX() error {
...   // [...]
808: 	lastBlock := ob.GetLastBlockHeightScanned()
809: 	startBlock := lastBlock + 1
810: 	toBlock := lastBlock + config.MaxBlocksPerPeriod // read at most 10 blocks in one go
...   // [...]
823: 	//task 1:  Query evm chain for zeta sent logs
824: 	func() {
...     // [...]
838: 		logs, err := connector.FilterZetaSent(&bind.FilterOpts{
839: 			// #nosec G701 always positive
840: 			Start:   uint64(startBlock),
841: 			End:     &tb,
842: 			Context: context.TODO(),
843: 		}, []ethcommon.Address{}, []*big.Int{})
844: 		if err != nil {
845: 			ob.logger.ChainLogger.Warn().Err(err).Msgf("observeInTx: FilterZetaSent error:")
846: ❌			return
847: 		}
...   // [...]
863: 	}()
864:
865: 	// task 2: Query evm chain for deposited logs
866: 	func() {
867: 		// #nosec G701 always positive
868: 		toB := uint64(toBlock)
869: 		erc20custody, err := ob.GetERC20CustodyContract()
870: 		if err != nil {
871: 			ob.logger.ExternalChainWatcher.Warn().Err(err).Msgf("observeInTx: GetERC20CustodyContract error:")
872: 			return
873: 		}
874: 		depositedLogs, err := erc20custody.FilterDeposited(&bind.FilterOpts{
875: 			// #nosec G701 always positive
876: 			Start:   uint64(startBlock),
877: 			End:     &toB,
878: 			Context: context.TODO(),
879: 		}, []ethcommon.Address{})
880:
881: 		if err != nil {
882: 			ob.logger.ExternalChainWatcher.Warn().Err(err).Msgf("observeInTx: FilterDeposited error:")
883: ❌			return
884: 		}
...   // [...]
905: 	}()
...   // [...]
987: 	// ============= end of query the incoming tx to TSS address ==============
988: 	ob.SetLastBlockHeightScanned(toBlock)
989: 	if err := ob.db.Save(clienttypes.ToLastBlockSQLType(ob.GetLastBlockHeightScanned())).Error; err != nil {
990: 		ob.logger.ExternalChainWatcher.Error().Err(err).Msg("error writing toBlock to db")
991: 	}
992: 	return nil
993: }

Tools Used

Manual review

Recommended mitigation steps

Consider only updating the lastBlockHeightScanned if all tasks have been completed successfully to ensure all logs have been processed successfully and observers vote on all inbound transactions.

Assessed type

Error

[c4-pre-sort]

DadeKuma marked the issue as duplicate of #485

[c4-pre-sort]

DadeKuma marked the issue as sufficient quality report

[c4-pre-sort]

DadeKuma marked the issue as duplicate of #416

[c4-judge]

0xean changed the severity to 3 (High Risk)

[c4-judge]

0xean marked the issue as satisfactory

[berndartmueller]

While it seems that this submission is a dupe of #416, it is actually different in the sense that #416 is about an error (maliciously/purposefully) caused by PostSend (in lines 859 and 901) and subsequently early returning via return. As a result, other legitimate transactions are not processed.

However, here in this submission, the issue is rather that the underlying EVM RPC calls (in FilterZetaSent and FilterDeposited) potentially errors (e.g., timeouts, RPC provider issues,..). Consequently, the early return statements would lead to the toBlock being stored as the new lastBlockHeightScanned, even though the blocks (and their logs) have not been fully processed, resulting in missed transactions. Due to this issue relying on the underlying RPC to fail, the initial severity was chosen as Medium.

According to Verdict: Similar exploits under a single issue from the C4 Supreme Court Session, fixing the outlined root causes in #416 and its dupe #485 (replacing return with continue in line 859 and 901) would not mitigate the issue from this submission and thus make this submission eligible for being a separate issue.

For the above-mentioned reasons, I would like to request a second look at this submission. Thanks a lot!

[0xean]

@berndartmueller - thanks for the comment and referencing the C4 docs. While I understand your argument, I disagree. The issue isn't a single line of code, in a broader context the issue is the function isn't handling errors correctly and the early return needs to be corrected to handle all possible states.

The findings are duplicates if they share the same root cause. More specifically, if fixing the Root Cause (in a reasonable manner) would cause the finding to no longer be exploitable, then the findings are duplicates.

I would argue that a "reasonable" fix to this function would resolve both of these issues.