Add nil-element validation to flow data models

consensus/hotstuff/model/timeout.go

@@ -94,13 +94,19 @@ func NewTimeoutObject(untrusted UntrustedTimeoutObject) (*TimeoutObject, error)
 	// If a TC is included, the TC must be for the past round, no matter whether a QC
 	// for the last round is also included. In some edge cases, a node might observe
 	// _both_ QC and TC for the previous round, in which case it can include both.
+	var lastViewTC *flow.TimeoutCertificate
 	if untrusted.LastViewTC != nil {
 		if untrusted.View != untrusted.LastViewTC.View+1 {
 			return nil, fmt.Errorf("invalid TC for non-previous view, expected view %d, got view %d", untrusted.View-1, untrusted.LastViewTC.View)
 		}
-		if untrusted.NewestQC.View < untrusted.LastViewTC.NewestQC.View {
-			return nil, fmt.Errorf("timeout.NewestQC is older (view=%d) than the QC in timeout.LastViewTC (view=%d)", untrusted.NewestQC.View, untrusted.LastViewTC.NewestQC.View)
+		tc, err := flow.NewTimeoutCertificate(flow.UntrustedTimeoutCertificate(*untrusted.LastViewTC))
+		if err != nil {
+			return nil, fmt.Errorf("invalid LastViewTC: %w", err)
 		}
+		if untrusted.NewestQC.View < tc.NewestQC.View {
+			return nil, fmt.Errorf("timeout.NewestQC is older (view=%d) than the QC in timeout.LastViewTC (view=%d)", untrusted.NewestQC.View, tc.NewestQC.View)
+		}
+		lastViewTC = tc
 	}
 	// The TO must contain a proof that sender legitimately entered View. Transitioning
 	// to round timeout.View is possible either by observing a QC or a TC for the previous round.
@@ -118,7 +124,7 @@ func NewTimeoutObject(untrusted UntrustedTimeoutObject) (*TimeoutObject, error)
 	return &TimeoutObject{
 		View:        untrusted.View,
 		NewestQC:    untrusted.NewestQC,
-		LastViewTC:  untrusted.LastViewTC,
+		LastViewTC:  lastViewTC,
 		SignerID:    untrusted.SignerID,
 		SigData:     untrusted.SigData,
 		TimeoutTick: untrusted.TimeoutTick,

consensus/hotstuff/model/timeout_test.go

@@ -195,13 +195,15 @@ func TestNewTimeoutObject(t *testing.T) {
 	})
 
 	t.Run("invalid input when TimeoutObject's QC is older than TC's QC", func(t *testing.T) {
+		// TC must be valid: TC.View (199) >= TC.NewestQC.View (150).
+		// TO.NewestQC.View (80) < TC.NewestQC.View (150) triggers the error.
 		tcQC := helper.MakeQC(helper.WithQCView(150))
-		tc := helper.MakeTC(helper.WithTCNewestQC(tcQC), helper.WithTCView(99))
+		tc := helper.MakeTC(helper.WithTCNewestQC(tcQC), helper.WithTCView(199))
 
 		res, err := model.NewTimeoutObject(
 			model.UntrustedTimeoutObject(
 				*helper.TimeoutObjectFixture(
-					helper.WithTimeoutObjectView(100),
+					helper.WithTimeoutObjectView(200), // must be TC.View+1
 					helper.WithTimeoutLastViewTC(tc),
 					helper.WithTimeoutNewestQC(helper.MakeQC(helper.WithQCView(80))), // older than TC.NewestQC
 				),
@@ -212,6 +214,28 @@ func TestNewTimeoutObject(t *testing.T) {
 		assert.Contains(t, err.Error(), "timeout.NewestQC is older")
 	})
 
+	t.Run("invalid input when LastViewTC has nil NewestQC", func(t *testing.T) {
+		// A non-nil TC whose nested NewestQC is nil must be rejected; this is the
+		// exact attack vector from the security audit (CBOR null → nil pointer).
+		// Build a TC that is initially valid (TC.View=199 >= TC.NewestQC.View=150),
+		// then poison the nested NewestQC pointer to nil.
+		tc := helper.MakeTC(helper.WithTCNewestQC(helper.MakeQC(helper.WithQCView(150))), helper.WithTCView(199))
+		tc.NewestQC = nil // poison the nested pointer
+
+		res, err := model.NewTimeoutObject(
+			model.UntrustedTimeoutObject(
+				*helper.TimeoutObjectFixture(
+					helper.WithTimeoutObjectView(200), // TO.View == TC.View+1
+					helper.WithTimeoutLastViewTC(tc),
+					helper.WithTimeoutNewestQC(helper.MakeQC(helper.WithQCView(100))), // TO.NewestQC.View < 200
+				),
+			),
+		)
+		require.Error(t, err)
+		require.Nil(t, res)
+		assert.Contains(t, err.Error(), "invalid LastViewTC")
+	})
+
 	t.Run("invalid input when no QC for previous round and TC is missing", func(t *testing.T) {
 		qc := helper.MakeQC(helper.WithQCView(90))
 

model/flow/block_test.go

@@ -118,18 +118,45 @@ func TestBlock_Status(t *testing.T) {
 	}
 }
 
+// blockWithLastViewTC returns a FullBlockFixture that is guaranteed to have a non-nil LastViewTC.
+// HeaderBodyWithParentFixture omits LastViewTC when view == parent.View+1 (1-in-10 chance).
+// NewHeaderBody now validates LastViewTC via NewTimeoutCertificate, so mutating a nil TC to a
+// zero-value struct would cause hashModel() to panic in the malleability checker.
+func blockWithLastViewTC() *flow.Block {
+	const maxAttempts = 1000
+	for i := 0; i < maxAttempts; i++ {
+		if b := unittest.FullBlockFixture(); b.LastViewTC != nil {
+			return b
+		}
+	}
+	panic("failed to generate FullBlockFixture with non-nil LastViewTC")
+}
+
 // TestBlockMalleability checks that flow.Block is not malleable: any change in its data
 // should result in a different ID.
-// Because our NewHeaderBody constructor enforces ParentView < View we use
-// WithFieldGenerator to safely pass it.
+// NewHeaderBody enforces ParentView < View and validates LastViewTC via NewTimeoutCertificate,
+// so WithFieldGenerator is used for both to keep those constraints intact.
 func TestBlockMalleability(t *testing.T) {
-	block := unittest.FullBlockFixture()
+	block := blockWithLastViewTC()
 	unittest.RequireEntityNonMalleable(
 		t,
-		unittest.FullBlockFixture(),
+		block,
 		unittest.WithFieldGenerator("HeaderBody.ParentView", func() uint64 {
 			return block.View - 1 // ParentView must stay below View, so set it to View-1
 		}),
+		// The field generator for LastViewTC must return the struct value (not a pointer):
+		// isModelMalleable dereferences *TimeoutCertificate before invoking the generator,
+		// so modelOrField is flow.TimeoutCertificate at that point.
+		unittest.WithFieldGenerator("HeaderBody.LastViewTC", func() flow.TimeoutCertificate {
+			qc := unittest.QuorumCertificateFixture()
+			return flow.TimeoutCertificate{
+				View:          qc.View + 1,
+				NewestQCViews: []uint64{qc.View},
+				NewestQC:      qc,
+				SignerIndices: unittest.SignerIndicesFixture(4),
+				SigData:       unittest.SignatureFixture(),
+			}
+		}),
 		unittest.WithFieldGenerator("Payload.Results", func() flow.ExecutionResultList {
 			return flow.ExecutionResultList{unittest.ExecutionResultFixture()}
 		}),

model/flow/chunk.go

@@ -230,11 +230,22 @@ func NewChunkDataPack(untrusted UntrustedChunkDataPack) (*ChunkDataPack, error)
 		return nil, fmt.Errorf("ExecutionDataRoot.ChunkExecutionDataIDs must not be empty")
 	}
 
+	var collection *Collection
+	if untrusted.Collection != nil {
+		c, err := NewCollection(UntrustedCollection{
+			Transactions: untrusted.Collection.Transactions,
+		})
+		if err != nil {
+			return nil, fmt.Errorf("invalid collection: %w", err)
+		}
+		collection = c
+	}
+
 	return &ChunkDataPack{
 		ChunkID:           untrusted.ChunkID,
 		StartState:        untrusted.StartState,
 		Proof:             untrusted.Proof,
-		Collection:        untrusted.Collection,
+		Collection:        collection,
 		ExecutionDataRoot: untrusted.ExecutionDataRoot,
 	}, nil
 }

model/flow/chunk_test.go

@@ -267,6 +267,18 @@ func TestFromUntrustedChunkDataPack(t *testing.T) {
 		assert.Nil(t, pack)
 		assert.Contains(t, err.Error(), "ExecutionDataRoot.ChunkExecutionDataIDs")
 	})
+
+	t.Run("Collection with nil transaction element rejected", func(t *testing.T) {
+		untrusted := baseChunkDataPack
+		untrusted.Collection = &flow.Collection{
+			Transactions: []*flow.TransactionBody{nil},
+		}
+
+		pack, err := flow.NewChunkDataPack(untrusted)
+		assert.Error(t, err)
+		assert.Nil(t, pack)
+		assert.Contains(t, err.Error(), "invalid collection")
+	})
 }
 
 // TestNewChunk verifies that NewChunk constructs a valid Chunk when given

model/flow/execution_result.go

@@ -47,6 +47,11 @@ func NewExecutionResult(untrusted UntrustedExecutionResult) (*ExecutionResult, e
 	if len(untrusted.Chunks) == 0 {
 		return nil, fmt.Errorf("Chunks must not be empty")
 	}
+	for i, ch := range untrusted.Chunks {
+		if ch == nil {
+			return nil, fmt.Errorf("chunk at index %d is nil", i)
+		}
+	}
 
 	if untrusted.ExecutionDataID == ZeroID {
 		return nil, fmt.Errorf("ExecutionDataID must not be empty")

model/flow/execution_result_test.go

@@ -244,6 +244,19 @@ func TestNewExecutionResult(t *testing.T) {
 		assert.Nil(t, res)
 		assert.Contains(t, err.Error(), "ExecutionDataID")
 	})
+
+	t.Run("nil Chunk element rejected", func(t *testing.T) {
+		u := flow.UntrustedExecutionResult{
+			PreviousResultID: validPrevID,
+			BlockID:          validBlockID,
+			Chunks:           flow.ChunkList{nil},
+			ExecutionDataID:  validExecDataID,
+		}
+		res, err := flow.NewExecutionResult(u)
+		assert.Error(t, err)
+		assert.Nil(t, res)
+		assert.Contains(t, err.Error(), "chunk at index 0 is nil")
+	})
 }
 
 // TestNewRootExecutionResult verifies the behavior of the NewRootExecutionResult constructor.

model/flow/header.go

@@ -111,8 +111,27 @@ func NewHeaderBody(untrusted UntrustedHeaderBody) (*HeaderBody, error) {
 		return nil, fmt.Errorf("Timestamp must not be zero-value")
 	}
 
-	hb := HeaderBody(untrusted)
-	return &hb, nil
+	var lastViewTC *TimeoutCertificate
+	if untrusted.LastViewTC != nil {
+		tc, err := NewTimeoutCertificate(UntrustedTimeoutCertificate(*untrusted.LastViewTC))
+		if err != nil {
+			return nil, fmt.Errorf("invalid LastViewTC: %w", err)
+		}
+		lastViewTC = tc
+	}
+
+	return &HeaderBody{
+		ChainID:            untrusted.ChainID,
+		ParentID:           untrusted.ParentID,
+		Height:             untrusted.Height,
+		Timestamp:          untrusted.Timestamp,
+		View:               untrusted.View,
+		ParentView:         untrusted.ParentView,
+		ParentVoterIndices: untrusted.ParentVoterIndices,
+		ParentVoterSigData: untrusted.ParentVoterSigData,
+		ProposerID:         untrusted.ProposerID,
+		LastViewTC:         lastViewTC,
+	}, nil
 }
 
 // NewRootHeaderBody creates a new instance of root HeaderBody.

model/flow/header_test.go

@@ -314,6 +314,22 @@ func TestNewHeaderBody(t *testing.T) {
 		assert.Nil(t, hb)
 		assert.Contains(t, err.Error(), "Timestamp must not be zero-value")
 	})
+
+	t.Run("non-nil LastViewTC with nil NewestQC rejected", func(t *testing.T) {
+		u := UntrustedHeaderBodyFixture(func(u *flow.UntrustedHeaderBody) {
+			u.LastViewTC = &flow.TimeoutCertificate{
+				View:          u.View - 1,
+				NewestQCViews: []uint64{u.View - 2},
+				NewestQC:      nil, // nil nested pointer — the attack vector
+				SignerIndices: unittest.SignerIndicesFixture(4),
+				SigData:       unittest.SignatureFixture(),
+			}
+		})
+		hb, err := flow.NewHeaderBody(u)
+		assert.Error(t, err)
+		assert.Nil(t, hb)
+		assert.Contains(t, err.Error(), "invalid LastViewTC")
+	})
 }
 
 // TestHeaderBodyBuilder_PresenceChecks verifies that HeaderBodyBuilder.Build

