fix: classify invalid external blocks safely

Author: randygrok

block/internal/syncing/syncer.go

@@ -897,12 +897,28 @@ func (s *Syncer) ValidateBlock(_ context.Context, currState types.State, data *t
 		return fmt.Errorf("invalid header: %w", err)
 	}
 
+	if err := currState.AssertExpectedProposer(header); err != nil {
+		return errors.Join(errInvalidBlock, err)
+	}
+
 	if err := currState.AssertValidForNextState(header, data); err != nil {
+		if isExternalBlockValidationError(err) {
+			return errors.Join(errInvalidBlock, err)
+		}
 		return errors.Join(errInvalidState, err)
 	}
 	return nil
 }
 
+func isExternalBlockValidationError(err error) bool {
+	return errors.Is(err, types.ErrUnexpectedProposer) ||
+		errors.Is(err, types.ErrInvalidChainID) ||
+		errors.Is(err, types.ErrInvalidBlockHeight) ||
+		errors.Is(err, types.ErrInvalidBlockTime) ||
+		errors.Is(err, types.ErrHeaderDataMismatch) ||
+		errors.Is(err, types.ErrDataHashMismatch)
+}
+
 var errMaliciousProposer = errors.New("malicious proposer detected")
 
 // hashTx returns a hex-encoded SHA256 hash of the transaction.

block/internal/syncing/syncer_test.go

@@ -214,6 +214,144 @@ func TestSyncer_ValidateBlock_UsesStateNextProposer(t *testing.T) {
 	require.Contains(t, err.Error(), "unexpected proposer")
 }
 
