refactor(crypto): add check in multisig verification (#23395)

Co-authored-by: Alex | Interchain Labs <alex@skip.money>
(cherry picked from commit 642e881)

crypto/keys/multisig/multisig.go

@@ -55,6 +55,14 @@ func (m *LegacyAminoPubKey) VerifyMultisignature(getSignBytes multisigtypes.GetS
 	sigs := sig.Signatures
 	size := bitarray.Count()
 	pubKeys := m.GetPubKeys()
+
+	// N-M rule verification
+	if int(m.Threshold) <= 0 {
+		return fmt.Errorf("invalid threshold: must be > 0, got %d", m.Threshold)
+	}
+	if len(pubKeys) <= int(m.Threshold) {
+		return fmt.Errorf("invalid N-M multisig: N (%d) must be > M (%d)", len(pubKeys), m.Threshold)
+	}
 	// ensure bit array is the correct size
 	if len(pubKeys) != size {
 		return fmt.Errorf("bit array size is incorrect, expecting: %d", len(pubKeys))

crypto/keys/multisig/multisig_test.go

@@ -260,8 +260,8 @@ func TestAddSignatureFromPubKeyNilCheck(t *testing.T) {
 
 func TestMultiSigMigration(t *testing.T) {
 	msg := []byte{1, 2, 3, 4}
-	pkSet, sigs := generatePubKeysAndSignatures(2, msg)
-	multisignature := multisig.NewMultisig(2)
+	pkSet, sigs := generatePubKeysAndSignatures(3, msg)
+	multisignature := multisig.NewMultisig(3)
 
 	multisigKey := kmultisig.NewLegacyAminoPubKey(2, pkSet)
 	signBytesFn := func(mode signing.SignMode) ([]byte, error) { return msg, nil }
@@ -318,6 +318,22 @@ func generatePubKeysAndSignatures(n int, msg []byte) (pubKeys []cryptotypes.PubK
 	return
 }
 
+// generateNestedMultiSignature creates a nested multisig structure for testing purposes.
+// It generates a top-level multisig with 'n' sub-multisigs, where each sub-multisig contains
+// 5 individual signatures.
+//
+// Parameters:
+// - n: number of nested multisigs to generate (determines the size of the top-level multisig)
+// - msg: the message to be signed
+//
+// Returns:
+// - multisig.PubKey: the top-level multisig public key containing n sub-multisig public keys
+// - *signing.MultiSignatureData: the corresponding signature data structure containing n sub-multisig signatures
+//
+// Each sub-multisig is configured with:
+// - threshold of 5 (requires all 4 signatures)
+// - 5 individual secp256k1 public keys and their corresponding signatures
+// All signature bits are set to true to simulate a fully signed multisig.
 func generateNestedMultiSignature(n int, msg []byte) (multisig.PubKey, *signing.MultiSignatureData) {
 	pubKeys := make([]cryptotypes.PubKey, n)
 	signatures := make([]signing.SignatureData, n)
@@ -333,10 +349,10 @@ func generateNestedMultiSignature(n int, msg []byte) (multisig.PubKey, *signing.
 			Signatures: nestedSigs,
 		}
 		signatures[i] = nestedSig
-		pubKeys[i] = kmultisig.NewLegacyAminoPubKey(5, nestedPks)
+		pubKeys[i] = kmultisig.NewLegacyAminoPubKey(4, nestedPks)
 		bitArray.SetIndex(i, true)
 	}
-	return kmultisig.NewLegacyAminoPubKey(n, pubKeys), &signing.MultiSignatureData{
+	return kmultisig.NewLegacyAminoPubKey(n-1, pubKeys), &signing.MultiSignatureData{
 		BitArray:   bitArray,
 		Signatures: signatures,
 	}
@@ -453,3 +469,107 @@ func TestAminoUnmarshalJSON(t *testing.T) {
 		require.NoError(t, err)
 	}
 }
+
+func TestVerifyMultisignatureNMRule(t *testing.T) {
+	makeTestKeysAndSignatures := func(n int, msg []byte) ([]cryptotypes.PubKey, []signing.SignatureData) {
+		pubKeys := make([]cryptotypes.PubKey, n)
+		sigs := make([]signing.SignatureData, n)
+
+		for i := 0; i < n; i++ {
+			// Generate private key and get its public key
+			privKey := secp256k1.GenPrivKey()
+			pubKeys[i] = privKey.PubKey()
+
+			// Create real signature
+			sig, err := privKey.Sign(msg)
+			require.NoError(t, err)
+			sigs[i] = &signing.SingleSignatureData{
+				Signature: sig,
+			}
+		}
+		return pubKeys, sigs
+	}
+
+	tests := []struct {
+		name      string
+		n         int    // number of keys
+		m         uint32 // threshold
+		expectErr string
+	}{
+		{
+			name:      "valid case: N=3 M=2",
+			n:         3,
+			m:         2,
+			expectErr: "",
+		},
+		{
+			name:      "invalid: M=0",
+			n:         3,
+			m:         0,
+			expectErr: "invalid threshold: must be > 0, got 0",
+		},
+		{
+			name:      "invalid: N=M",
+			n:         3,
+			m:         3,
+			expectErr: "invalid N-M multisig: N (3) must be > M (3)",
+		},
+		{
+			name:      "invalid: N < M",
+			n:         2,
+			m:         3,
+			expectErr: "invalid N-M multisig: N (2) must be > M (3)",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			msg := []byte("test message")
+			pubKeys, sigs := makeTestKeysAndSignatures(tc.n, msg)
+
+			// Create the key directly instead of using NewLegacyAminoPubKey
+			pubKeysAny := make([]*types.Any, len(pubKeys))
+			for i, key := range pubKeys {
+				anyKey, err := types.NewAnyWithValue(key)
+				require.NoError(t, err)
+				pubKeysAny[i] = anyKey
+			}
+
+			multisigKey := &kmultisig.LegacyAminoPubKey{
+				Threshold: tc.m,
+				PubKeys:   pubKeysAny,
+			}
+
+			// Create a dummy signature with a proper bit array
+			bitArray := cryptotypes.NewCompactBitArray(tc.n)
+			// Set the first M bits to simulate M signatures
+			for i := 0; i < int(tc.m) && i < tc.n; i++ {
+				bitArray.SetIndex(i, true)
+			}
+
+			multiSig := &signing.MultiSignatureData{
+				BitArray:   bitArray,
+				Signatures: make([]signing.SignatureData, 0, tc.m),
+			}
+
+			// Fill in dummy signatures
+			for i := 0; i < int(tc.m) && i < tc.n; i++ {
+				multiSig.Signatures = append(multiSig.Signatures, sigs[i])
+			}
+
+			// Create getSignBytes function that returns our test message
+			getSignBytes := func(mode signing.SignMode) ([]byte, error) {
+				return msg, nil
+			}
+
+			err := multisigKey.VerifyMultisignature(getSignBytes, multiSig)
+
+			if tc.expectErr == "" {
+				require.NoError(t, err)
+			} else {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tc.expectErr)
+			}
+		})
+	}
+}

tests/integration/auth/client/cli/suite_test.go

@@ -85,6 +85,8 @@ func (s *CLITestSuite) SetupSuite() {
 	s.Require().NoError(err)
 	s.val, err = valAcc.GetAddress()
 	s.Require().NoError(err)
+	pub, err := valAcc.GetPubKey()
+	s.Require().NoError(err)
 
 	account1, _, err := kb.NewMnemonic("newAccount1", keyring.English, sdk.FullFundraiserPath, keyring.DefaultBIP39Passphrase, hd.Secp256k1)
 	s.Require().NoError(err)
@@ -102,7 +104,8 @@ func (s *CLITestSuite) SetupSuite() {
 	_, _, err = kb.NewMnemonic("dummyAccount", keyring.English, sdk.FullFundraiserPath, keyring.DefaultBIP39Passphrase, hd.Secp256k1)
 	s.Require().NoError(err)
 
-	multi := kmultisig.NewLegacyAminoPubKey(2, []cryptotypes.PubKey{pub1, pub2})
+	// create a 2-3 multisig
+	multi := kmultisig.NewLegacyAminoPubKey(2, []cryptotypes.PubKey{pub, pub1, pub2})
 	_, err = kb.SaveMultisig("multi", multi)
 	s.Require().NoError(err)