model/flow/payload.go

@@ -64,6 +64,11 @@ func NewPayload(untrusted UntrustedPayload) (*Payload, error) {
 		if r == nil {
 			return nil, fmt.Errorf("result at index %d is nil", i)
 		}
+		for j, ch := range r.Chunks {
+			if ch == nil {
+				return nil, fmt.Errorf("chunk at index %d in result at index %d is nil", j, i)
+			}
+		}
 	}
 
 	return &Payload{

model/flow/payload_test.go

@@ -133,4 +133,19 @@ func TestNewPayload(t *testing.T) {
 		require.Nil(t, res)
 		require.Contains(t, err.Error(), "result at index 0 is nil")
 	})
+
+	t.Run("nil Chunk element in Result rejected", func(t *testing.T) {
+		er := unittest.ExecutionResultFixture()
+		er.Chunks = flow.ChunkList{nil}
+
+		untrusted := flow.UntrustedPayload(unittest.PayloadFixture(
+			unittest.WithProtocolStateID(unittest.IdentifierFixture()),
+		))
+		untrusted.Results = flow.ExecutionResultList{er}
+
+		res, err := flow.NewPayload(untrusted)
+		require.Error(t, err)
+		require.Nil(t, res)
+		require.Contains(t, err.Error(), "chunk at index 0 in result at index 0 is nil")
+	})
 }