refactor: fix invalid zrc20withdraw (#1766)

changelog.md

@@ -17,7 +17,7 @@
 *[1728] (https://github.com/zeta-chain/node/pull/1728) - allow aborted transactions to be refunded by minting tokens to zEvm.
 
 ### Refactor
-
+* [1766](https://github.com/zeta-chain/node/pull/1766) - Refactors the `PostTxProcessing` EVM hook functionality to deal with invalid withdraw events
 * [1630](https://github.com/zeta-chain/node/pull/1630) added password prompts for hotkey and tss keyshare in zetaclient
   Starting zetaclient now requires two passwords to be input; one for the hotkey and another for the tss key-share.
 * [1760](https://github.com/zeta-chain/node/pull/1760) - Make staking keeper private in crosschain module
@@ -35,6 +35,7 @@
 * [1721](https://github.com/zeta-chain/node/issues/1721) - zetaclient should provide bitcoin_chain_id when querying TSS address
 * [1744](https://github.com/zeta-chain/node/pull/1744) - added cmd to encrypt tss keyshare file, allowing empty tss password for backward compatibility.
 
+
 ### Tests
 
 * [1584](https://github.com/zeta-chain/node/pull/1584) - allow to run E2E tests on any networks

common/address.go

@@ -72,6 +72,9 @@ func DecodeBtcAddress(inputAddress string, chainID int64) (address btcutil.Addre
 		return nil, fmt.Errorf("chain params not found")
 	}
 	address, err = btcutil.DecodeAddress(inputAddress, chainParams)
+	if err != nil {
+		return nil, fmt.Errorf("decode address failed: %s , for input address %s", err.Error(), inputAddress)
+	}
 	ok := address.IsForNet(chainParams)
 	if !ok {
 		return nil, fmt.Errorf("address is not for network %s", chainParams.Name)

common/address_test.go

@@ -23,20 +23,41 @@ func TestAddress(t *testing.T) {
 
 func TestDecodeBtcAddress(t *testing.T) {
 	t.Run("invalid string", func(t *testing.T) {
-		_, err := DecodeBtcAddress("�U�ڷ���i߭����꿚�l", 18332)
+		_, err := DecodeBtcAddress("�U�ڷ���i߭����꿚�l", BtcTestNetChain().ChainId)
 		require.ErrorContains(t, err, "runtime error: index out of range")
 	})
 	t.Run("invalid chain", func(t *testing.T) {
 		_, err := DecodeBtcAddress("14CEjTd5ci3228J45GdnGeUKLSSeCWUQxK", 0)
-		require.ErrorContains(t, err, "is not a Bitcoin chain")
+		require.ErrorContains(t, err, "is not a bitcoin chain")
 	})
-	t.Run("nil pointer dereference", func(t *testing.T) {
-		_, err := DecodeBtcAddress("tb1qy9pqmk2pd9sv63g27jt8r657wy0d9uee4x2dt2", 18332)
-		require.ErrorContains(t, err, "runtime error: invalid memory address or nil pointer dereference")
+	t.Run("invalid checksum", func(t *testing.T) {
+		_, err := DecodeBtcAddress("tb1qy9pqmk2pd9sv63g27jt8r657wy0d9uee4x2dt2", BtcTestNetChain().ChainId)
+		require.ErrorContains(t, err, "invalid checksum")
 	})
-	t.Run("valid address", func(t *testing.T) {
-		_, err := DecodeBtcAddress("bcrt1qy9pqmk2pd9sv63g27jt8r657wy0d9uee4x2dt2", 18444)
+	t.Run("valid legacy main-net address address incorrect params TestNet", func(t *testing.T) {
+		_, err := DecodeBtcAddress("1EYVvXLusCxtVuEwoYvWRyN5EZTXwPVvo3", BtcTestNetChain().ChainId)
+		require.ErrorContains(t, err, "decode address failed")
+	})
+	t.Run("valid legacy main-net address address incorrect params RegTestNet", func(t *testing.T) {
+		_, err := DecodeBtcAddress("1EYVvXLusCxtVuEwoYvWRyN5EZTXwPVvo3", BtcRegtestChain().ChainId)
+		require.ErrorContains(t, err, "decode address failed")
+	})
+
+	t.Run("valid legacy main-net address address correct params", func(t *testing.T) {
+		_, err := DecodeBtcAddress("1EYVvXLusCxtVuEwoYvWRyN5EZTXwPVvo3", BtcMainnetChain().ChainId)
+		require.NoError(t, err)
+	})
+	t.Run("valid legacy testnet address with correct params", func(t *testing.T) {
+		_, err := DecodeBtcAddress("n2TCLD16i8SNjwPCcgGBkTEeG6CQAcYTN1", BtcTestNetChain().ChainId)
 		require.NoError(t, err)
 	})
 
+	t.Run("non legacy valid address with incorrect params", func(t *testing.T) {
+		_, err := DecodeBtcAddress("bcrt1qy9pqmk2pd9sv63g27jt8r657wy0d9uee4x2dt2", BtcMainnetChain().ChainId)
+		require.ErrorContains(t, err, "address is not for network mainnet")
+	})
+	t.Run("non legacy valid address with correct params", func(t *testing.T) {
+		_, err := DecodeBtcAddress("bcrt1qy9pqmk2pd9sv63g27jt8r657wy0d9uee4x2dt2", BtcRegtestChain().ChainId)
+		require.NoError(t, err)
+	})
 }

common/chain.go

@@ -191,7 +191,7 @@ func GetBTCChainParams(chainID int64) (*chaincfg.Params, error) {
 	case 8332:
 		return &chaincfg.MainNetParams, nil
 	default:
-		return nil, fmt.Errorf("error chainID %d is not a Bitcoin chain", chainID)
+		return nil, fmt.Errorf("error chainID %d is not a bitcoin chain", chainID)
 	}
 }
 
@@ -204,7 +204,7 @@ func GetBTCChainIDFromChainParams(params *chaincfg.Params) (int64, error) {
 	case chaincfg.MainNetParams.Name:
 		return 8332, nil
 	default:
-		return 0, fmt.Errorf("error chain %s is not a Bitcoin chain", params.Name)
+		return 0, fmt.Errorf("error chain %s is not a bitcoin chain", params.Name)
 	}
 }
 

x/crosschain/keeper/evm_hooks.go

@@ -20,7 +20,7 @@ import (
 	"github.com/zeta-chain/zetacore/common"
 	"github.com/zeta-chain/zetacore/x/crosschain/types"
 	fungibletypes "github.com/zeta-chain/zetacore/x/fungible/types"
-	zetaObserverTypes "github.com/zeta-chain/zetacore/x/observer/types"
+	observertypes "github.com/zeta-chain/zetacore/x/observer/types"
 )
 
 var _ evmtypes.EvmHooks = Hooks{}
@@ -55,6 +55,11 @@ func (k Keeper) PostTxProcessing(
 // ProcessLogs post-processes logs emitted by a zEVM contract; if the log contains Withdrawal event
 // from registered ZRC20 contract, new CCTX will be created to trigger and track outbound
 // transaction.
+// Returning error from process logs does the following:
+// - revert the whole tx.
+// - clear the logs
+// TODO: implement unit tests
+// https://github.com/zeta-chain/node/issues/1759
 func (k Keeper) ProcessLogs(ctx sdk.Context, logs []*ethtypes.Log, emittingContract ethcommon.Address, txOrigin string) error {
 	system, found := k.fungibleKeeper.GetSystemContract(ctx)
 	if !found {
@@ -66,15 +71,52 @@ func (k Keeper) ProcessLogs(ctx sdk.Context, logs []*ethtypes.Log, emittingContr
 	}
 
 	for _, log := range logs {
-		eventWithdrawal, err := k.ParseZRC20WithdrawalEvent(ctx, *log)
-		if err == nil {
-			if err := k.ProcessZRC20WithdrawalEvent(ctx, eventWithdrawal, emittingContract, txOrigin); err != nil {
+		eventZrc20Withdrawal, errZrc20 := ParseZRC20WithdrawalEvent(*log)
+		eventZetaSent, errZetaSent := ParseZetaSentEvent(*log, connectorZEVMAddr)
+		if errZrc20 != nil && errZetaSent != nil {
+			// This log does not contain any of the two events
+			continue
+		}
+		if eventZrc20Withdrawal != nil && eventZetaSent != nil {
+			// This log contains both events, this is not possible
+			ctx.Logger().Error(fmt.Sprintf("ProcessLogs: log contains both ZRC20Withdrawal and ZetaSent events, %s , %s", log.Topics, log.Data))
+			continue
+		}
+
+		// We have found either eventZrc20Withdrawal or eventZetaSent
+		// These cannot be processed without TSS keys, return an error if TSS is not found
+		tss, found := k.zetaObserverKeeper.GetTSS(ctx)
+		if !found {
+			return errorsmod.Wrap(types.ErrCannotFindTSSKeys, "Cannot process logs without TSS keys")
+		}
+
+		// Do not process withdrawal events if inbound is disabled
+		if !k.zetaObserverKeeper.IsInboundEnabled(ctx) {
+			return observertypes.ErrInboundDisabled
+		}
+
+		// if eventZrc20Withdrawal is not nil we will try to validate it and see if it can be processed
+		if eventZrc20Withdrawal != nil {
+			// Check if the contract is a registered ZRC20 contract. If its not a registered ZRC20 contract, we can discard this event as it is not relevant
+			coin, foundCoin := k.fungibleKeeper.GetForeignCoins(ctx, eventZrc20Withdrawal.Raw.Address.Hex())
+			if !foundCoin {
+				ctx.Logger().Info(fmt.Sprintf("cannot find foreign coin with contract address %s", eventZrc20Withdrawal.Raw.Address.Hex()))
+				continue
+			}
+			// If Validation fails, we will not process the event and return and error. This condition means that the event was correct, and emitted from a registered ZRC20 contract
+			// But the information entered by the user is incorrect. In this case we can return an error and roll back the transaction
+			if err := ValidateZrc20WithdrawEvent(eventZrc20Withdrawal, coin.ForeignChainId); err != nil {
+				return err
+			}
+			// If the event is valid, we will process it and create a new CCTX
+			// If the process fails, we will return an error and roll back the transaction
+			if err := k.ProcessZRC20WithdrawalEvent(ctx, eventZrc20Withdrawal, emittingContract, txOrigin, tss); err != nil {
 				return err
 			}
 		}
-		eZeta, err := ParseZetaSentEvent(*log, connectorZEVMAddr)
-		if err == nil {
-			if err := k.ProcessZetaSentEvent(ctx, eZeta, emittingContract, txOrigin); err != nil {
+		// if eventZetaSent is not nil we will try to validate it and see if it can be processed
+		if eventZetaSent != nil {
+			if err := k.ProcessZetaSentEvent(ctx, eventZetaSent, emittingContract, txOrigin, tss); err != nil {
 				return err
 			}
 		}
@@ -84,15 +126,9 @@ func (k Keeper) ProcessLogs(ctx sdk.Context, logs []*ethtypes.Log, emittingContr
 
 // ProcessZRC20WithdrawalEvent creates a new CCTX to process the withdrawal event
 // error indicates system error and non-recoverable; should abort
-func (k Keeper) ProcessZRC20WithdrawalEvent(ctx sdk.Context, event *zrc20.ZRC20Withdrawal, emittingContract ethcommon.Address, txOrigin string) error {
-	if !k.zetaObserverKeeper.IsInboundEnabled(ctx) {
-		return types.ErrNotEnoughPermissions
-	}
-	ctx.Logger().Info("ZRC20 withdrawal to %s amount %d\n", hex.EncodeToString(event.To), event.Value)
-	tss, found := k.zetaObserverKeeper.GetTSS(ctx)
-	if !found {
-		return errorsmod.Wrap(types.ErrCannotFindTSSKeys, "ProcessZRC20WithdrawalEvent: cannot be processed without TSS keys")
-	}
+func (k Keeper) ProcessZRC20WithdrawalEvent(ctx sdk.Context, event *zrc20.ZRC20Withdrawal, emittingContract ethcommon.Address, txOrigin string, tss observertypes.TSS) error {
+
+	ctx.Logger().Info(fmt.Sprintf("ZRC20 withdrawal to %s amount %d", hex.EncodeToString(event.To), event.Value))
 	foreignCoin, found := k.fungibleKeeper.GetForeignCoins(ctx, event.Raw.Address.Hex())
 	if !found {
 		return fmt.Errorf("cannot find foreign coin with emittingContract address %s", event.Raw.Address.Hex())
@@ -137,7 +173,6 @@ func (k Keeper) ProcessZRC20WithdrawalEvent(ctx sdk.Context, event *zrc20.ZRC20W
 	// Get gas price and amount
 	gasprice, found := k.GetGasPrice(ctx, receiverChain.ChainId)
 	if !found {
-		fmt.Printf("gasprice not found for %s\n", receiverChain)
 		return fmt.Errorf("gasprice not found for %s", receiverChain)
 	}
 	cctx.GetCurrentOutTxParam().OutboundTxGasPrice = fmt.Sprintf("%d", gasprice.Prices[gasprice.MedianIndex])
@@ -147,21 +182,14 @@ func (k Keeper) ProcessZRC20WithdrawalEvent(ctx sdk.Context, event *zrc20.ZRC20W
 	return k.ProcessCCTX(ctx, cctx, receiverChain)
 }
 
-func (k Keeper) ProcessZetaSentEvent(ctx sdk.Context, event *connectorzevm.ZetaConnectorZEVMZetaSent, emittingContract ethcommon.Address, txOrigin string) error {
-	if !k.zetaObserverKeeper.IsInboundEnabled(ctx) {
-		return types.ErrNotEnoughPermissions
-	}
+func (k Keeper) ProcessZetaSentEvent(ctx sdk.Context, event *connectorzevm.ZetaConnectorZEVMZetaSent, emittingContract ethcommon.Address, txOrigin string, tss observertypes.TSS) error {
 	ctx.Logger().Info(fmt.Sprintf(
 		"Zeta withdrawal to %s amount %d to chain with chainId %d",
 		hex.EncodeToString(event.DestinationAddress),
 		event.ZetaValueAndGas,
 		event.DestinationChainId,
 	))
 
-	tss, found := k.zetaObserverKeeper.GetTSS(ctx)
-	if !found {
-		return errorsmod.Wrap(types.ErrCannotFindTSSKeys, "ProcessZetaSentEvent: cannot be processed without TSS keys")
-	}
 	if err := k.bankKeeper.BurnCoins(
 		ctx,
 		fungibletypes.ModuleName,
@@ -175,7 +203,7 @@ func (k Keeper) ProcessZetaSentEvent(ctx sdk.Context, event *connectorzevm.ZetaC
 
 	receiverChain := k.zetaObserverKeeper.GetSupportedChainFromChainID(ctx, receiverChainID.Int64())
 	if receiverChain == nil {
-		return zetaObserverTypes.ErrSupportedChains
+		return observertypes.ErrSupportedChains
 	}
 	// Validation if we want to send ZETA to an external chain, but there is no ZETA token.
 	chainParams, found := k.zetaObserverKeeper.GetChainParamsByChainID(ctx, receiverChain.ChainId)
@@ -242,10 +270,9 @@ func (k Keeper) ProcessCCTX(ctx sdk.Context, cctx types.CrossChainTx, receiverCh
 	return nil
 }
 
-// ParseZRC20WithdrawalEvent tries extracting Withdrawal event from registered ZRC20 contract;
-// returns error if the log entry is not a Withdrawal event, or is not emitted from a
-// registered ZRC20 contract
-func (k Keeper) ParseZRC20WithdrawalEvent(ctx sdk.Context, log ethtypes.Log) (*zrc20.ZRC20Withdrawal, error) {
+// ParseZRC20WithdrawalEvent tries extracting ZRC20Withdrawal event from the input logs using the zrc20 contract;
+// It only returns a not-nil event if the event has been correctly validated as a valid withdrawal event
+func ParseZRC20WithdrawalEvent(log ethtypes.Log) (*zrc20.ZRC20Withdrawal, error) {
 	zrc20ZEVM, err := zrc20.NewZRC20Filterer(log.Address, bind.ContractFilterer(nil))
 	if err != nil {
 		return nil, err
@@ -257,28 +284,33 @@ func (k Keeper) ParseZRC20WithdrawalEvent(ctx sdk.Context, log ethtypes.Log) (*z
 	if err != nil {
 		return nil, err
 	}
+	return event, nil
+}
+
+// ValidateZrc20WithdrawEvent checks if the ZRC20Withdrawal event is valid
+// It verifies event information for BTC chains and returns an error if the event is invalid
+func ValidateZrc20WithdrawEvent(event *zrc20.ZRC20Withdrawal, chainID int64) error {
+	// The event was parsed; that means the user has deposited tokens to the contract.
 
-	coin, found := k.fungibleKeeper.GetForeignCoins(ctx, event.Raw.Address.Hex())
-	if !found {
-		return nil, fmt.Errorf("ParseZRC20WithdrawalEvent: cannot find foreign coin with contract address %s", event.Raw.Address.Hex())
-	}
-	chainID := coin.ForeignChainId
 	if common.IsBitcoinChain(chainID) {
 		if event.Value.Cmp(big.NewInt(0)) <= 0 {
-			return nil, fmt.Errorf("ParseZRC20WithdrawalEvent: invalid amount %s", event.Value.String())
+			return fmt.Errorf("ParseZRC20WithdrawalEvent: invalid amount %s", event.Value.String())
 		}
 		addr, err := common.DecodeBtcAddress(string(event.To), chainID)
 		if err != nil {
-			return nil, fmt.Errorf("ParseZRC20WithdrawalEvent: invalid address %s: %s", event.To, err)
+			return fmt.Errorf("ParseZRC20WithdrawalEvent: invalid address %s: %s", event.To, err)
 		}
 		_, ok := addr.(*btcutil.AddressWitnessPubKeyHash)
 		if !ok {
-			return nil, fmt.Errorf("ParseZRC20WithdrawalEvent: invalid address %s (not P2WPKH address)", event.To)
+			return fmt.Errorf("ParseZRC20WithdrawalEvent: invalid address %s (not P2WPKH address)", event.To)
 		}
 	}
-	return event, nil
+	return nil
 }
 
+// ParseZetaSentEvent tries extracting ZetaSent event from connectorZEVM contract;
+// returns error if the log entry is not a ZetaSent event, or is not emitted from connectorZEVM
+// It only returns a not-nil event if all the error checks pass
 func ParseZetaSentEvent(log ethtypes.Log, connectorZEVM ethcommon.Address) (*connectorzevm.ZetaConnectorZEVMZetaSent, error) {
 	zetaConnectorZEVM, err := connectorzevm.NewZetaConnectorZEVMFilterer(log.Address, bind.ContractFilterer(nil))
 	if err != nil {