Require matching ModeInfo and nested signature counts (CON-393)

[wen-coding]

Summary

• Enforce len(ModeInfos) == len(signatures) in ModeInfoAndSigToSignatureData.
• Enforce len(SignerInfos) == len(Signatures) in Tx.ValidateBasic (covers ante paths that index signers by pubkey/SignerInfos count before signature decode).
• Unit coverage for both mismatches.

Test plan

• GOWORK=off go test -run 'TestModeInfo|TestDecode|TestBuilderValidate' ./sei-cosmos/x/auth/tx/
• CI

[github-actions]

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

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	✅ passed	✅ passed	✅ passed	Aug 6, 2026, 12:08 AM

[seidroid]

Small, correct hardening fix: ModeInfoAndSigToSignatureData now rejects a multi ModeInfo whose nested ModeInfos count doesn't match the decoded signature count, replacing an index-out-of-range panic with a clean error, plus a regression test. No blockers; a few coverage/polish notes and one unguarded sibling index in the same call path.

Findings: 0 blocking | 6 non-blocking | 2 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Same bug class is still unguarded one frame up: wrapper.GetSignaturesV2 (sei-cosmos/x/auth/tx/builder.go:182) indexes sigs[i] while ranging over AuthInfo.SignerInfos, and nothing in the decoder (decoder.go just copies raw.Signatures) validates len(SignerInfos) == len(Signatures). The count check in ante/sigverify.go:257 runs after GetSignaturesV2, so a tx with more SignerInfos than Signatures still panics (recovered by baseapp, but a panic per malformed tx). Worth folding into this PR or the same ticket, since it's exactly the invariant this change is about.
• Cursor's second-opinion pass (cursor-review.md) is empty — no output was produced for this PR. Codex reported "No material findings."
• I could not execute the test in this sandbox (go test was not permitted), so the test-plan checkbox is unverified from my side; the APIs used (multisig.NewMultisig, multisig.AddSignature, cryptotypes.NewCompactBitArray) all exist and the round-trip equality should hold, but CI is the confirmation.
• Behavior note for the non-app-hash-breaking label: for len(sigs) < len(ModeInfos) this converts a recovered panic into a returned error, which changes the ABCI result (code/gas) for such txs. Previously-valid txs are unaffected (the len(sigs) > len(ModeInfos) case already produced nil entries rejected by ValidateSignatureDataStructure), so the label looks right — just flagging that the reasoning rests on the panic path, not on the code path being unreachable.
• 2 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[nit] Two small things on this error:

1. Wording: "invalid nested multisig" fires for any ModeInfo_Multi_, including a top-level (non-nested) one. Something like "invalid multisig: %d mode infos, %d signatures" reads more accurately.
2. This is a bare fmt.Errorf, so once it propagates out of GetSignaturesV2 into the ante handler, sdkerrors.ABCIInfo maps it to code 1 and redacts the message to "internal error" outside debug mode — the useful counts are lost to the submitter. Wrapping in an sdk error (e.g. sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, ...)) would surface it. Non-blocking: decodeMultisignatures right below already returns a bare error, so this is consistent with the file as-is.

[seidroid]

[suggestion] Two coverage gaps worth closing while you're here:

• Only the len(sigs) < len(ModeInfos) direction is tested. The opposite (more nested signatures than ModeInfos) is the case that previously produced a sigv2s slice with trailing nil entries rather than a panic; a case for it pins the new check in both directions.
• Despite the comment, neither case exercises a genuinely nested multisig (a ModeInfo_Multi_ inside ModeInfos), which is where the recursive call re-enters this branch. A two-level fixture would cover the recursion path.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.72%. Comparing base (a0ad413) to head (66af35d).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3863      +/-   ##
==========================================
- Coverage   61.58%   60.72%   -0.86%     
==========================================
  Files        2369     2275      -94     
  Lines      199837   189368   -10469     
==========================================
- Hits       123075   115000    -8075     
+ Misses      65812    64256    -1556     
+ Partials    10950    10112     -838     

Flag	Coverage Δ
sei-chain-pr	61.41% <100.00%> (?)
sei-db	70.41% <ø> (ø)
sei-db-state-db	?

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

Files with missing lines	Coverage Δ
sei-cosmos/types/tx/types.go	72.72% <100.00%> (+1.29%)	⬆️
sei-cosmos/x/auth/tx/sigs.go	57.14% <100.00%> (+0.96%)	⬆️

