fix(network): add vote payload validation in Attest handler#360
Conversation
The Attest handler previously accepted any vote payload, including empty or garbage data. This allowed attesters to submit meaningless votes that would be counted toward quorum. This commit adds vote payload validation: - Rejects nil/empty vote payloads - Enforces minimum 48-byte length (BLS signature size) - Documents that full cryptographic verification should be added once the vote format is finalized Adds 5 table-driven test cases for vote payload validation.
📝 WalkthroughWalkthroughAdds exported Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @alpe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the security and integrity of the attestation process by introducing crucial validation for vote payloads. It prevents the submission of meaningless or malformed votes that could undermine quorum calculations, laying the groundwork for future cryptographic signature verification. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
modules/network/keeper/msg_server.go (1)
69-80: Centralize the minimum vote length constant to avoid drift.Consider moving the
minVoteLenvalue to a package-level constant (ortypes), and reuse it in both the handler and tests so future format changes can’t diverge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/network/keeper/msg_server.go` around lines 69 - 80, The local minVoteLen constant in msg_server.go should be moved to a shared package-level constant (e.g., export it from the keeper package or place it in types as VoteMinLen or MinVoteLen) and then replace the local declaration in the Vote handler (the code block using minVoteLen and msg.Vote) with that shared constant; update any tests that assert/construct votes to import and use the new symbol so both handler and tests reference the single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@modules/network/keeper/msg_server.go`:
- Around line 69-80: The local minVoteLen constant in msg_server.go should be
moved to a shared package-level constant (e.g., export it from the keeper
package or place it in types as VoteMinLen or MinVoteLen) and then replace the
local declaration in the Vote handler (the code block using minVoteLen and
msg.Vote) with that shared constant; update any tests that assert/construct
votes to import and use the new symbol so both handler and tests reference the
single source of truth.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces security validation for the vote payload in the Attest handler to prevent empty or malformed votes. However, it has critical security vulnerabilities: it lacks an authorization check for the message signer, enabling validator impersonation, and omits height validation, which could lead to state bloat via future-dated attestations. A broader architectural concern is that JoinAttesterSet allows unrestricted joining, potentially facilitating Sybil attacks. The current validation logic could also be simplified for conciseness.
- MinVoteLen is now a package-level constant (48 bytes) shared between handler and tests to avoid drift - Removed redundant len(msg.Vote) == 0 check since len < 48 covers it - Check SetParams error return (errcheck)
alpe
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
modules/network/keeper/msg_server.go (1)
73-80: Vote payload validation fires after two potentially expensive reads; consider moving it earlier.The check at line 78 is placed after
GetAttestationBitmap(line 56) andGetAllAttesters(line 61), which involve KV-store reads and can allocate a new bitmap. An invalid payload will cause all of that work to be discarded. Moving the guard immediately after the attester-set checks (after line 47) lets the handler fail fast without touching the bitmap at all.♻️ Suggested reordering
if !has { return nil, sdkerr.Wrapf(sdkerrors.ErrUnauthorized, "consensus address %s not in attester set", msg.ConsensusAddress) } + // Validate vote payload meets minimum signature length. + // A valid vote must contain at least a cryptographic signature (48 bytes for BLS, + // 64 bytes for Ed25519). We enforce the minimum here; full cryptographic + // verification of the signature against the block data should be added once + // the vote format is finalized. + if len(msg.Vote) < MinVoteLen { + return nil, sdkerr.Wrapf(sdkerrors.ErrInvalidRequest, "vote payload too short: got %d bytes, minimum %d", len(msg.Vote), MinVoteLen) + } + index, found := k.GetValidatorIndex(ctx, msg.ConsensusAddress) ... bitmap, err := k.GetAttestationBitmap(ctx, msg.Height) ... - // Validate vote payload meets minimum signature length. - ... - if len(msg.Vote) < MinVoteLen { - return nil, sdkerr.Wrapf(sdkerrors.ErrInvalidRequest, "vote payload too short: got %d bytes, minimum %d", len(msg.Vote), MinVoteLen) - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/network/keeper/msg_server.go` around lines 73 - 80, The vote payload length check (len(msg.Vote) < MinVoteLen) should be moved earlier to fail fast before any KV reads/allocations; specifically, after the attester-set validation block and immediately after those checks (i.e., right after the attester-set checks that precede the bitmap code), validate msg.Vote against MinVoteLen and return the error if too short, so that GetAttestationBitmap and GetAllAttesters are not invoked for invalid payloads.modules/network/keeper/msg_server_test.go (1)
154-189: Per-iteration keeper setup is duplicated; consider a shared helper.The full store/keeper/context initialization block (lines 156–172) is identical for every sub-test. Extracting it into a
t.Helper()function would shorten the test body and make future changes (e.g., adding new default params) apply in one place.♻️ Suggested helper extraction
+func newAttestTestServer(t *testing.T) (msgServer, sdk.Context, *Keeper) { + t.Helper() + sk := NewMockStakingKeeper() + cdc := moduletestutil.MakeTestEncodingConfig().Codec + keys := storetypes.NewKVStoreKeys(types.StoreKey) + logger := log.NewTestLogger(t) + cms := integration.CreateMultiStore(keys, logger) + authority := authtypes.NewModuleAddress("gov") + keeper := NewKeeper(cdc, runtime.NewKVStoreService(keys[types.StoreKey]), sk, nil, nil, authority.String()) + server := msgServer{Keeper: keeper} + ctx := sdk.NewContext(cms, cmtproto.Header{ + ChainID: "test-chain", + Time: time.Now().UTC(), + Height: 10, + }, false, logger).WithContext(t.Context()) + return server, ctx, &keeper +} for name, spec := range specs { t.Run(name, func(t *testing.T) { - sk := NewMockStakingKeeper() - cdc := moduletestutil.MakeTestEncodingConfig().Codec - keys := storetypes.NewKVStoreKeys(types.StoreKey) - logger := log.NewTestLogger(t) - cms := integration.CreateMultiStore(keys, logger) - authority := authtypes.NewModuleAddress("gov") - keeper := NewKeeper(cdc, runtime.NewKVStoreService(keys[types.StoreKey]), sk, nil, nil, authority.String()) - server := msgServer{Keeper: keeper} - ctx := sdk.NewContext(cms, cmtproto.Header{ - ChainID: "test-chain", - Time: time.Now().UTC(), - Height: 10, - }, false, logger).WithContext(t.Context()) + server, ctx, keeper := newAttestTestServer(t) require.NoError(t, keeper.SetParams(ctx, types.DefaultParams()))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/network/keeper/msg_server_test.go` around lines 154 - 189, The per-subtest setup (creating NewMockStakingKeeper, codec via moduletestutil.MakeTestEncodingConfig, keys/store/logger, CreateMultiStore, authority, NewKeeper, msgServer, ctx creation and the setup calls keeper.SetParams, keeper.SetAttesterSetMember, keeper.BuildValidatorIndexMap) is duplicated; extract this into a t.Helper function (e.g., makeTestCtx or setupKeeper) that returns the keeper, server/msgServer, ctx and any common values (like myValAddr) and call that from each t.Run; mark the helper with t.Helper() so failures report the caller and update tests to use the returned values before constructing MsgAttest and calling Attest.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@modules/network/keeper/msg_server_test.go`:
- Around line 154-189: The per-subtest setup (creating NewMockStakingKeeper,
codec via moduletestutil.MakeTestEncodingConfig, keys/store/logger,
CreateMultiStore, authority, NewKeeper, msgServer, ctx creation and the setup
calls keeper.SetParams, keeper.SetAttesterSetMember,
keeper.BuildValidatorIndexMap) is duplicated; extract this into a t.Helper
function (e.g., makeTestCtx or setupKeeper) that returns the keeper,
server/msgServer, ctx and any common values (like myValAddr) and call that from
each t.Run; mark the helper with t.Helper() so failures report the caller and
update tests to use the returned values before constructing MsgAttest and
calling Attest.
In `@modules/network/keeper/msg_server.go`:
- Around line 73-80: The vote payload length check (len(msg.Vote) < MinVoteLen)
should be moved earlier to fail fast before any KV reads/allocations;
specifically, after the attester-set validation block and immediately after
those checks (i.e., right after the attester-set checks that precede the bitmap
code), validate msg.Vote against MinVoteLen and return the error if too short,
so that GetAttestationBitmap and GetAllAttesters are not invoked for invalid
payloads.
* main: fix(network): cap attester set size to prevent DoS and uint16 overflow (#361)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modules/network/keeper/msg_server_test.go (1)
162-224: Add state‑mutation assertions for rejected votes.Nice coverage for length validation. To lock in the “no side effects on error” behavior, consider asserting that the attestation bitmap (and signature store if a getter exists) remains unset when
expErris expected. This will prevent regressions where the validation is moved after state writes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/network/keeper/msg_server_test.go` around lines 162 - 224, Add assertions after the rejected-case branch in TestAttestVotePayloadValidation to ensure no state was mutated when server.Attest returns an error: after the require.ErrorIs/require.Nil checks for spec.expErr, call the keeper getters that expose attestation state (e.g. GetAttestationBitmap(ctx, msg.Height, msg.ConsensusAddress) and, if present, GetSignature/LookupSignature/AttestationSignatureStore getters) and assert they are empty/nil/unset; place these checks right after the error assertions so any attempt by Attest to write the attestation bitmap or signature will fail the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/network/keeper/msg_server.go`:
- Around line 73-80: The vote-length check currently uses a single MinVoteLen
for all signatures; instead, use the retrieved SignMode value to enforce
per-mode sizes: require len(msg.Vote) == 48 when SignMode == SignMode_BLS (or
the local enum/const name used) and len(msg.Vote) == 64 when SignMode ==
SignMode_Ed25519, returning a wrapped sdkerrors.ErrInvalidRequest (like the
existing check) that states the actual and expected byte lengths; update the
validation around the existing if that reads msg.Vote and references MinVoteLen
to branch on SignMode and validate accordingly.
---
Nitpick comments:
In `@modules/network/keeper/msg_server_test.go`:
- Around line 162-224: Add assertions after the rejected-case branch in
TestAttestVotePayloadValidation to ensure no state was mutated when
server.Attest returns an error: after the require.ErrorIs/require.Nil checks for
spec.expErr, call the keeper getters that expose attestation state (e.g.
GetAttestationBitmap(ctx, msg.Height, msg.ConsensusAddress) and, if present,
GetSignature/LookupSignature/AttestationSignatureStore getters) and assert they
are empty/nil/unset; place these checks right after the error assertions so any
attempt by Attest to write the attestation bitmap or signature will fail the
test.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modules/network/keeper/msg_server.go (1)
73-80: Consider moving vote-length validation before the storage reads for fail-fast behaviour.The vote payload check at line 78 is a pure in-memory operation, but it currently executes after
IsInAttesterSet(line 41),GetValidatorIndex(line 49),GetAttestationBitmap(line 56), andIsSet(line 69) — all of which hit storage. Hoisting it immediately after the SignMode check (line 40) eliminates those I/O calls for any malformed request.♻️ Proposed refactor — move validation earlier
if k.GetParams(ctx).SignMode == types.SignMode_SIGN_MODE_CHECKPOINT && !k.IsCheckpointHeight(ctx, msg.Height) { return nil, sdkerr.Wrapf(sdkerrors.ErrInvalidRequest, "height %d is not a checkpoint", msg.Height) } + +// Validate vote payload before any storage reads. +if len(msg.Vote) < MinVoteLen { + return nil, sdkerr.Wrapf(sdkerrors.ErrInvalidRequest, "vote payload too short: got %d bytes, minimum %d", len(msg.Vote), MinVoteLen) +} + has, err := k.IsInAttesterSet(ctx, msg.ConsensusAddress) ... - - // Validate vote payload meets minimum signature length. - // A valid vote must contain at least a cryptographic signature ( - // 64 bytes for Ed25519). We enforce the minimum here; full cryptographic - // verification of the signature against the block data should be added once - // the vote format is finalized. - if len(msg.Vote) < MinVoteLen { - return nil, sdkerr.Wrapf(sdkerrors.ErrInvalidRequest, "vote payload too short: got %d bytes, minimum %d", len(msg.Vote), MinVoteLen) - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/network/keeper/msg_server.go` around lines 73 - 80, The vote-length check (comparing len(msg.Vote) to MinVoteLen) is a pure in-memory validation and should be executed immediately after the SignMode check to fail fast and avoid unnecessary storage reads; move the existing if len(msg.Vote) < MinVoteLen { ... } block so it runs before calls that hit storage (IsInAttesterSet, GetValidatorIndex, GetAttestationBitmap, IsSet) while keeping the same error wrapper and message, referencing the same MinVoteLen and msg.Vote symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/network/keeper/msg_server.go`:
- Around line 73-80: The handler currently only checks a minimum length for
msg.Vote and then persists it via SetSignature, allowing arbitrarily large
payloads to be written on-chain; add an explicit maximum size guard (e.g.,
define a MaxVoteLen constant in the same package) and in the msg handling code
check if len(msg.Vote) > MaxVoteLen and return an sdkerrors.ErrInvalidRequest
with a clear message if so; ensure the check sits alongside the existing
MinVoteLen check (before calling SetSignature) so oversized votes are rejected
before state persistence.
---
Nitpick comments:
In `@modules/network/keeper/msg_server.go`:
- Around line 73-80: The vote-length check (comparing len(msg.Vote) to
MinVoteLen) is a pure in-memory validation and should be executed immediately
after the SignMode check to fail fast and avoid unnecessary storage reads; move
the existing if len(msg.Vote) < MinVoteLen { ... } block so it runs before calls
that hit storage (IsInAttesterSet, GetValidatorIndex, GetAttestationBitmap,
IsSet) while keeping the same error wrapper and message, referencing the same
MinVoteLen and msg.Vote symbols.
* main: fix(network): add vote payload validation in Attest handler (#360)
3 participants
Summary
The
Attesthandler previously accepted any vote payload including empty/nil/garbage data. This allowed attesters to submit meaningless votes that counted toward quorum, undermining soft confirmation integrity.Changes
msg_server.go: Added vote payload validation before storing the signature:msg_server_test.go: 5 table-driven tests for vote payload validationSecurity Impact
Prevents garbage/empty votes from counting toward quorum. When combined with future BLS signature verification, this ensures only cryptographically valid attestations are accepted.
Testing
Summary by CodeRabbit
Bug Fixes
Tests