+func TestSyncer_TrySyncNextBlock_ClassifiesExternalValidationFailures(t *testing.T) {
+	expectedAddr, expectedPub, expectedSigner := buildSyncTestSigner(t)
+	wrongAddr, wrongPub, wrongSigner := buildSyncTestSigner(t)
+
+	now := time.Now()
+	baseState := types.State{
+		ChainID:             "tchain",
+		InitialHeight:       1,
+		LastBlockHeight:     1,
+		LastBlockTime:       now,
+		LastHeaderHash:      []byte("last-header-hash"),
+		AppHash:             []byte("app0"),
+		NextProposerAddress: expectedAddr,
+	}
+
+	makeSyncer := func(tb testing.TB) *Syncer {
+		tb.Helper()
+		ds := dssync.MutexWrap(datastore.NewMapDatastore())
+		st := store.New(ds)
+		cm, err := cache.NewManager(config.DefaultConfig(), st, zerolog.Nop())
+		require.NoError(tb, err)
+
+		return &Syncer{
+			cache:   cm,
+			logger:  zerolog.Nop(),
+			options: common.DefaultBlockOptions(),
+		}
+	}
+
+	makeEvent := func(tb testing.TB, chainID string, proposer []byte, pub crypto.PubKey, signer signerpkg.Signer, appHash []byte, data *types.Data) common.DAHeightEvent {
+		tb.Helper()
+		_, header := makeSignedHeaderBytes(tb, chainID, 2, proposer, pub, signer, appHash, data, baseState.LastHeaderHash)
+		return common.DAHeightEvent{
+			Header: header,
+			Data:   data,
+			Source: common.SourceDA,
+		}
+	}
+
+	tests := map[string]struct {
+		event       func(testing.TB) common.DAHeightEvent
+		wantState   bool
+		wantInvalid bool
+	}{
+		"wrong proposer with bad app hash is an invalid external block": {
+			event: func(tb testing.TB) common.DAHeightEvent {
+				data := makeData(baseState.ChainID, 2, 1)
+				data.Metadata.Time = uint64(now.Add(time.Second).UnixNano())
+				return makeEvent(tb, baseState.ChainID, wrongAddr, wrongPub, wrongSigner, []byte("forged-app"), data)
+			},
+			wantInvalid: true,
+		},
+		"wrong chain ID is an invalid external block": {
+			event: func(tb testing.TB) common.DAHeightEvent {
+				data := makeData("other-chain", 2, 1)
+				data.Metadata.Time = uint64(now.Add(time.Second).UnixNano())
+				return makeEvent(tb, "other-chain", expectedAddr, expectedPub, expectedSigner, baseState.AppHash, data)
+			},
+			wantInvalid: true,
+		},
+		"data hash mismatch is an invalid external block": {
+			event: func(tb testing.TB) common.DAHeightEvent {
+				headerData := makeData(baseState.ChainID, 2, 1)
+				headerData.Metadata.Time = uint64(now.Add(time.Second).UnixNano())
+				event := makeEvent(tb, baseState.ChainID, expectedAddr, expectedPub, expectedSigner, baseState.AppHash, headerData)
+				event.Data = makeData(baseState.ChainID, 2, 2)
+				event.Data.Metadata.Time = headerData.Metadata.Time
+				return event
+			},
+			wantInvalid: true,
+		},
+		"header data metadata mismatch is an invalid external block": {
+			event: func(tb testing.TB) common.DAHeightEvent {
+				headerData := makeData(baseState.ChainID, 2, 1)
+				headerData.Metadata.Time = uint64(now.Add(time.Second).UnixNano())
+				event := makeEvent(tb, baseState.ChainID, expectedAddr, expectedPub, expectedSigner, baseState.AppHash, headerData)
+				event.Data = &types.Data{
+					Metadata: &types.Metadata{
+						ChainID: baseState.ChainID,
+						Height:  3,
+						Time:    headerData.Metadata.Time,
+					},
+					Txs: headerData.Txs,
+				}
+				return event
+			},
+			wantInvalid: true,
+		},
+		"expected proposer with bad app hash is invalid state": {
+			event: func(tb testing.TB) common.DAHeightEvent {
+				data := makeData(baseState.ChainID, 2, 1)
+				data.Metadata.Time = uint64(now.Add(time.Second).UnixNano())
+				return makeEvent(tb, baseState.ChainID, expectedAddr, expectedPub, expectedSigner, []byte("wrong-app"), data)
+			},
+			wantState: true,
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			event := tc.event(t)
+			err := makeSyncer(t).trySyncNextBlockWithState(t.Context(), &event, baseState)
+			require.Error(t, err)
+			assert.Equal(t, tc.wantInvalid, errors.Is(err, errInvalidBlock), "invalid block classification")
+			assert.Equal(t, tc.wantState, errors.Is(err, errInvalidState), "invalid state classification")
+		})
+	}
+}
+
+func TestSyncer_ValidateBlock_RejectsSignerAddressNotDerivedFromPubKey(t *testing.T) {
+	expectedAddr, _, _ := buildSyncTestSigner(t)
+	_, attackerPub, attackerSigner := buildSyncTestSigner(t)
+
+	now := time.Now()
+	state := types.State{
+		ChainID:             "tchain",
+		InitialHeight:       1,
+		LastBlockHeight:     1,
+		LastBlockTime:       now,
+		LastHeaderHash:      []byte("last-header-hash"),
+		AppHash:             []byte("app0"),
+		NextProposerAddress: expectedAddr,
+	}
+
+	data := makeData(state.ChainID, 2, 1)
+	data.Metadata.Time = uint64(now.Add(time.Second).UnixNano())
+	_, header := makeSignedHeaderBytes(t, state.ChainID, 2, expectedAddr, attackerPub, attackerSigner, state.AppHash, data, state.LastHeaderHash)
+
+	s := &Syncer{
+		logger:  zerolog.Nop(),
+		options: common.DefaultBlockOptions(),
+	}
+
+	err := s.ValidateBlock(t.Context(), state, data, header)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "signer address")
+}
+
 func TestSyncer_ApplyBlockPersistsExecutionNextProposer(t *testing.T) {
 	addr, _, _ := buildSyncTestSigner(t)
 	execNext := []byte("execution-next-proposer")

types/data.go

@@ -9,6 +9,14 @@ import (
 	"google.golang.org/protobuf/proto"
 )
 
+var (
+	// ErrHeaderDataMismatch is returned when header and data metadata do not describe the same block.
+	ErrHeaderDataMismatch = errors.New("header and data do not match")
+
+	// ErrDataHashMismatch is returned when a header's data hash does not match its block data.
+	ErrDataHashMismatch = errors.New("dataHash from the header does not match with hash of the block's data")
+)
+
 // Version captures the consensus rules for processing a block in the blockchain,
 // including all blockchain data structures and the rules of the application's
 // state transition machine.
@@ -60,14 +68,14 @@ func Validate(header *SignedHeader, data *Data) error {
 		if header.ChainID() != data.ChainID() ||
 			header.Height() != data.Height() ||
 			header.Time() != data.Time() { // skipping LastDataHash comparison as it needs access to previous header
-			return errors.New("header and data do not match")
+			return ErrHeaderDataMismatch
 		}
 	}
 	// exclude Metadata while computing the data hash for comparison
 	d := Data{Txs: data.Txs}
 	dataHash := d.DACommitment()
 	if !bytes.Equal(dataHash[:], header.DataHash[:]) {
-		return errors.New("dataHash from the header does not match with hash of the block's data")
+		return ErrDataHashMismatch
 	}
 	return nil
 }

