fix(consensus): harden FullProposal.Verify and NewProposal against malicious proposals by wen-coding · Pull Request #2908 · sei-protocol/sei-chain

wen-coding · 2026-02-17T16:55:43Z

Summary

Verify proposer signature in FullProposal.Verify (was only checking key identity, not the actual sig)
Verify LaneQC header hash matches the proposal's LaneRange.LastHash
Reject proposals containing non-committee lanes via new Proposal.Verify method
Add leader validation to NewProposal constructor (fail-fast on wrong key)
Use utils.OrPanic1 for compact error handling in tests
Comprehensive test coverage for all verification paths

Test plan

All TestProposalVerify* tests pass
Full autobahn/... test suite passes (avail, consensus, data, types)

…osal.Verify FullProposal.Verify only validated lane ranges for committee lanes, never checking for extra non-committee lanes. A malicious proposer could inject fake lanes to inflate GlobalRange, causing a panic in NewFullCommitQC when header count didn't match. Add a lane count check and comprehensive Verify test suite (20 tests covering all verification branches). Co-authored-by: Cursor <cursoragent@cursor.com>

github-actions · 2026-02-17T16:56:37Z

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
`✅ passed`	`✅ passed`	`✅ passed`	`✅ passed`	Feb 19, 2026, 11:53 PM

codecov · 2026-02-17T17:02:47Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.19%. Comparing base (bfad51f) to head (110b080).

❗ There is a different number of reports uploaded between BASE (bfad51f) and HEAD (110b080). Click for more details.

HEAD has 2 uploads less than BASE

Flag BASE (bfad51f) HEAD (110b080)

sei-db 1 0

sei-chain 1 0

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2908       +/-   ##
===========================================
- Coverage   57.19%   48.19%    -9.01%     
===========================================
  Files        2093      666     -1427     
  Lines      171732    50167   -121565     
===========================================
- Hits        98228    24179    -74049     
+ Misses      64738    23871    -40867     
+ Partials     8766     2117     -6649

Flag	Coverage Δ
sei-chain	`?`
sei-cosmos	`48.19% <ø> (ø)`
sei-db	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.
see 1549 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov · 2026-02-17T17:05:02Z

Codecov Report

❌ Patch coverage is 91.30435% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.74%. Comparing base (8cf29d8) to head (e8c5af4).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
sei-tendermint/internal/autobahn/types/proposal.go	86.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2908       +/-   ##
===========================================
- Coverage   68.42%   57.74%   -10.68%     
===========================================
  Files           5     2111     +2106     
  Lines         456   174226   +173770     
===========================================
+ Hits          312   100603   +100291     
- Misses        114    64652    +64538     
- Partials       30     8971     +8941

