From c63dba4df790b3f02070467a253a22a903ef3f5b Mon Sep 17 00:00:00 2001 From: Alex Hentschel Date: Tue, 19 May 2026 22:00:21 -0700 Subject: [PATCH 1/5] first AI DRAFT - NOT yet REVIEWED by human; proposed fix for https://github.com/onflow/flow-go-internal/issues/7214; Summary: added a single localized check at the top of `validateSeal` method that retrieves the verified final state from `executionResult.FinalStateCommitment()` and rejects seals with mismatching `Seal.FinalState` by returning an `engine.NewInvalidInputErrorf` --- .gitignore | 1 + module/validation/seal_validator.go | 32 ++++++++++++++++---- module/validation/seal_validator_test.go | 38 ++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 6 deletions(-) diff --git a/.gitignore b/.gitignore index 243f2d008a0..6dee1dfe72f 100644 --- a/.gitignore +++ b/.gitignore @@ -84,3 +84,4 @@ tps-results*.json # Custom golangcilint build custom-gcl tools/custom-gcl +ai-notes/ diff --git a/module/validation/seal_validator.go b/module/validation/seal_validator.go index c6c5341a97e..28179e609d0 100644 --- a/module/validation/seal_validator.go +++ b/module/validation/seal_validator.go @@ -243,16 +243,36 @@ func (s *sealValidator) Validate(candidate *flow.Block) (*flow.Seal, error) { // 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. +// 1. Commits to the final state derived from the referenced execution result. `Seal.FinalState` +// is redundant data (the authoritative value is `executionResult.FinalStateCommitment()`); the +// verifier attestations only bind {BlockID, ExecutionResultID, ChunkIndex} and do _not_ cover +// `Seal.FinalState`. Without this explicit binding, an authorized Byzantine consensus proposer +// could swap in an arbitrary non-empty `FinalState` while keeping the approval signatures valid. +// 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 + // Bind `Seal.FinalState` to the referenced execution result. We do this _before_ all other + // per-chunk checks so that a poisoned `FinalState` is rejected as early as possible. + // `FinalStateCommitment` only errors with `ErrNoChunks`, which cannot occur here because a + // valid `ExecutionResult` always has at least one chunk (enforced by `flow.NewExecutionResult`). + // Any error returned therefore signals a corrupted/malformed execution result that bypassed + // upstream validation — treat as exception, not a benign input error. + expectedFinalState, err := executionResult.FinalStateCommitment() + if err != nil { + return fmt.Errorf("could not derive final state commitment from execution result %x: %w", executionResult.ID(), err) + } + if seal.FinalState != expectedFinalState { + return engine.NewInvalidInputErrorf("seal final state does not match execution result final state") + } + // check that each chunk has an AggregatedSignature if len(seal.AggregatedApprovalSigs) != executionResult.Chunks.Len() { return engine.NewInvalidInputErrorf("mismatching signatures, expected: %d, got: %d", diff --git a/module/validation/seal_validator_test.go b/module/validation/seal_validator_test.go index f88d98f4e21..3aaf6dbb27f 100644 --- a/module/validation/seal_validator_test.go +++ b/module/validation/seal_validator_test.go @@ -272,6 +272,44 @@ 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 derived from the referenced execution result (i.e. the last chunk's `EndState`). +// +// Background: `Seal.FinalState` is a supplied field whose authoritative value is already determined +// by the referenced `ExecutionResult` (via `executionResult.FinalStateCommitment()`). The verifier +// attestations only cover `{BlockID, ExecutionResultID, ChunkIndex}` — they do _not_ commit to +// `Seal.FinalState`. Without an explicit binding check in the seal validator, an authorized +// Byzantine consensus proposer could build 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: 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) + + // Mutate one byte of `FinalState` to model the Byzantine proposer who keeps all attestation-bound + // fields (BlockID, ResultID, ChunkIndex, AggregatedApprovalSigs) intact but tampers with the + // redundant `Seal.FinalState`. The seal pointer is already included in `newBlock`'s payload. + seal.FinalState[0] ^= 0xff + 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: // From a12459b7ca18d21bffe4cbd67b0a53a12661c806 Mon Sep 17 00:00:00 2001 From: Alex Hentschel Date: Tue, 19 May 2026 22:30:06 -0700 Subject: [PATCH 2/5] updated reasoning for raising an irrecoverable error if no final state commitment is available for the sealed result --- module/validation/seal_validator.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/module/validation/seal_validator.go b/module/validation/seal_validator.go index 28179e609d0..a141be5f9df 100644 --- a/module/validation/seal_validator.go +++ b/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" @@ -259,18 +260,19 @@ func (s *sealValidator) Validate(candidate *flow.Block) (*flow.Seal, error) { func (s *sealValidator) validateSeal(seal *flow.Seal, incorporatedResult *flow.IncorporatedResult) error { executionResult := incorporatedResult.Result - // Bind `Seal.FinalState` to the referenced execution result. We do this _before_ all other - // per-chunk checks so that a poisoned `FinalState` is rejected as early as possible. - // `FinalStateCommitment` only errors with `ErrNoChunks`, which cannot occur here because a - // valid `ExecutionResult` always has at least one chunk (enforced by `flow.NewExecutionResult`). - // Any error returned therefore signals a corrupted/malformed execution result that bypassed - // upstream validation — treat as exception, not a benign input error. + // Bind `Seal.FinalState` to the referenced execution result. We run this check first so a + // poisoned `FinalState` is rejected before any of the more expensive per-chunk work. + // + // `FinalStateCommitment` is documented to fail only with `flow.ErrNoChunks`. That error is unreachable here: `executionResult` is + // loaded from local storage and was incorporated in a strict ancestor of the candidate block, so `ReceiptValidator.verifyChunksFormat` + // has already enforced `Chunks.Len() ≥ 1` (the system chunk is mandatory). A `nil` result pointer is likewise impossible (rejected by + // `flow.NewIncorporatedResult`). Any error observed here therefore indicates a bug or storage corruption. expectedFinalState, err := executionResult.FinalStateCommitment() if err != nil { - return fmt.Errorf("could not derive final state commitment from execution result %x: %w", executionResult.ID(), err) + return irrecoverable.NewExceptionf("could not retrieve final state commitment from incorporated execution result %x: %w", executionResult.ID(), err) } if seal.FinalState != expectedFinalState { - return engine.NewInvalidInputErrorf("seal final state does not match execution result final state") + return engine.NewInvalidInputErrorf("seal's final state does not match execution result") } // check that each chunk has an AggregatedSignature From d5f80cdc2a207348901a7163a214dc34dfff2cb9 Mon Sep 17 00:00:00 2001 From: Alex Hentschel Date: Tue, 19 May 2026 22:31:26 -0700 Subject: [PATCH 3/5] wording polish --- module/validation/seal_validator.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/module/validation/seal_validator.go b/module/validation/seal_validator.go index a141be5f9df..cab9b64da6c 100644 --- a/module/validation/seal_validator.go +++ b/module/validation/seal_validator.go @@ -260,8 +260,8 @@ func (s *sealValidator) Validate(candidate *flow.Block) (*flow.Seal, error) { func (s *sealValidator) validateSeal(seal *flow.Seal, incorporatedResult *flow.IncorporatedResult) error { executionResult := incorporatedResult.Result - // Bind `Seal.FinalState` to the referenced execution result. We run this check first so a - // poisoned `FinalState` is rejected before any of the more expensive per-chunk work. + // Verify `Seal.FinalState` matches the execution result. We run this check first so a + // byzantine `FinalState` is rejected before any of the more expensive per-chunk work. // // `FinalStateCommitment` is documented to fail only with `flow.ErrNoChunks`. That error is unreachable here: `executionResult` is // loaded from local storage and was incorporated in a strict ancestor of the candidate block, so `ReceiptValidator.verifyChunksFormat` From 24a0df32bfff302c7f55b061a1685ff6b17fe1ca Mon Sep 17 00:00:00 2001 From: Alex Hentschel Date: Wed, 20 May 2026 12:42:24 -0700 Subject: [PATCH 4/5] updating documentation --- module/validation/seal_validator.go | 30 ++++++++++++------------ module/validation/seal_validator_test.go | 23 +++++++++++------- 2 files changed, 29 insertions(+), 24 deletions(-) diff --git a/module/validation/seal_validator.go b/module/validation/seal_validator.go index cab9b64da6c..a75d434c5f3 100644 --- a/module/validation/seal_validator.go +++ b/module/validation/seal_validator.go @@ -242,13 +242,8 @@ 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. Commits to the final state derived from the referenced execution result. `Seal.FinalState` -// is redundant data (the authoritative value is `executionResult.FinalStateCommitment()`); the -// verifier attestations only bind {BlockID, ExecutionResultID, ChunkIndex} and do _not_ cover -// `Seal.FinalState`. Without this explicit binding, an authorized Byzantine consensus proposer -// could swap in an arbitrary non-empty `FinalState` while keeping the approval signatures valid. +// 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. @@ -260,16 +255,21 @@ func (s *sealValidator) Validate(candidate *flow.Block) (*flow.Seal, error) { func (s *sealValidator) validateSeal(seal *flow.Seal, incorporatedResult *flow.IncorporatedResult) error { executionResult := incorporatedResult.Result - // Verify `Seal.FinalState` matches the execution result. We run this check first so a - // byzantine `FinalState` is rejected before any of the more expensive per-chunk work. - // - // `FinalStateCommitment` is documented to fail only with `flow.ErrNoChunks`. That error is unreachable here: `executionResult` is - // loaded from local storage and was incorporated in a strict ancestor of the candidate block, so `ReceiptValidator.verifyChunksFormat` - // has already enforced `Chunks.Len() ≥ 1` (the system chunk is mandatory). A `nil` result pointer is likewise impossible (rejected by - // `flow.NewIncorporatedResult`). Any error observed here therefore indicates a bug or storage corruption. + // 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 { - return irrecoverable.NewExceptionf("could not retrieve final state commitment from incorporated execution result %x: %w", executionResult.ID(), err) + // `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 does not match execution result") diff --git a/module/validation/seal_validator_test.go b/module/validation/seal_validator_test.go index 3aaf6dbb27f..d0d7ddbae09 100644 --- a/module/validation/seal_validator_test.go +++ b/module/validation/seal_validator_test.go @@ -273,13 +273,13 @@ func (s *SealValidationSuite) TestSealDuplicatedApproval() { } // TestSealInvalidFinalState verifies that we reject a seal whose `FinalState` does not equal the -// final state derived from the referenced execution result (i.e. the last chunk's `EndState`). +// final state from the sealed execution result (i.e. the last chunk's `EndState`). // // Background: `Seal.FinalState` is a supplied field whose authoritative value is already determined // by the referenced `ExecutionResult` (via `executionResult.FinalStateCommitment()`). The verifier // attestations only cover `{BlockID, ExecutionResultID, ChunkIndex}` — they do _not_ commit to -// `Seal.FinalState`. Without an explicit binding check in the seal validator, an authorized -// Byzantine consensus proposer could build a block payload with valid aggregated approvals for a +// `Seal.FinalState`. Without an explicit check for equality 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. // @@ -292,16 +292,21 @@ func (s *SealValidationSuite) TestSealDuplicatedApproval() { func (s *SealValidationSuite) TestSealInvalidFinalState() { _, _, newBlock, receipt, seal := s.generateBasicTestFork() - // Sanity: 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. + // 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) - // Mutate one byte of `FinalState` to model the Byzantine proposer who keeps all attestation-bound - // fields (BlockID, ResultID, ChunkIndex, AggregatedApprovalSigs) intact but tampers with the - // redundant `Seal.FinalState`. The seal pointer is already included in `newBlock`'s payload. - seal.FinalState[0] ^= 0xff + // 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) From d0f1bb03a9847bbb6706c33943a046e64455a06e Mon Sep 17 00:00:00 2001 From: Alex Hentschel Date: Tue, 2 Jun 2026 12:09:36 -0700 Subject: [PATCH 5/5] addressed review suggestions --- module/validation/seal_validator.go | 2 +- module/validation/seal_validator_test.go | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/module/validation/seal_validator.go b/module/validation/seal_validator.go index a75d434c5f3..229464afb13 100644 --- a/module/validation/seal_validator.go +++ b/module/validation/seal_validator.go @@ -272,7 +272,7 @@ func (s *sealValidator) validateSeal(seal *flow.Seal, incorporatedResult *flow.I 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 does not match execution result") + 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 diff --git a/module/validation/seal_validator_test.go b/module/validation/seal_validator_test.go index d0d7ddbae09..e396418aaa5 100644 --- a/module/validation/seal_validator_test.go +++ b/module/validation/seal_validator_test.go @@ -275,13 +275,13 @@ func (s *SealValidationSuite) TestSealDuplicatedApproval() { // 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 a supplied field whose authoritative value is already determined -// by the referenced `ExecutionResult` (via `executionResult.FinalStateCommitment()`). The verifier -// attestations only cover `{BlockID, ExecutionResultID, ChunkIndex}` — they do _not_ commit to -// `Seal.FinalState`. Without an explicit check for equality 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. +// 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: //