types/signed_header.go

@@ -102,6 +102,12 @@ var (
 	// ErrProposerAddressMismatch is returned when the proposer address in the signed header does not match the proposer address in the validator set
 	ErrProposerAddressMismatch = errors.New("proposer address in SignedHeader does not match the proposer address in the validator set")
 
+	// ErrSignerPubKeyMissing is returned when the signed header does not include the signer public key.
+	ErrSignerPubKeyMissing = errors.New("signer public key is missing")
+
+	// ErrSignerAddressMismatch is returned when the signer address does not derive from the signer public key.
+	ErrSignerAddressMismatch = errors.New("signer address does not match signer public key")
+
 	// ErrSignatureEmpty is returned when signature is empty
 	ErrSignatureEmpty = errors.New("signature is empty")
 )
@@ -116,6 +122,10 @@ func (sh *SignedHeader) ValidateBasic() error {
 		return err
 	}
 
+	if err := sh.validateSignerIdentity(); err != nil {
+		return err
+	}
+
 	// Check that the proposer address in the signed header matches the proposer address in the validator set
 	if !bytes.Equal(sh.ProposerAddress, sh.Signer.Address) {
 		return ErrProposerAddressMismatch
@@ -195,6 +205,10 @@ func (sh *SignedHeader) ValidateBasicWithData(data *Data) error {
 		return err
 	}
 
+	if err := sh.validateSignerIdentity(); err != nil {
+		return err
+	}
+
 	// Check that the proposer address in the signed header matches the proposer address in the validator set
 	if !bytes.Equal(sh.ProposerAddress, sh.Signer.Address) {
 		return ErrProposerAddressMismatch
@@ -263,3 +277,13 @@ func (sh *SignedHeader) ValidateBasicWithData(data *Data) error {
 
 	return ErrSignatureVerificationFailed
 }
+
+func (sh *SignedHeader) validateSignerIdentity() error {
+	if sh.Signer.PubKey == nil {
+		return ErrSignerPubKeyMissing
+	}
+	if !bytes.Equal(KeyAddress(sh.Signer.PubKey), sh.Signer.Address) {
+		return ErrSignerAddressMismatch
+	}
+	return nil
+}

types/state.go

@@ -21,6 +21,23 @@ var InitStateVersion = Version{
 // different from the proposer expected by the current state.
 var ErrUnexpectedProposer = errors.New("unexpected proposer")
 
+var (
+	// ErrInvalidChainID is returned when a header belongs to a different chain than the current state.
+	ErrInvalidChainID = errors.New("invalid chain ID")
+
+	// ErrInvalidBlockHeight is returned when a header is not the next height after the current state.
+	ErrInvalidBlockHeight = errors.New("invalid block height")
+
+	// ErrInvalidBlockTime is returned when a header time is earlier than the current state's block time.
+	ErrInvalidBlockTime = errors.New("invalid block time")
+
+	// ErrInvalidLastHeaderHash is returned when a header does not link to the current state's last header.
+	ErrInvalidLastHeaderHash = errors.New("invalid last header hash")
+
+	// ErrInvalidLastAppHash is returned when a header does not reference the current state's app hash.
+	ErrInvalidLastAppHash = errors.New("invalid last app hash")
+)
+
 // State contains information about current state of the blockchain.
 type State struct {
 	Version Version
@@ -74,13 +91,22 @@ func (s *State) NextState(header Header, stateRoot []byte, nextProposerAddress .
 // AssertValidForNextState performs common validation of a header and data against the current state.
 // It assumes any context-specific basic header checks and verifier setup have already been performed
 func (s State) AssertValidForNextState(header *SignedHeader, data *Data) error {
+	if err := s.AssertExpectedProposer(header); err != nil {
+		return err
+	}
+
 	if err := s.AssertValidSequence(header); err != nil {
 		return err
 	}
 
 	if err := Validate(header, data); err != nil {
 		return fmt.Errorf("header-data validation failed: %w", err)
 	}
+	return nil
+}
+
+// AssertExpectedProposer checks that the header was signed by the proposer expected for the next block.
+func (s State) AssertExpectedProposer(header *SignedHeader) error {
 	if len(s.NextProposerAddress) > 0 && !bytes.Equal(header.ProposerAddress, s.NextProposerAddress) {
 		return fmt.Errorf("%w - got: %x, want: %x", ErrUnexpectedProposer, header.ProposerAddress, s.NextProposerAddress)
 	}
@@ -95,27 +121,27 @@ var (
 // AssertValidSequence performs lightweight state-sequence validation for self-produced blocks.
 func (s State) AssertValidSequence(header *SignedHeader) error {
 	if header.ChainID() != s.ChainID {
-		return fmt.Errorf("invalid chain ID - got %s, want %s", header.ChainID(), s.ChainID)
+		return fmt.Errorf("%w - got %s, want %s", ErrInvalidChainID, header.ChainID(), s.ChainID)
 	}
 
 	if len(s.LastHeaderHash) == 0 { // initial state
 		return nil
 	}
 
 	if expdHeight := s.LastBlockHeight + 1; header.Height() != expdHeight {
-		return fmt.Errorf("invalid block height - got: %d, want: %d", header.Height(), expdHeight)
+		return fmt.Errorf("%w - got: %d, want: %d", ErrInvalidBlockHeight, header.Height(), expdHeight)
 	}
 
 	if headerTime := header.Time(); s.LastBlockTime.After(headerTime) {
-		return fmt.Errorf("invalid block time - got: %v, last: %v", headerTime, s.LastBlockTime)
+		return fmt.Errorf("%w - got: %v, last: %v", ErrInvalidBlockTime, headerTime, s.LastBlockTime)
 	}
 
 	// Trick to support the switch from a base sequencer to a normal syncing node.
 	// Based sequencers do not sign ehaders, meaning the last header hash will be different from the
 	// newly derived header hash when a base sequencer have switched to a syncing node
 	if !bytes.Equal(header.LastHeaderHash, s.LastHeaderHash) {
 		if lastHeaderHashErrCount == 1 {
-			return fmt.Errorf("invalid last header hash - got: %x, want: %x", header.LastHeaderHash, s.LastHeaderHash)
+			return fmt.Errorf("%w - got: %x, want: %x", ErrInvalidLastHeaderHash, header.LastHeaderHash, s.LastHeaderHash)
 		}
 
 		basedSequencerTracking.Do(func() {
@@ -124,7 +150,7 @@ func (s State) AssertValidSequence(header *SignedHeader) error {
 	}
 
 	if !bytes.Equal(header.AppHash, s.AppHash) {
-		return fmt.Errorf("invalid last app hash - got: %x, want: %x", header.AppHash, s.AppHash)
+		return fmt.Errorf("%w - got: %x, want: %x", ErrInvalidLastAppHash, header.AppHash, s.AppHash)
 	}
 
 	return nil