From a8680370797134b6b81756025552407fd8a71864 Mon Sep 17 00:00:00 2001 From: John Saigle Date: Tue, 4 Mar 2025 14:56:28 -0500 Subject: [PATCH] Address code review comments - Avoid modifying the message or changing error handling in the watcher's message publication flow when transfer verifier is disabled - Remove continue statements when verifyAndPublish returns an error in the reobservation flow - Modify code comments around VerificationState - Use UintSlice when parsing CLI flags - Change new SDK constants to be strings instead of common.Address to be consistent with other SDK values --- node/cmd/guardiand/node.go | 34 +++++++++---------- node/pkg/common/chainlock.go | 49 ++++++++++++++------------- node/pkg/watchers/evm/reobserve.go | 3 -- node/pkg/watchers/evm/watcher.go | 36 +++++++++++--------- node/pkg/watchers/evm/watcher_test.go | 14 ++++---- sdk/devnet_consts.go | 5 ++- sdk/mainnet_consts.go | 5 ++- sdk/testnet_consts.go | 7 ++-- 8 files changed, 75 insertions(+), 78 deletions(-) diff --git a/node/cmd/guardiand/node.go b/node/cmd/guardiand/node.go index 932f7852d9..d97a0c6741 100644 --- a/node/cmd/guardiand/node.go +++ b/node/cmd/guardiand/node.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "math" "net" _ "net/http/pprof" // #nosec G108 we are using a custom router (`router := mux.NewRouter()`) and thus not automatically expose pprof. "os" @@ -11,7 +12,6 @@ import ( "path" "runtime" "slices" - "strconv" "strings" "syscall" "time" @@ -282,7 +282,7 @@ var ( subscribeToVAAs *bool // A list of chain IDs that should enable the Transfer Verifier. If empty, Transfer Verifier will not be enabled. - transferVerifierEnabledChains *string + transferVerifierEnabledChains *[]uint // Global variable used to store enabled Chain IDs for Transfer Verification. Contents are parsed from // transferVerifierEnabledChains. txVerifierChains []vaa.ChainID @@ -511,7 +511,7 @@ func init() { subscribeToVAAs = NodeCmd.Flags().Bool("subscribeToVAAs", false, "Guardiand should subscribe to incoming signed VAAs, set to true if running a public RPC node") - transferVerifierEnabledChains = NodeCmd.Flags().String("transferVerifierEnabledChains", "", "Transfer Verifier will be enabled for these chain IDs (comma-separated)") + transferVerifierEnabledChains = NodeCmd.Flags().UintSlice("transferVerifierEnabledChains", make([]uint, 2), "Transfer Verifier will be enabled for these chain IDs (comma-separated)") } var ( @@ -930,13 +930,15 @@ func runNode(cmd *cobra.Command, args []string) { logger.Fatal("If coinGeckoApiKey is set, then chainGovernorEnabled must be set") } + // NOTE: If this flag isn't set, or the list is empty, Transfer Verifier should not be enabled. if cmd.Flags().Changed("transferVerifierEnabledChains") { var parseErr error // NOTE: avoid shadowing txVerifierChains here. It should refer to the global variable. - txVerifierChains, parseErr = parseTxVerifierChains(*transferVerifierEnabledChains) - logger.Debug("parsed txVerifierChains", zap.Any("chains", txVerifierChains)) + txVerifierChains, parseErr = validateTxVerifierChains(*transferVerifierEnabledChains) + + logger.Debug("validated txVerifierChains", zap.Any("chains", txVerifierChains)) if parseErr != nil { - logger.Fatal("Could not parse transferVerifierEnabledChains", zap.Error(parseErr)) + logger.Fatal("transferVerifierEnabledChains input is invalid", zap.Error(parseErr)) } } @@ -1910,28 +1912,24 @@ func argsConsistent(args []string) bool { return true } -// parseTxVerifierChains parses a list of chainIds from a comma separated string and validate that they have a Transfer Verifier implementation. -// If the input is empty, Transfer Verifier will not be enabled. -func parseTxVerifierChains( +// validateTxVerifierChains validates that a slice of uints correspond to chain IDs with a Transfer Verifier implementation. +func validateTxVerifierChains( // A comma-separated list of Wormhole ChainIDs. - input string, + input []uint, ) ([]vaa.ChainID, error) { if len(input) == 0 { - return nil, errors.New("could not parse input to parseTxVerifierChains: input empty") + return nil, errors.New("no chain IDs provided for transfer verification") } - parsed := strings.Split(input, ",") knownChains := vaa.GetAllNetworkIDs() supportedChains := txverifier.SupportedChains() // NOTE: Using a known capacity and counter here avoids unnecessary reallocations compared to using `append()`. - enabled := make([]vaa.ChainID, len(parsed)) + enabled := make([]vaa.ChainID, len(input)) i := uint8(0) - for _, chainStr := range parsed { - chain, parseErr := strconv.Atoi(chainStr) - if parseErr != nil { - return nil, fmt.Errorf("could not parse chainId from string %s: %w", chainStr, parseErr) + for _, chain := range input { + if chain > uint(math.MaxUint16) { + return nil, fmt.Errorf("invalid chain ID %d: exceeds MaxUint16", chain) } - chainId := vaa.ChainID(chain) if !slices.Contains(knownChains, chainId) { diff --git a/node/pkg/common/chainlock.go b/node/pkg/common/chainlock.go index 2cb33a24a1..4983736d26 100644 --- a/node/pkg/common/chainlock.go +++ b/node/pkg/common/chainlock.go @@ -18,25 +18,41 @@ import ( const HashLength = 32 const AddressLength = 32 -// This type represents whether a message has been verified for some definition of verification. This may mean different things -// on a per-application or per-chain basis. -// NOTE: This status is currently used only by the Transfer Verifier for supported chains. +// The `VerificationState` is the result of applying transfer verification to the transaction associated with the `MessagePublication`. +// While this could likely be extended to additional security controls in the future, it is only used for `txverifier` at present. +// Consequently, its status should be set to `NotVerified` or `NotApplicable` for all messages that aren't token transfers. type VerificationState uint8 const ( // The default state for a message. This can be used before verification occurs. If no verification is required, `NotApplicable` should be used instead. - // NOTE: this value is used as the default, zero-value for the type, so this field should not be re-ordered among other variants. NotVerified VerificationState = iota // Represents a "known bad" status where a Message has been validated and the result indicates an erroneous or invalid message. The message should be discarded. Rejected // Represents an unusual state after validation, neither confirmed to be good or bad. Anomalous - // Represents a "known good" status where a Message has been validated and the result is good. The message should be process normally. + // Represents a "known good" status where a Message has been validated and the result is good. The message should be processed normally. Valid // Indicates that no verification is necessary. NotApplicable ) +func (v VerificationState) String() string { + switch v { + case NotVerified: + return "NotVerified" + case Valid: + return "Valid" + case Anomalous: + return "Anomalous" + case Rejected: + return "Rejected" + case NotApplicable: + return "NotApplicable" + default: + return "" + } +} + type MessagePublication struct { TxID []byte Timestamp time.Time @@ -52,8 +68,10 @@ type MessagePublication struct { // Unreliable indicates if this message can be reobserved. If a message is considered unreliable it cannot be // reobserved. Unreliable bool - // This type represents whether a message has been verified for some definition of verification. This may mean different things - // on a per-application or per-chain basis. + + // The `VerificationState` is the result of applying transfer verification to the transaction associated with the `MessagePublication`. + // While this could likely be extended to additional security controls in the future, it is only used for `txverifier` at present. + // Consequently, its status should be set to `NotVerified` or `NotApplicable` for all messages that aren't token transfers. verificationState VerificationState } @@ -298,20 +316,3 @@ func (msg *MessagePublication) ZapFields(fields ...zap.Field) []zap.Field { zap.String("verificationState", msg.verificationState.String()), ) } - -func (v VerificationState) String() string { - switch v { - case NotVerified: - return "NotVerified" - case Valid: - return "Valid" - case Anomalous: - return "Anomalous" - case Rejected: - return "Rejected" - case NotApplicable: - return "NotApplicable" - default: - return "" - } -} diff --git a/node/pkg/watchers/evm/reobserve.go b/node/pkg/watchers/evm/reobserve.go index a6dc76f5ae..be29b235b5 100644 --- a/node/pkg/watchers/evm/reobserve.go +++ b/node/pkg/watchers/evm/reobserve.go @@ -52,7 +52,6 @@ func (w *Watcher) handleReobservationRequest(ctx context.Context, chainId vaa.Ch if pubErr != nil { w.logger.Error("Error when publishing message", zap.Error(err)) - continue } numObservations++ @@ -80,7 +79,6 @@ func (w *Watcher) handleReobservationRequest(ctx context.Context, chainId vaa.Ch if pubErr != nil { w.logger.Error("Error when publishing message", zap.Error(err)) - continue } numObservations++ @@ -125,7 +123,6 @@ func (w *Watcher) handleReobservationRequest(ctx context.Context, chainId vaa.Ch if pubErr != nil { w.logger.Error("Error when publishing message", zap.Error(err)) - continue } numObservations++ diff --git a/node/pkg/watchers/evm/watcher.go b/node/pkg/watchers/evm/watcher.go index c531d0c723..0962c0edde 100644 --- a/node/pkg/watchers/evm/watcher.go +++ b/node/pkg/watchers/evm/watcher.go @@ -273,24 +273,25 @@ func (w *Watcher) Run(parentCtx context.Context) error { return errors.New("watcher attempted to create Transfer Verifier but this chainId is not supported") } - var tbridge eth_common.Address - var weth eth_common.Address + // Fetch the constants for the Token Bridge and the WETH address from the SDK. + var tbridge []byte + var weth string switch w.env { case common.UnsafeDevNet: - tbridge = eth_common.BytesToAddress(sdk.KnownDevnetTokenbridgeEmitters[w.chainID]) + tbridge = sdk.KnownDevnetTokenbridgeEmitters[w.chainID] weth = sdk.KnownDevnetWrappedNativeAddresses[w.chainID] case common.TestNet: - tbridge = eth_common.BytesToAddress(sdk.KnownTestnetTokenbridgeEmitters[w.chainID]) + tbridge = sdk.KnownTestnetTokenbridgeEmitters[w.chainID] weth = sdk.KnownTestnetWrappedNativeAddresses[w.chainID] case common.MainNet: - tbridge = eth_common.Address(sdk.KnownTokenbridgeEmitters[w.chainID]) + tbridge = sdk.KnownTokenbridgeEmitters[w.chainID] // https://etherscan.io/token/0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2 weth = sdk.KnownWrappedNativeAddress[w.chainID] } addrs := txverifier.TVAddresses{ CoreBridgeAddr: w.contract, - TokenBridgeAddr: tbridge, - WrappedNativeAddr: weth, + TokenBridgeAddr: eth_common.BytesToAddress(tbridge[:]), + WrappedNativeAddr: eth_common.HexToAddress(weth), } // The block height difference between the latest block and the oldest block to keep in memory. @@ -861,15 +862,17 @@ func (w *Watcher) verifyAndPublish( return errors.New("verifyAndPublish: message publication already has a verification status") } - // Verification is only relevant when Transfer Verification is enabled. - verificationState := common.NotApplicable - if w.txVerifierEnabled { // This should have already been initialized by the constructor. if w.txVerifier == nil { return errors.New("verifyAndPublish: transfer verifier should be instantiated but is nil") } + // Setting a custom verification state is only required when + // Transfer Verification is enabled. If disabled, the message + // will be emitted with the default value `NotVerified`. + var verificationState common.VerificationState + // Verify the transfer by analyzing the transaction receipt. This is a defense-in-depth mechanism // to protect against fraudulent message emissions. valid := w.txVerifier.ProcessEvent(ctx, txHash, receipt) @@ -880,13 +883,14 @@ func (w *Watcher) verifyAndPublish( w.logger.Debug("verifyAndPublish: transfer verification successful", zap.String("txHash", txHash.String())) verificationState = common.Valid } - } - updateErr := msg.SetVerificationState(verificationState) - if updateErr != nil { - errMsg := fmt.Sprintf("verifyAndPublish: could not set verification state for message with txID %s", msg.TxIDString()) - w.logger.Error(errMsg, zap.Error(updateErr)) - return fmt.Errorf("%s %w", errMsg, updateErr) + // Update the state of the message depending on the results of transfer verification. + updateErr := msg.SetVerificationState(verificationState) + if updateErr != nil { + errMsg := fmt.Sprintf("verifyAndPublish: could not set verification state for message with txID %s", msg.TxIDString()) + w.logger.Error(errMsg, zap.Error(updateErr)) + return fmt.Errorf("%s %w", errMsg, updateErr) + } } w.msgC <- msg diff --git a/node/pkg/watchers/evm/watcher_test.go b/node/pkg/watchers/evm/watcher_test.go index a7eeced9fc..d1b0411c2e 100644 --- a/node/pkg/watchers/evm/watcher_test.go +++ b/node/pkg/watchers/evm/watcher_test.go @@ -69,12 +69,12 @@ func TestVerifyAndPublish(t *testing.T) { // Check preconditions require.Equal(t, 0, len(w.msgC)) - require.Equal(t, common.NotVerified, msg.VerificationState()) + require.Equal(t, common.NotVerified.String(), msg.VerificationState().String()) // Check nil message err := w.verifyAndPublish(nil, ctx, eth_common.Hash{}, &types.Receipt{}) require.ErrorContains(t, err, "message publication cannot be nil") - require.Equal(t, common.NotVerified, msg.VerificationState()) + require.Equal(t, common.NotVerified.String(), msg.VerificationState().String()) // Check transfer verifier not enabled case. The message should be published normally msg = common.MessagePublication{} @@ -84,7 +84,7 @@ func TestVerifyAndPublish(t *testing.T) { publishedMsg := <-msgC require.NotNil(t, publishedMsg) require.Equal(t, 0, len(msgC)) - require.Equal(t, common.NotApplicable, publishedMsg.VerificationState()) + require.Equal(t, common.NotVerified.String(), publishedMsg.VerificationState().String()) // Check scenario where transfer verifier is enabled but isn't initialized. msg = common.MessagePublication{} @@ -93,7 +93,7 @@ func TestVerifyAndPublish(t *testing.T) { err = w.verifyAndPublish(&msg, ctx, eth_common.Hash{}, &types.Receipt{}) require.ErrorContains(t, err, "transfer verifier should be instantiated but is nil") require.Equal(t, 0, len(msgC)) - require.Equal(t, common.NotVerified, publishedMsg.VerificationState()) + require.Equal(t, common.NotVerified.String(), publishedMsg.VerificationState().String()) // Check scenario where the message already has a verification status. msg = common.MessagePublication{} @@ -103,7 +103,7 @@ func TestVerifyAndPublish(t *testing.T) { err = w.verifyAndPublish(&msg, ctx, eth_common.Hash{}, &types.Receipt{}) require.ErrorContains(t, err, "message publication already has a verification status") require.Equal(t, 0, len(msgC)) - require.Equal(t, common.Anomalous, msg.VerificationState()) + require.Equal(t, common.Anomalous.String(), msg.VerificationState().String()) // Check case where Transfer Verifier finds a dangerous transaction. Note that this case does // not return an error, but the published message should be marked as Rejected. @@ -117,7 +117,7 @@ func TestVerifyAndPublish(t *testing.T) { publishedMsg = <-msgC require.NotNil(t, publishedMsg) require.Equal(t, 0, len(msgC)) - require.Equal(t, common.Rejected, publishedMsg.VerificationState()) + require.Equal(t, common.Rejected.String(), publishedMsg.VerificationState().String()) // Check happy path where txverifier is enabled and initialized msg = common.MessagePublication{} @@ -130,7 +130,7 @@ func TestVerifyAndPublish(t *testing.T) { publishedMsg = <-msgC require.NotNil(t, publishedMsg) require.Equal(t, 0, len(msgC)) - require.Equal(t, common.Valid, publishedMsg.VerificationState()) + require.Equal(t, common.Valid.String(), publishedMsg.VerificationState().String()) } // Helper function to set up a test Ethereum Watcher diff --git a/sdk/devnet_consts.go b/sdk/devnet_consts.go index 5cd6d984f1..7ff6b43b2f 100644 --- a/sdk/devnet_consts.go +++ b/sdk/devnet_consts.go @@ -1,7 +1,6 @@ package sdk import ( - "github.com/ethereum/go-ethereum/common" "github.com/wormhole-foundation/wormhole/sdk/vaa" ) @@ -44,7 +43,7 @@ var KnownDevnetAutomaticRelayerEmitters = []struct { } // KnownDevnetWrappedNativeAddress is a map of wrapped native addresses by chain ID, e.g. WETH for Ethereum -var KnownDevnetWrappedNativeAddresses = map[vaa.ChainID]common.Address{ +var KnownDevnetWrappedNativeAddresses = map[vaa.ChainID]string{ // WETH deployed by the Tilt devnet configuration. - vaa.ChainIDEthereum: common.HexToAddress("0xDDb64fE46a91D46ee29420539FC25FD07c5FEa3E"), + vaa.ChainIDEthereum: "0xDDb64fE46a91D46ee29420539FC25FD07c5FEa3E", } diff --git a/sdk/mainnet_consts.go b/sdk/mainnet_consts.go index bd8784885e..b70348f205 100644 --- a/sdk/mainnet_consts.go +++ b/sdk/mainnet_consts.go @@ -4,7 +4,6 @@ import ( "encoding/hex" "fmt" - "github.com/ethereum/go-ethereum/common" "github.com/wormhole-foundation/wormhole/sdk/vaa" ) @@ -197,7 +196,7 @@ var KnownAutomaticRelayerEmitters = []struct { } // KnownWrappedNativeAddress is a map of wrapped native addresses by chain ID, e.g. WETH for Ethereum -var KnownWrappedNativeAddress = map[vaa.ChainID]common.Address{ +var KnownWrappedNativeAddress = map[vaa.ChainID]string{ // WETH - vaa.ChainIDEthereum: common.HexToAddress("0xc8f93d9738e7Ad5f3aF8c548DB2f6B7F8082B5e8"), + vaa.ChainIDEthereum: "0xc8f93d9738e7Ad5f3aF8c548DB2f6B7F8082B5e8", } diff --git a/sdk/testnet_consts.go b/sdk/testnet_consts.go index b0089bb823..3a583c65ad 100644 --- a/sdk/testnet_consts.go +++ b/sdk/testnet_consts.go @@ -1,7 +1,6 @@ package sdk import ( - "github.com/ethereum/go-ethereum/common" "github.com/wormhole-foundation/wormhole/sdk/vaa" ) @@ -107,9 +106,9 @@ var KnownTestnetAutomaticRelayerEmitters = []struct { } // KnownTestnetWrappedNativeAddresses is a list of addresses for deployments of wrapped native asssets (e.g. WETH) on various testnets. -var KnownTestnetWrappedNativeAddresses = map[vaa.ChainID]common.Address{ +var KnownTestnetWrappedNativeAddresses = map[vaa.ChainID]string{ // WETH - vaa.ChainIDSepolia: common.HexToAddress("0x7b79995e5f793a07bc00c21412e50ecae098e7f9"), + vaa.ChainIDSepolia: "0x7b79995e5f793a07bc00c21412e50ecae098e7f9", // WETH - vaa.ChainIDHolesky: common.HexToAddress("0xc8f93d9738e7Ad5f3aF8c548DB2f6B7F8082B5e8"), + vaa.ChainIDHolesky: "0xc8f93d9738e7Ad5f3aF8c548DB2f6B7F8082B5e8", }