Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wen-coding wants to merge 7 commits into
base: main
Choose a base branch
from
+53 −3
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a86c7ea
Require matching ModeInfo and nested signature counts (CON-393)
wen-coding Aug 5, 2026
e92c707
Address review nits for ModeInfo signature count check
wen-coding Aug 5, 2026
c754c39
Align SignerInfos, signers, and ModeInfo length checks
wen-coding Aug 5, 2026
8b01a64
Narrow CON-393 to ModeInfo and GetSignaturesV2 checks
wen-coding Aug 5, 2026
e3d7133
Compact ModeInfo and GetSignaturesV2 unit tests
wen-coding Aug 5, 2026
688f6ac
Enforce SignerInfos count in ValidateBasic (CON-393)
wen-coding Aug 6, 2026
66af35d
Remove redundant GetSignaturesV2 SignerInfos length check
wen-coding Aug 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions sei-cosmos/types/tx/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,14 @@ func (t *Tx) ValidateBasic() error {
)
}

// SignerInfos and Signatures are 1:1 (see SetSignatures).
if len(authInfo.SignerInfos) != len(sigs) {
return sdkerrors.Wrapf(
sdkerrors.ErrUnauthorized,
"wrong number of SignerInfos; expected %d, got %d", len(sigs), len(authInfo.SignerInfos),
)
}

return nil
}

Expand Down
11 changes: 11 additions & 0 deletions sei-cosmos/x/auth/tx/builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,17 @@ func TestBuilderValidateBasic(t *testing.T) {
err = txBuilder.ValidateBasic()
require.NoError(t, err)

// SignerInfos must match Signatures
origInfos := txBuilder.tx.AuthInfo.SignerInfos
txBuilder.tx.AuthInfo.SignerInfos = append(origInfos, origInfos[0])

@seidroid seidroid Bot Aug 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

err = txBuilder.ValidateBasic()
require.Error(t, err)
_, code, _ = sdkerrors.ABCIInfo(err, false)
require.Equal(t, sdkerrors.ErrUnauthorized.ABCICode(), code)
txBuilder.tx.AuthInfo.SignerInfos = origInfos
err = txBuilder.ValidateBasic()
require.NoError(t, err)

// gas limit too high
txBuilder.SetGasLimit(txtypes.MaxGasWanted + 1)
err = txBuilder.ValidateBasic()
Expand Down
4 changes: 4 additions & 0 deletions sei-cosmos/x/auth/tx/sigs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ func ModeInfoAndSigToSignatureData(modeInfo *tx.ModeInfo, sig []byte) (signing.S
if err != nil {
return nil, err
}
// ModeInfos and nested signatures are 1:1 (see SignatureDataToModeInfoAndSig).
if len(multi.ModeInfos) != len(sigs) {
return nil, fmt.Errorf("invalid multisig: %d mode infos, %d signatures", len(multi.ModeInfos), len(sigs))

@seidroid seidroid Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 seidroid Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 seidroid Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 seidroid Bot Aug 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.)

}

sigv2s := make([]signing.SignatureData, len(sigs))
for i, mi := range multi.ModeInfos {
Expand Down
33 changes: 30 additions & 3 deletions sei-cosmos/x/auth/tx/sigs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@ import (

"github.com/stretchr/testify/require"

"github.com/sei-protocol/sei-chain/sei-cosmos/crypto/types"

cryptotypes "github.com/sei-protocol/sei-chain/sei-cosmos/crypto/types"
"github.com/sei-protocol/sei-chain/sei-cosmos/crypto/types/multisig"
"github.com/sei-protocol/sei-chain/sei-cosmos/testutil/testdata"
txtypes "github.com/sei-protocol/sei-chain/sei-cosmos/types/tx"
"github.com/sei-protocol/sei-chain/sei-cosmos/types/tx/signing"
)

func TestDecodeMultisignatures(t *testing.T) {
Expand All @@ -27,7 +29,7 @@ func TestDecodeMultisignatures(t *testing.T) {
_, err = decodeMultisignatures(bz)
require.Error(t, err)

goodMultisig := types.MultiSignature{
goodMultisig := cryptotypes.MultiSignature{
Signatures: testSigs,
}
bz, err = goodMultisig.Marshal()
Expand All @@ -38,3 +40,28 @@ func TestDecodeMultisignatures(t *testing.T) {

require.Equal(t, testSigs, decodedSigs)
}

func TestModeInfoAndSigToSignatureData(t *testing.T) {
msig := multisig.NewMultisig(2)
multisig.AddSignature(msig, &signing.SingleSignatureData{
SignMode: signing.SignMode_SIGN_MODE_DIRECT,
Signature: []byte("a"),
}, 0)
modeInfo, raw := SignatureDataToModeInfoAndSig(msig)
got, err := ModeInfoAndSigToSignatureData(modeInfo, raw)
require.NoError(t, err)
require.Equal(t, msig, got)

// fewer nested sigs than ModeInfos must error
rawShort, err := (&cryptotypes.MultiSignature{Signatures: [][]byte{[]byte("a")}}).Marshal()
require.NoError(t, err)
mi := &txtypes.ModeInfo_Single_{Single: &txtypes.ModeInfo_Single{Mode: signing.SignMode_SIGN_MODE_DIRECT}}
bad := &txtypes.ModeInfo{Sum: &txtypes.ModeInfo_Multi_{
Multi: &txtypes.ModeInfo_Multi{
Bitarray: cryptotypes.NewCompactBitArray(2),
ModeInfos: []*txtypes.ModeInfo{{Sum: mi}, {Sum: mi}},
},
}}
_, err = ModeInfoAndSigToSignatureData(bad, rawShort)

@seidroid seidroid Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 seidroid Bot Aug 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

require.Error(t, err)
}
Loading