... and 96 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.

[claude]

No bugs found, but this PR modifies core transaction signature-verification code (ModeInfoAndSigToSignatureData, used in every tx's GetSignaturesV2), so I'm deferring for a human look rather than shadow-approving.

What was reviewed:

• Confirmed the new length check prevents an out-of-bounds panic/read when a nested multisig has fewer raw signatures than ModeInfos entries, and closes a case where extra raw signatures beyond len(ModeInfos) were silently dropped, leaving trailing nil entries in the decoded MultiSignatureData.
• Checked the only caller (builder.go: GetSignaturesV2) properly propagates the new error.
• Verified the added test exercises both the round-trip success case and the mismatched-count rejection case.

Extended reasoning...

Overview

The PR adds a length check in ModeInfoAndSigToSignatureData (sei-cosmos/x/auth/tx/sigs.go) so that a nested multisig's ModeInfos count must match the decoded raw signature count, returning an error on mismatch instead of proceeding. Previously, if len(sigs) < len(ModeInfos), the loop indexing sigs[i] could panic (out-of-bounds); if len(sigs) > len(ModeInfos), the resulting MultiSignatureData.Signatures slice (sized to len(sigs)) would have trailing nil entries corresponding to raw signatures that were silently dropped rather than validated. The change is minimal and paired with a new unit test covering both a successful round-trip and the new error path.

Security risks

This is core signature-decoding logic used on every transaction's ante-handler signature verification path (sigs.go -> builder.go GetSignaturesV2). The change itself hardens the code (rejecting malformed/malleable nested multisig encodings), so I don't see a new vulnerability introduced. The main risk category here is consensus-correctness: any change to what counts as a valid/invalid transaction in signature parsing is sensitive, though a mismatched mode-info/signature count almost certainly could never correspond to a legitimately-signed transaction, so this looks like tightening rather than a behavior change for honest signers.

Level of scrutiny

Even though the diff is small, self-contained, and includes a targeted test, it sits in cryptographic signature-verification code that runs on every transaction and affects consensus-critical validity determination. Per repo review guidance, changes touching auth/crypto code should get human eyes rather than being shadow-approved, regardless of how small the diff is.

Other factors

No outstanding review comments in the PR timeline, and the bug-hunting system found no issues. The fix looks correct and well-tested; my deferral is about the sensitivity of the code area rather than a stated concern about this specific change.

[seidroid]

Correct, well-scoped fix: enforcing len(ModeInfos) == len(sigs) closes a reachable index-out-of-range panic in multisig decoding (and silent nil-padding in the inverse case) at a point that runs before ValidateSignatureDataStructure, with tests covering both mismatch directions and the recursive nested case. Only nits and one adjacent pre-existing gap in GetSignaturesV2; nothing blocking.

Findings: 0 blocking | 6 non-blocking | 2 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Adjacent, pre-existing gap of the same class (out of this diff's scope): wrapper.GetSignaturesV2 (sei-cosmos/x/auth/tx/builder.go:182) indexes sigs[i] over signerInfos with no len(tx.Signatures) == len(AuthInfo.SignerInfos) check, and DefaultTxDecoder doesn't validate that either. A tx with fewer signatures than signer_infos still panics there. Worth confirming CON-393 covers it or filing a follow-up.
• Behavior-change sanity check for the non-app-hash-breaking label: both newly-rejected shapes were already failing before (the short-sig case via recovered panic, the long-sig case via ValidateSignatureDataStructure's case nil: signature data is required), so the accepted-tx set is unchanged — only the resulting error/ABCI code differs. Reasonable, but flagging since tx result codes feed LastResultsHash.
• The Cursor review file (cursor-review.md) is empty — that pass produced no output. Codex reported no material findings.
• Tests could not be executed in this environment (Go 1.25.6 toolchain download is network-blocked); verification is static plus CI.
• 2 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[nit] Consider wrapping with a registered SDK error, e.g. sdkerrors.Wrapf(sdkerrors.ErrTxDecode, "invalid multisig: %d mode infos, %d signatures", ...). This error propagates unmodified through GetSignaturesV2 into the ante decorators, and a bare fmt.Errorf lands in ABCIInfo as codespace undefined/code 1 rather than a stable tx-decode code — decoder.go wraps its rejections this way. (Neighboring decodeMultisignatures also returns a bare error, so this is consistency-with-decoder vs. consistency-with-file; low priority either way.)

[seidroid]

[nit] mustMarshal and mustErr call require.* inside closures — add t.Helper() as the first statement in each so a failure reports the calling line rather than the closure body. With four mustErr call sites the reported line matters for diagnosing which case broke.

[claude]

I didn't find any bugs, and this looks like a correct, well-tested fix — the earlier nits (error wording, extra test coverage for the reverse-mismatch and nested-multisig cases) have already been addressed. Since this is in the auth/tx signature-decoding path, I'm deferring for a human look rather than approving outright.

Checked: the invariant this relies on (ModeInfos and nested signatures are always equal-length) holds by construction in the corresponding encode path (SignatureDataToModeInfoAndSig), and a similar count invariant is already enforced independently via ValidateSignatureDataStructure elsewhere in the crypto/multisig package, so this check is consistent with existing conventions.

Extended reasoning...

Overview

This PR adds a length check in ModeInfoAndSigToSignatureData (sei-cosmos/x/auth/tx/sigs.go) to reject multisig ModeInfo/signature slices whose lengths don't match, returning an error instead of indexing out of bounds. It also adds unit tests covering the round-trip success case, both mismatch directions, and a nested multisig mismatch.

Security risks

This is signature-decoding code in the tx auth path, so it's security-relevant by nature. The change itself reduces risk: previously, a malformed/crafted tx with a nested multisig signature whose sub-signature count didn't match its ModeInfo count could trigger an out-of-bounds slice index inside ModeInfoAndSigToSignatureData. The fix converts that into a clean, returned error. I confirmed the invariant the check assumes (ModeInfos and signatures are always same-length for validly-encoded data) holds by construction in SignatureDataToModeInfoAndSig, so no regression is expected for well-formed transactions.

Level of scrutiny

The diff itself is small, mechanical, and narrowly scoped, with solid test coverage added (including the previously-untested reverse-mismatch and nested-multisig-recursion cases raised by an earlier bot review, which are now covered). However, because it sits directly in transaction signature verification/decoding — a security- and consensus-relevant code path — I think it warrants a human's eyes rather than an automatic approval, even though I found nothing wrong with it.

Other factors

All prior automated review comments (seidroid bot nits on error wording and test coverage gaps) appear to have already been addressed in the current diff. CI (Buf, Codecov) is passing and coverage on the changed lines is complete.

[cursor]

PR Summary

Medium Risk
Changes which transactions pass basic validation and multisig parsing; well-formed txs are unaffected, but any reliance on lenient mismatched wire data would break.

Overview
Tightens transaction shape checks so malformed auth metadata is rejected before signature decoding and ante handling.

Tx.ValidateBasic now requires len(SignerInfos) == len(Signatures), matching the 1:1 relationship established by SetSignatures. Mismatches return ErrUnauthorized.

Multisig decode in ModeInfoAndSigToSignatureData now errors when nested ModeInfos and decoded signature slices differ in length, instead of indexing past the shorter slice.

Unit tests cover both failure modes in builder_test and sigs_test.

^{Reviewed by Cursor Bugbot for commit 66af35d. Bugbot is set up for automated code reviews on this repo. Configure here.}

[seidroid]

Sound defense-in-depth hardening: the new count checks reject earlier in the ante chain without changing which txs are accepted (I verified no previously-valid tx becomes invalid, including in simulate mode). No blockers, but the legacy amino multisig path is only half-hardened — extra trailing signatures are still silently dropped — and a pre-existing indexing bug in that same function is worth fixing while it's being touched.

Findings: 0 blocking | 8 non-blocking | 3 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Pre-existing bug adjacent to the change (sei-cosmos/x/auth/legacy/legacytx/stdsign.go:179): multisig.AddSignature(signatures, data, sigIdx) passes the compacted signature index, but AddSignature's third parameter is the pubkey index (see sei-cosmos/crypto/types/multisig/multisignature.go:85, which does BitArray.SetIndex(index, true)). For a bitarray whose set bits aren't a prefix (e.g. {false, true}), the reconstructed MultiSignatureData gets bitarray {true, false}, so verification checks the signature against the wrong pubkey and fails. It should be i. Related: sigDatas is now written but never read — upstream cosmos-sdk returns &signing.MultiSignatureData{BitArray: bitArray, Signatures: sigDatas}, which both fixes the index bug and makes the variable live. Not introduced here, but this PR is hardening exactly this function.
• Test coverage gaps: none of the three new pubKeySigToSigData guards (nil bit array, bit-array/pubkey count mismatch, insufficient signatures) have tests, despite the test plan listing the legacytx package. Likewise, TestModeInfoAndSigToSignatureData's mustErr helper asserts "invalid multisig", so the new nil branches (nil modeInfo, nil Sum, typed-nil Single/Multi) are all uncovered. The five new ante-decorator guards have no tests either.
• The error string "invalid number of signers; expected: %d, got: %d" is now duplicated across five sites and is slightly inaccurate at each: it reports a pubkey count or signature count, not a signer count. Consider "invalid number of public keys" / "invalid number of signatures", or a small shared helper.
• Worth confirming the non-app-hash-breaking label. No previously-valid tx becomes invalid (verified above), but txs that previously panicked mid-loop in SetPubKeyDecorator now short-circuit before that loop, so their reported GasUsed differs. That feeds Tendermint's LastResultsHash rather than the app hash, and such txs already fail CheckTx, so this is likely fine — just flagging that the behavior for already-failing txs is not byte-identical.
• The Cursor second-opinion pass produced no output (cursor-review.md is empty), so this review reflects only Codex's findings plus my own. No prompt-injection attempts were found in the diff or PR description.
• 3 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[suggestion] This closes the "too few signatures" direction but not the inverse: after the loop there's no sigIdx == len(multiSig.Sigs) check, so an amino multisig carrying extra signatures past the set bits decodes successfully with the surplus silently dropped. That's the same count-mismatch malleability the rest of the PR rejects — two distinct byte encodings both decode to the same valid MultiSignatureData (different tx hashes). Blast radius is limited (the chain decoder is the proto one, so StdTx isn't reachable from the network — this is client/legacy-REST only), which is why I'm marking it a suggestion rather than a blocker, but it's cheap to add:

if sigIdx != len(multiSig.Sigs) {
    return nil, fmt.Errorf("invalid multisig: %d signatures for %d set bits", len(multiSig.Sigs), sigIdx)
}

Worth a test alongside the other new guards in this function.

(Also raised by Codex.)

[seidroid]

[nit] Every other negative case in this test asserts the ABCI code (sdkerrors.ABCIInfo + require.Equal). Doing the same here would pin that the failure is the new SignerInfos/Signatures check rather than some unrelated validation regressing into a false pass:

_, code, _ = sdkerrors.ABCIInfo(err, false)
require.Equal(t, sdkerrors.ErrUnauthorized.ABCICode(), code)

[seidroid]

[nit] Consider hoisting this check above the validatePubKey loop at line 224. This is the CheckTx hot path, and validatePubKey walks nested multisig trees; rejecting a structurally malformed tx on a length compare first avoids doing that recursive work for a tx that's going to be rejected anyway. signers := sigTx.GetSigners() would need to move up with it.

[claude]

No bugs found, but this PR modifies consensus-critical signature-verification and ante-handling code (tx decoding, ModeInfo/multisig parsing, CheckTx signer checks), so I'm deferring for a human look rather than shadow-approving.

What was reviewed:

• New length-parity guards across ModeInfo/signature/SignerInfo paths (ante decorators, tx builder GetSignaturesV2, Tx.ValidateBasic, legacy amino multisig decoding) that prevent index-out-of-range panics on malformed/adversarial tx encodings.
• Confirmed each new guard sits before the indexing loop it protects, and that the length invariants it assumes (e.g. GetPubKeys()/GetSignaturesV2() sizing) hold given the surrounding checks.
• Looked at the amino multisig pubKeySigToSigData excess-signature-ignored behavior flagged by the bug hunter — pre-existing behavior, not introduced or worsened by this PR.
• New unit tests cover both directions of the ModeInfo/signature and SignerInfo/signature mismatches.

Extended reasoning...

Overview

This PR adds defensive length-parity checks across the Cosmos SDK signature-verification stack: ModeInfoAndSigToSignatureData (nested multisig ModeInfo/signature counts), Tx.ValidateBasic and wrapper.GetSignaturesV2 (SignerInfos vs Signatures), several ante decorators and CheckTx helpers (SetPubKeyDecorator, SigGasConsumeDecorator, CheckPubKeys, CosmosStatelessChecks) that index parallel slices of pubkeys/signers/signatures, and the legacy amino pubKeySigToSigData multisig decoder (bitarray/pubkey/signature bounds). The stated goal is to turn out-of-bounds panics on malformed or adversarial tx encodings into proper errors.

Security risks

This is squarely security-sensitive code: it is on the transaction signature-verification and ante-handler path that every transaction (including untrusted, attacker-submitted ones) goes through in CheckTx/DeliverTx. A panic-to-error conversion here is a good defensive hardening (a crafted tx with mismatched ModeInfo/signature counts could previously panic on a slice index), but bugs in this exact area (wrong length comparison, wrong operand order, a guard that's too strict and rejects valid txs, or too loose and still allows a panic) would be consensus-affecting or could cause a chain-wide DoS/halt if a malformed tx makes it into a block before the fix, or availability regressions if valid txs are newly rejected.

Level of scrutiny

Given the change sits in the core signing/ante path (auth-sensitive, consensus-critical), I'd hold this to a high scrutiny bar regardless of whether the automated bug hunt found issues — the guidelines call out security-sensitive auth/crypto code as something that should get human eyes rather than a shadow-approval.

Other factors

Bug hunting found no confirmed issues this run, and one candidate (amino multisig silently ignoring excess trailing signatures) was investigated and refuted — that behavior predates this PR and is not changed by it. The PR carries new unit tests for both directions of the ModeInfo/signature-count and SignerInfo/signature-count mismatches, buf/codecov checks are green, and there's minor unaddressed non-blocking review feedback (error wrapping type, missing t.Helper()) that doesn't affect correctness. None of that changes the fact that this is exactly the kind of auth/consensus-path change that benefits from a human reviewer's sign-off.

[seidroid]

The two count invariants are correct and valid transactions can't be falsely rejected, but the SignerInfos == Signatures check is placed in GetSignaturesV2, which runs after three call sites that already index signers[i] by SignerInfo index — so the index-out-of-range panic this PR targets is still reachable on both the CheckTx and DeliverTx paths.

Findings: 1 blocking | 6 non-blocking | 3 posted inline

Blockers

• None at the file/PR level.
• 1 blocking issue(s) flagged inline on specific lines.

Non-blocking

• Cursor's second-opinion pass produced no output (cursor-review.md is empty), so this review merges only Claude's and Codex's findings.
• No ante-level regression test. Both new tests exercise the builder/sigs helpers directly; a test driving a crafted tx (1 msg, 1 signer, 1 raw signature, 2 SignerInfos) through CosmosStatelessChecks or the full ante chain is what would have caught the gap in the inline blocker, and is what will keep it closed.
• Test assertions use require.Contains(err.Error(), "invalid multisig"/"invalid tx"). Once the errors are wrapped in sdkerrors (see inline comments), prefer asserting on the sentinel with errors.Is / sdkerrors.IsOf so the tests don't break on message rewording and actually pin the ABCI code.
• Re Codex finding #2: I confirmed the default: panic is reachable, but not the other cases it lists. Nil *tx.ModeInfo elements inside multi.ModeInfos, and nil Single/Multi inside the oneof wrapper, are not reachable from cdc.Unmarshal — gogoproto always allocates repeated message elements and oneof inner messages. An unset Sum oneof is the real reachable case.
• 2 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[blocker] This invariant is enforced too late to close the hole it targets — three call sites iterate GetPubKeys() (whose length is len(AuthInfo.SignerInfos)) and index signers[i] (whose length is len(GetSigners())) before GetSignaturesV2 is ever called:

• app/ante/cosmos_checktx.go:241 — CosmosStatelessChecks, the earliest one
• app/ante/cosmos_checktx.go:458 — CheckPubKeys, called at line 110, before CheckSignatures at line 115
• sei-cosmos/x/auth/ante/sigverify.go:79/:84 — SetPubKeyDecorator, wired at app/ante.go:92, before its own GetSignaturesV2() call at line 103

Tx.ValidateBasic (sei-cosmos/types/tx/types.go:94) only checks len(Signatures) == len(GetSigners()); len(SignerInfos) is unconstrained. Concrete repro: a tx with one message, one signer, one raw Signature, and two SignerInfos passes ValidateBasic, then panics with index-out-of-range at signers[1] in CosmosStatelessChecks / SetPubKeyDecorator — the new check never runs.

Net effect: only the len(SignerInfos) < len(Signatures) direction actually reaches this check; the > direction (the one that panics today) does not. baseapp.runTx recovers the panic so this is a tx failure rather than a halt, but it's still the exact defect CON-393 is about.

Suggest moving the check to Tx.ValidateBasic, immediately alongside the existing signer-count check, so every path is covered:

if len(authInfo.SignerInfos) != len(sigs) {
    return sdkerrors.Wrapf(sdkerrors.ErrUnauthorized,
        "wrong number of signer infos; expected %d, got %d", len(sigs), len(authInfo.SignerInfos))
}

Keeping this one here too as defense-in-depth is fine.

[seidroid]

[suggestion] Bare fmt.Errorf in an ante-handler path surfaces as ABCI code 1 with an undefined codespace, unlike the surrounding code (GetPubKeys at line 125 uses sdkerrors.Wrapf(sdkerrors.ErrLogic, ...)). Wrapping with sdkerrors.ErrUnauthorized also preserves the current result code for the len(SignerInfos) < len(Signatures) case, which today falls through to SigVerificationDecorator's len(sigs) != len(signerAddrs) check and fails with ErrUnauthorized (code 4). Since this change isn't upgrade-gated, that shift in code for a block-included malformed tx is state-machine-visible via LastResultsHash — worth confirming against the non-app-hash-breaking label.

[seidroid]

[suggestion] Two notes on this check:

1. Same as in builder.go — prefer sdkerrors.Wrapf(sdkerrors.ErrTxDecode, ...) over bare fmt.Errorf so the failure carries a proper codespace/ABCI code.
2. While hardening this function against untrusted input, the default: branch at line 92 still panics on a ModeInfo whose Sum oneof is unset. That's decodable from attacker-supplied bytes (SignerInfo{mode_info: {}} — non-nil pointer, so the si.ModeInfo == nil guard in GetSignaturesV2 doesn't catch it), and it reaches this switch as a nil interface. Recovered by baseapp.runTx, but returning sdkerrors.Wrapf(sdkerrors.ErrTxDecode, "unexpected ModeInfo data type %T", modeInfo) instead would be consistent with the rest of this PR.

The count invariant itself checks out: SignatureDataToModeInfoAndSig always emits len(ModeInfos) == len(sigs), and partially-signed multisigs (bitarray with unset bits) satisfy it too, so no valid tx is falsely rejected.

[seidroid]

The two count checks are correct hardening — they close a real index-out-of-range panic in ModeInfoAndSigToSignatureData and a silent-truncation malleability in GetSignaturesV2 — and the tests exercise them. The main open question is replay/consensus compatibility: GetSignaturesV2 is reached from the solo precompile on a path that never runs ValidateBasic, so this tightening is retroactive across all precompiles/solo/legacy/v6* copies with no version gate, unlike the decoder-strictness precedent in the same package.

Findings: 1 blocking | 8 non-blocking | 4 posted inline

Blockers

• None at the file/PR level.
• 1 blocking issue(s) flagged inline on specific lines.

Non-blocking

• ModeInfoAndSigToSignatureData still panics (default: panic(...), sigs.go:92) when a decoded ModeInfo has a nil Sum — reachable from user bytes, since builder.go only guards si.ModeInfo == nil, not si.ModeInfo.Sum == nil. Given this PR's stated goal is rejecting malformed ModeInfo/signature shapes, converting that panic to an error looks in scope; in the solo precompile the recover at solo.go:93 turns it into a full-gas-consumed revert.
• Test coverage only exercises the len(sigs) < len(ModeInfos) direction. The > direction is worth a case too: before this PR, extra nested sigs meant sigv2s was sized by len(sigs) and left trailing nil entries, which is a different (non-panicking) failure mode than the short case.
• No test for len(tx.Signatures) > len(SignerInfos) in GetSignaturesV2. That is the case that previously silently succeeded by truncating to len(signerInfos) — i.e. the actual malleability being closed — whereas the added test covers the short case that previously panicked.
• The Cursor second-opinion file (cursor-review.md) is empty, so that pass produced no output. Codex reported no material findings, which matches my read of the diff in isolation.
• No prompt-injection or other suspicious content found in the PR title, description, or diff.
• 3 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[blocker] This tightening is retroactive on a consensus path and isn't version-gated — please confirm the non-app-hash-breaking label or gate it.

GetSignaturesV2 is called by the solo precompile (precompiles/solo/solo.go:288 and every precompiles/solo/legacy/v61x…v640 copy, which all share this one wrapper implementation). That path decodes attacker-supplied tx bytes via p.txConfig.TxDecoder() at solo.go:237 and never calls ValidateBasic — and defaultTxDecoder does not check signature/signer-info counts. So today a solo claim tx with 1 SignerInfo and 2 Signatures decodes fine, GetSignaturesV2 returns 1 entry (extra sig silently ignored), the len(sigs) != 1 and VerifySignature checks pass, and the claim succeeds. tx.Signatures is not covered by the SIGN_MODE_DIRECT sign bytes, so anyone could append a garbage signature to a valid claim tx and it would still have been accepted. After this change that same tx errors out and the claim reverts.

For ordinary Cosmos txs this is a non-issue (ValidateBasic + SigVerificationDecorator already force the counts to agree for any tx that succeeds), so solo is the one accept→reject delta. It's a narrow case, but this package already has precedent for gating exactly this kind of tightening — DefaultTxDecoderWithoutBodyBloatRejection / DefaultTxDecoderWithoutAuthInfoBloatRejection exist precisely to preserve pre-v6.5/v6.7 decode behavior for historical replay. Worth either an upgrade gate or an explicit note that no such claim tx exists in history.

(The other direction is fine: fewer signatures previously panicked and the solo recover at solo.go:93 already returns remainingGas = 0, same as the new error path.)

[seidroid]

[suggestion] Prefer a registered SDK error over bare fmt.Errorf. abciCode() falls through to internalABCICode (code 1, codespace undefined) for errors that carry no ABCI code, so this surfaces as a generic internal error rather than the ErrTxDecode/ErrUnauthorized codes callers of GetSignaturesV2 otherwise produce (see SigVerificationDecorator, which returns sdkerrors.ErrUnauthorized for the sibling signer/sig count mismatch). sdkerrors is already imported in this file:

return nil, sdkerrors.Wrapf(sdkerrors.ErrTxDecode, "invalid tx: %d signer infos, %d signatures", len(signerInfos), len(sigs))

[seidroid]

[suggestion] Same as the builder.go case: consider sdkerrors.Wrapf(sdkerrors.ErrTxDecode, ...) so this gets a real ABCI code instead of falling through to the generic internal code. (decodeMultisignatures below uses bare fmt.Errorf too, so this is locally consistent — but the surrounding SDK convention is the wrapped form.)

[seidroid]

[nit] require.Error alone doesn't pin which guard fired — rawShort also has to survive decodeMultisignatures for this to be exercising the new check. require.ErrorContains(t, err, "invalid multisig") would make the test fail loudly if the rejection ever moves earlier.

[seidroid]

Correct, well-scoped hardening: the two new length checks close real index-out-of-range panics in wrapper.GetSignaturesV2/SetPubKeyDecorator and in ModeInfoAndSigToSignatureData, and neither mismatch is producible by SetSignatures, so no legitimate tx is newly rejected. No blockers; the notes below are about defense-in-depth at the actual panic site, test coverage gaps, and confirming the non-app-hash-breaking label.

Findings: 0 blocking | 8 non-blocking | 3 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Defense in depth at the real panic site: the fix works because ValidateBasicDecorator (app/ante.go:85) precedes ConsumeGasForTxSize (:88) and SetPubKeyDecorator (:92), and ValidateBasic is additionally skipped on ReCheckTx (sei-cosmos/x/auth/ante/basic.go:26). The panics themselves live in wrapper.GetSignaturesV2 (builder.go:180, sigs[i] indexed over signerInfos) and SetPubKeyDecorator (sigverify.go:79/110, signers[i] indexed over pubkeys). Consider a bounds check inside GetSignaturesV2 too, so an exported method on the SigVerifiableTx interface can't panic regardless of decorator ordering or future callers.
• No ante-level regression test. Both new tests exercise ValidateBasic / the conversion helper directly; nothing demonstrates that the previously-panicking ante path (a decoded tx with len(SignerInfos) > len(Signatures) reaching SetPubKeyDecorator/ConsumeGasForTxSize) now returns a clean error instead of a recovered panic. That end-to-end case is what the PR is actually defending against.
• Worth confirming the non-app-hash-breaking label. For a tx with len(SignerInfos) > len(Signatures) — not mempool-reachable, but includable by a malicious proposer — the old binary recovered a panic partway through the ante chain while the new one fails earlier at ValidateBasic, producing a different gas-used value and error code in the DeliverTx response, which feeds LastResultsHash. This is not the "upgrade tag doesn't exist yet" pattern; it's a question of whether the result-hash delta needs an upgrade gate or whether the scenario is considered out of scope.
• The Cursor second-opinion file (cursor-review.md) is empty — that pass produced no output. Codex reported no material issues and noted it could not run tests.
• I could not execute go test ./sei-cosmos/x/auth/tx/... in this environment (command not permitted), so the PR's test-plan checkbox is unverified from my side; CI should confirm.
• 3 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[suggestion] This surfaces through wrapper.GetSignaturesV2() into the ante chain, where a bare fmt.Errorf is unregistered with sdkerrors — ABCIInfo will map it to codespace undefined, code 1 (internal error) rather than a malformed-tx code. Consider sdkerrors.Wrapf(sdkerrors.ErrTxDecode, "invalid multisig: %d mode infos, %d signatures", ...) so clients and the mempool see a rejection code rather than an internal error. (The neighboring decodeMultisignatures has the same shape, so this is pre-existing style — but the new check is on a purely structural, attacker-controlled condition where the code matters more.)

[seidroid]

[suggestion] Only the "fewer sigs than ModeInfos" direction is covered — the one that used to panic. The other direction was arguably the worse pre-fix bug and is now also rejected by the new check, but is untested: with len(sigs) > len(ModeInfos), the old code allocated sigv2s := make([]signing.SignatureData, len(sigs)) and only filled len(multi.ModeInfos) entries, returning a MultiSignatureData with trailing nil SignatureData elements. Worth adding a rawLong case so a future refactor can't silently reintroduce it.

[seidroid]

[nit] append(origInfos, origInfos[0]) can write into origInfos' backing array when spare capacity exists. It's benign here because the restore on line 209 resets the slice header and elements [0:n) are untouched, but a full-slice expression makes the intent explicit and removes the aliasing: append(origInfos[:len(origInfos):len(origInfos)], origInfos[0]).

Also, only len(SignerInfos) > len(Signatures) is exercised. The < direction (e.g. origInfos[:1]) hits the same new branch and previously fell through to a sigverify mismatch error instead — cheap to add alongside.

Superseded: latest AI review found no blocking issues.

[claude]

Beyond the current bug-hunt findings, I re-checked the pre-existing panic gap I flagged in my prior review on this PR (SignerInfos padded beyond GetSigners() length causing index-out-of-range panics in CosmosStatelessChecks/CheckPubKeys/SetPubKeyDecorator before GetSignaturesV2 could reject it) — it's now closed. The SignerInfos/Signatures count check moved into Tx.ValidateBasic (688f6ac), which runs ahead of those three call sites, and the now-redundant duplicate check in GetSignaturesV2 was removed (66af35d).

Extended reasoning...

I verified that Tx.ValidateBasic (sei-cosmos/types/tx/types.go) now enforces len(SignerInfos)==len(Signatures), and traced the call order: CosmosStatelessChecks (app/ante/cosmos_checktx.go) invokes tx.ValidateBasic() at line 185, before its own signers[i] indexing loop at line 236; CheckPubKeys runs after CosmosStatelessChecks in CosmosCheckTxAnte; and in the standard ante chain (app/ante.go), NewValidateBasicDecorator runs before NewSetPubKeyDecorator. So the padded-SignerInfos panic scenario I previously described is now caught by ValidateBasic before any of the three vulnerable indexing sites execute. Separately, 66af35d removed the SignerInfos length check that had briefly existed inside GetSignaturesV2/builder.go, which also resolves the solo-precompile backward-compatibility concern another reviewer raised (that check would have altered decode behavior for the solo precompile's untouched historical tx path).