adds missing check for Seal.FinalState

.gitignore

@@ -84,3 +84,4 @@ tps-results*.json
 # Custom golangcilint build
 custom-gcl
 tools/custom-gcl
+ai-notes/

module/validation/seal_validator.go

@@ -8,6 +8,7 @@ import (
 	"github.com/onflow/flow-go/engine"
 	"github.com/onflow/flow-go/model/flow"
 	"github.com/onflow/flow-go/module"
+	"github.com/onflow/flow-go/module/irrecoverable"
 	"github.com/onflow/flow-go/module/signature"
 	"github.com/onflow/flow-go/state/fork"
 	"github.com/onflow/flow-go/state/protocol"
@@ -241,18 +242,39 @@ func (s *sealValidator) Validate(candidate *flow.Block) (*flow.Seal, error) {
 	return latestSeal, nil
 }
 
-// validateSeal performs integrity checks of single seal. To be valid, we
-// require that seal:
-// 1) Contains correct number of approval signatures, one aggregated sig for each chunk.
-// 2) Every aggregated signature contains valid signer ids. module.ChunkAssigner is used to perform this check.
-// 3) Every aggregated signature contains valid signatures.
+// validateSeal performs integrity checks of single seal. To be valid, we require that the seal:
+//  1. `Seal.FinalState` is equal to the final state as committed to by the sealed execution result.
+//  2. Contains correct number of approval signatures, one aggregated sig for each chunk.
+//  3. Every aggregated signature contains valid signer ids. `module.ChunkAssigner` is used to perform this check.
+//  4. Every aggregated signature contains valid signatures.
+//
 // Returns:
-// * nil - in case of success
-// * engine.InvalidInputError - in case of malformed seal
-// * exception - in case of unexpected error
+//   - nil - in case of success
+//   - engine.InvalidInputError - in case of malformed seal
+//   - exception - in case of unexpected error
 func (s *sealValidator) validateSeal(seal *flow.Seal, incorporatedResult *flow.IncorporatedResult) error {
 	executionResult := incorporatedResult.Result
 
+	// Check that `Seal.FinalState` equals the final state of the execution result, i.e. the last chunk's `EndState`, returned by
+	// `executionResult.FinalStateCommitment()`. `Seal.FinalState` is a redundant copy of that value, included so that callers such as
+	// `Snapshot.Commit` can read the sealed state commitment directly from the seal without looking up the execution result. The verifier
+	// signatures are computed over the fields `{chunk.BlockID, ExecutionResultID, chunk.Index}` for each chunk in the execution result,
+	// so tampering with those fields in the seal would invalidate verifier signatures in seal, which is caught when checking those
+	// verifier signatures. Verifiers check the `IncorporatedResult` while consensus nodes check the correct construction of the seal for
+	// the `IncorporatedResult`. So the only remaining field that a Byzantine proposer could tamper with is the `Seal.FinalState`. We check
+	// this field first before the  per-chunk work, because the check is cheap. But formally, there is no order dependence of the checks.
+	expectedFinalState, err := executionResult.FinalStateCommitment()
+	if err != nil {
+		// `FinalStateCommitment` is documented to fail only with `flow.ErrNoChunks`. That error cannot occur here: `incorporatedResult.Result`
+		// was loaded from local storage after being incorporated in an ancestor of the candidate block (not the candidate itself), so
+		// `ReceiptValidator.verifyChunksFormat` has already enforced `Chunks.Len() ≥ 1` (the system chunk is mandatory). Any error observed here
+		// therefore indicates a bug or storage corruption.
+		return irrecoverable.NewExceptionf("could not derive final state commitment for execution result %x: %w", executionResult.ID(), err)
+	}
+	if seal.FinalState != expectedFinalState {
+		return engine.NewInvalidInputErrorf("seal's final state %x does not match execution result (final state %x)", seal.FinalState, expectedFinalState)
+	}
+
 	// check that each chunk has an AggregatedSignature
 	if len(seal.AggregatedApprovalSigs) != executionResult.Chunks.Len() {
 		return engine.NewInvalidInputErrorf("mismatching signatures, expected: %d, got: %d",

module/validation/seal_validator_test.go

@@ -272,6 +272,49 @@ func (s *SealValidationSuite) TestSealDuplicatedApproval() {
 	s.Require().True(engine.IsInvalidInputError(err))
 }
 
+// TestSealInvalidFinalState verifies that we reject a seal whose `FinalState` does not equal the
+// final state from the sealed execution result (i.e. the last chunk's `EndState`).
+//
+// Background: `Seal.FinalState` is an auxiliary field whose value is supposed to copied from the
+// sealed `ExecutionResult` (specifically `executionResult.FinalStateCommitment()`). The verifier
+// attestations only cover `{BlockID, ExecutionResultID, ChunkIndex}` from the execution result.
+// Verifiers do _not_ commit to `Seal.FinalState`. Without an explicit check by consensus nodes in
+// the seal validator, a Byzantine consensus node could propose a block payload with valid aggregated
+// approvals for a real incorporated execution result but set `Seal.FinalState` to an arbitrary non-empty
+// value. The downstream `Snapshot.Commit` would then expose the poisoned commitment as the sealed state.
+//
+// We test with the following fork:
+//
+//	... <- LatestSealedBlock <- B0 <- B1{ Result[B0], Receipt[B0] } <- B2 <- ░newBlock{ Seal[B0]}░
+//
+// The gap of 1 block, i.e. B2, is required to avoid a sealing edge-case
+// (see test `TestSeal_EnforceGap` for more details).
+func (s *SealValidationSuite) TestSealInvalidFinalState() {
+	_, _, newBlock, receipt, seal := s.generateBasicTestFork()
+
+	// Sanity check for correct testing setup: the fixture's seal must agree with the execution result's final state.
+	// This guarantees that the subsequent mutation below is the _only_ deviation between the seal and the result.
+	expectedFinalState, err := receipt.ExecutionResult.FinalStateCommitment()
+	s.Require().NoError(err)
+	s.Require().Equal(expectedFinalState, seal.FinalState)
+
+	// Attack Details: The Byzantine proposer publishes `newBlock` containing a seal for B0. The byzantine proposer complies with the
+	// protocol in that it includes the required number of valid verifier signatures in the seal. It truthfully sets all fields in the
+	// seal that are covered by the verifier signatures (BlockID, ResultID, ChunkIndex, AggregatedApprovalSigs), to not invalidate the
+	// aggregated verifier signatures. However, while the protocol mandates that `Seal.FinalState` is set to the `FinalStateCommitment`
+	// of the execution result previously incorporated in the fork, the byzantine proposer chooses a conflicting value.
+	// To mount such an attack, the proposer would build (and sign) their proposal with the poisoned `Seal.FinalState` from the start.
+	// The test takes the shortcut of mutating `Seal.FinalState` in place because `sealValidator.Validate` only inspects the block
+	// payload; proposer signatures will have been checked in production by the compliance layer before (which we omit here).
+	seal.FinalState[0] ^= 0xff                                       // bitwise XOR with `0xFF`: inverts all bits
+	s.Require().NotEqual(flow.EmptyStateCommitment, seal.FinalState) // remains non-empty (NewSeal precondition)
+	s.Require().NotEqual(expectedFinalState, seal.FinalState)
+
+	_, err = s.sealValidator.Validate(newBlock)
+	s.Require().Error(err)
+	s.Require().True(engine.IsInvalidInputError(err), err)
+}
+
 // TestSealInvalidChunkAssignment tests that we reject seal with invalid signerID of approval signature for
 // submitted seal. We test with the following fork:
 //