Flag	Coverage Δ
sei-chain	`57.71% <91.30%> (?)`
sei-db	`68.42% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
sei-tendermint/internal/autobahn/data/testonly.go	`62.26% <100.00%> (ø)`
sei-tendermint/internal/autobahn/types/proposal.go	`85.01% <86.66%> (ø)`

... and 2104 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

FullProposal.Verify checked that the LaneQC's block number matched the proposal's LaneRange but not the header hash. A malicious proposer could present a valid LaneQC for header H1 while setting LastHash to H2, causing a liveness attack: the committed proposal would reference a header hash no honest replica has votes for, permanently stalling FullCommitQC construction. Add a header hash check alongside the existing block number check. Also remove unused laneKey parameter from test helper makeLaneQC. Co-authored-by: Cursor <cursoragent@cursor.com>

FullProposal.Verify checked that the proposal's claimed signing key matched the expected leader but never cryptographically verified the signature itself. A man-in-the-middle could intercept a valid proposal, modify its content, and relay it with the original leader's key claim and stale signature, passing the key identity check. Add m.proposal.VerifySig(c) to verify the ed25519 signature against the proposal hash, and add a test for forged proposal signatures. Co-authored-by: Cursor <cursoragent@cursor.com>

…sal_test.go Test names are self-explanatory; section comments add noise. Also remove stale _ = rng assignment. Co-authored-by: Cursor <cursoragent@cursor.com>

The ExtraLane test already covers this case; the GlobalRange inflation variant adds no additional code path coverage. Co-authored-by: Cursor <cursoragent@cursor.com>

sei-tendermint/internal/autobahn/types/proposal.go

sei-tendermint/internal/autobahn/types/proposal_test.go

….Verify - Add Proposal.Verify(c) to check lane count and membership against the committee, fixing a bug where a non-committee lane substituted for a committee lane (same count) could pass verification at genesis due to Proposal.LaneRange() returning a synthetic [0,0) fallback. - Extract proposal local variable to avoid repeated .Msg() calls. - Replace TestVerifyRejectsExtraLane with two targeted tests: TestVerifyRejectsNonCommitteeLane (substitution) and TestVerifyRejectsMissingLaneRange (count mismatch). Co-authored-by: Cursor <cursoragent@cursor.com>

- Relax Proposal.Verify to allow implicit empty lane ranges for omitted lanes - Rename all test functions with TestProposal prefix for clarity - Use NewProposal as source of truth in tests, tamper fields for negative cases - Remove stringly-typed error assertions (require.Contains) in favor of require.Error - Add tests for implicit lane ranges (contiguous and non-contiguous) - Discard NewProposal error with _ since it cannot fail with valid test inputs Co-authored-by: Cursor <cursoragent@cursor.com>

sei-tendermint/internal/autobahn/types/proposal.go

sei-tendermint/internal/autobahn/types/proposal_test.go

pompon0 · 2026-02-19T09:42:41Z

sei-tendermint/internal/autobahn/types/proposal_test.go

+			break
+		}
+	}
+	tamperedFP := &FullProposal{


nit: I think that NewProposal does not verify whether leader is correct, so it can be called directly

Hmm, but maybe we should check?
Since NewProposal has everything to verify the leader, we can check at construction time, instead of sending the wrong proposal across the wires and getting it dropped.

Adding the check now and updating tests.

pompon0 · 2026-02-19T09:45:55Z

sei-tendermint/internal/autobahn/types/proposal_test.go

+			tamperedRanges = append(tamperedRanges, r)
+		}
+	}
+	tamperedProposal := newProposal(origP.view, origP.createdAt, tamperedRanges, origP.app)


NewProposal?

Same here, NewProposal would set first to a sane value, we need to tamper and sign the wrong value.

pompon0 · 2026-02-19T09:46:46Z

sei-tendermint/internal/autobahn/types/proposal_test.go

+		i++
+	}
+
+	shortProposal := newProposal(origP.view, origP.createdAt, keptRanges, origP.app)


NewProposal? Especially the happy path test (no tampering) shouldn't need to access private constructors.

Can't use NewProposal here, because NewProposal's construction logic always produces all lanes and signs over the content. If we use NewProposal and replaces signature, it would fail at "bad signature" in Verify.

pompon0 · 2026-02-19T09:49:19Z

sei-tendermint/internal/autobahn/types/proposal_test.go

+	for _, r := range origP.laneRanges {
+		ranges = append(ranges, r)
+	}
+	tamperedProposal := newProposal(origP.view, origP.createdAt, ranges, utils.None[*AppProposal]())


NewProposal?

We force app to be None here, NewProposal will reset app to be old appQC I believe.

pompon0

lgtm, left some comments for tests.

…1 in tests - NewProposal now validates that the key belongs to the view leader - Replace `fp, _ := NewProposal(...)` with `utils.OrPanic1(...)` in tests - Simplify TestProposalVerifyRejectsForgedSignature - Fix all callers to use correct leader key (data/testonly, avail tests) Co-authored-by: Cursor <cursoragent@cursor.com>

wen-coding added the non-app-hash-breaking label Feb 17, 2026

wen-coding changed the title ~~fix(consensus): reject proposals with non-committee lanes in FullProposal.Verify~~ fix(consensus): reject malicious proposals in FullProposal.Verify Feb 17, 2026

wen-coding changed the title ~~fix(consensus): reject malicious proposals in FullProposal.Verify~~ fix(consensus): harden FullProposal.Verify against malicious proposals Feb 17, 2026

wen-coding and others added 3 commits February 17, 2026 11:16

cleanup: remove section divider comments and unused variable in propo…

e710270

…sal_test.go Test names are self-explanatory; section comments add noise. Also remove stale _ = rng assignment. Co-authored-by: Cursor <cursoragent@cursor.com>

cleanup: remove redundant ExtraLaneInflatesGlobalRange test

31f4389

The ExtraLane test already covers this case; the GlobalRange inflation variant adds no additional code path coverage. Co-authored-by: Cursor <cursoragent@cursor.com>

Merge branch 'main' into wen/fix_fullproposal_verify

ff3d07e

wen-coding requested review from pompon0 and stevenlanders February 17, 2026 22:10