diff --git a/node/pkg/processor/cleanup.go b/node/pkg/processor/cleanup.go index 843ca0e61..40a4da681 100644 --- a/node/pkg/processor/cleanup.go +++ b/node/pkg/processor/cleanup.go @@ -107,7 +107,7 @@ func (p *Processor) handleCleanup(ctx context.Context) { } hasSigs := len(s.signatures) - wantSigs := CalculateQuorum(len(gs.Keys)) + wantSigs := vaa.CalculateQuorum(len(gs.Keys)) quorum := hasSigs >= wantSigs var chain vaa.ChainID @@ -211,7 +211,7 @@ func (p *Processor) handleCleanup(ctx context.Context) { // network reached consensus without us. We don't know the correct guardian // set, so we simply use the most recent one. hasSigs := len(s.signatures) - wantSigs := CalculateQuorum(len(p.gs.Keys)) + wantSigs := vaa.CalculateQuorum(len(p.gs.Keys)) p.logger.Info("expiring unsubmitted nil observation", zap.String("digest", hash), diff --git a/node/pkg/processor/observation.go b/node/pkg/processor/observation.go index f2b26b83e..fe0806c51 100644 --- a/node/pkg/processor/observation.go +++ b/node/pkg/processor/observation.go @@ -192,7 +192,7 @@ func (p *Processor) handleObservation(ctx context.Context, m *gossipv1.SignedObs // We have made this observation on chain! // 2/3+ majority required for VAA to be valid - wait until we have quorum to submit VAA. - quorum := CalculateQuorum(len(gs.Keys)) + quorum := vaa.CalculateQuorum(len(gs.Keys)) p.logger.Info("aggregation state for observation", zap.String("digest", hash), @@ -247,36 +247,8 @@ func (p *Processor) handleInboundSignedVAAWithQuorum(ctx context.Context, m *gos return } - // Check if VAA doesn't have any signatures - if len(v.Signatures) == 0 { - p.logger.Warn("received SignedVAAWithQuorum message with no VAA signatures", - zap.String("digest", hash), - zap.Any("message", m), - zap.Any("vaa", v), - ) - return - } - - // Verify VAA has enough signatures for quorum - quorum := CalculateQuorum(len(p.gs.Keys)) - if len(v.Signatures) < quorum { - p.logger.Warn("received SignedVAAWithQuorum message without quorum", - zap.String("digest", hash), - zap.Any("message", m), - zap.Any("vaa", v), - zap.Int("wanted_sigs", quorum), - zap.Int("got_sigs", len(v.Signatures)), - ) - return - } - - // Verify VAA signatures to prevent a DoS attack on our local store. - if !v.VerifySignatures(p.gs.Keys) { - p.logger.Warn("received SignedVAAWithQuorum message with invalid VAA signatures", - zap.String("digest", hash), - zap.Any("message", m), - zap.Any("vaa", v), - ) + if err := v.Verify(p.gs.Keys); err != nil { + p.logger.Warn("dropping SignedVAAWithQuorum message because it failed verification: " + err.Error()) return } diff --git a/node/pkg/processor/observation_test.go b/node/pkg/processor/observation_test.go index a9a6c6b98..c384ffc64 100644 --- a/node/pkg/processor/observation_test.go +++ b/node/pkg/processor/observation_test.go @@ -75,13 +75,13 @@ func TestHandleInboundSignedVAAWithQuorum(t *testing.T) { {label: "GuardianSetNoKeys", keyOrder: []*ecdsa.PrivateKey{}, indexOrder: []uint8{}, addrs: []ethcommon.Address{}, errString: "dropping SignedVAAWithQuorum message since we have a guardian set without keys"}, {label: "VAANoSignatures", keyOrder: []*ecdsa.PrivateKey{}, indexOrder: []uint8{0}, addrs: []ethcommon.Address{goodAddr1}, - errString: "received SignedVAAWithQuorum message with no VAA signatures"}, + errString: "dropping SignedVAAWithQuorum message because it failed verification: VAA was not signed"}, {label: "VAAInvalidSignatures", keyOrder: []*ecdsa.PrivateKey{badPrivateKey1}, indexOrder: []uint8{0}, addrs: []ethcommon.Address{goodAddr1}, - errString: "received SignedVAAWithQuorum message with invalid VAA signatures"}, + errString: "dropping SignedVAAWithQuorum message because it failed verification: VAA had bad signatures"}, {label: "DuplicateGoodSignaturesNonMonotonic", keyOrder: []*ecdsa.PrivateKey{goodPrivateKey1, goodPrivateKey1, goodPrivateKey1, goodPrivateKey1}, indexOrder: []uint8{0, 0, 0, 0}, addrs: []ethcommon.Address{goodAddr1}, - errString: "received SignedVAAWithQuorum message with invalid VAA signatures"}, + errString: "dropping SignedVAAWithQuorum message because it failed verification: VAA had bad signatures"}, {label: "DuplicateGoodSignaturesMonotonic", keyOrder: []*ecdsa.PrivateKey{goodPrivateKey1, goodPrivateKey1, goodPrivateKey1, goodPrivateKey1}, indexOrder: []uint8{0, 1, 2, 3}, addrs: []ethcommon.Address{goodAddr1}, - errString: "received SignedVAAWithQuorum message with invalid VAA signatures"}, + errString: "dropping SignedVAAWithQuorum message because it failed verification: VAA had bad signatures"}, } for _, tc := range tests { diff --git a/node/pkg/processor/quorum_test.go b/node/pkg/processor/quorum_test.go index 7c769112d..44b3e250c 100644 --- a/node/pkg/processor/quorum_test.go +++ b/node/pkg/processor/quorum_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/wormhole-foundation/wormhole/sdk/vaa" ) func TestCalculateQuorum(t *testing.T) { @@ -31,7 +32,7 @@ func TestCalculateQuorum(t *testing.T) { } for _, tc := range tests { t.Run(fmt.Sprint(tc.have), func(t *testing.T) { - assert.Equal(t, tc.want, CalculateQuorum(tc.have)) + assert.Equal(t, tc.want, vaa.CalculateQuorum(tc.have)) }) } } diff --git a/node/pkg/processor/quorum.go b/sdk/vaa/quorum.go similarity index 95% rename from node/pkg/processor/quorum.go rename to sdk/vaa/quorum.go index 45dd8510c..5bfbfc4f1 100644 --- a/node/pkg/processor/quorum.go +++ b/sdk/vaa/quorum.go @@ -1,4 +1,4 @@ -package processor +package vaa // CalculateQuorum returns the minimum number of guardians that need to sign a VAA for a given guardian set. // diff --git a/sdk/vaa/structs.go b/sdk/vaa/structs.go index be2957145..0239dc60c 100644 --- a/sdk/vaa/structs.go +++ b/sdk/vaa/structs.go @@ -5,6 +5,7 @@ import ( "crypto/ecdsa" "encoding/binary" "encoding/hex" + "errors" "fmt" "io" "math/big" @@ -444,6 +445,37 @@ func (v *VAA) VerifySignatures(addresses []common.Address) bool { return true } +// Verify is a function on the VAA that takes a complete set of guardian keys as input and attempts certain checks with respect to this guardian. +// Verify will return nil if the VAA passes checks. Otherwise, Verify will return an error containing the text of the first check to fail. +// NOTE: Verify will not work correctly if a subset of the guardian set keys is passed in. The complete guardian set must be passed in. +// Verify does the following checks: +// - If the guardian does not have or know its own guardian set keys, then the VAA cannot be verified. +// - Quorum is calculated on the guardian set passed in and checks if the VAA has enough signatures. +// - The signatures in the VAA is verified against the guardian set keys. +func (v *VAA) Verify(addresses []common.Address) error { + if addresses == nil { + return errors.New("No addresses were provided") + } + + // Check if VAA doesn't have any signatures + if len(v.Signatures) == 0 { + return errors.New("VAA was not signed") + } + + // Verify VAA has enough signatures for quorum + quorum := CalculateQuorum(len(addresses)) + if len(v.Signatures) < quorum { + return errors.New("VAA did not have a quorum") + } + + // Verify VAA signatures to prevent a DoS attack on our local store. + if !v.VerifySignatures(addresses) { + return errors.New("VAA had bad signatures") + } + + return nil +} + // Marshal returns the binary representation of the VAA func (v *VAA) Marshal() ([]byte, error) { buf := new(bytes.Buffer)