Fix multisig PubKey VerifyMultisignature (cosmos#7377)

* Fix VerifyMultisignature method * Remove unrelevant test * Remove empty line * Fix verify test Co-authored-by: Robert Zaremba <[email protected]> Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
swelf19 · Sep 24, 2020 · fb0f6d6 · fb0f6d6
1 parent 7ea6b2c
commit fb0f6d6
Show file tree

Hide file tree

Showing 3 changed files with 18 additions and 39 deletions.
diff --git a/crypto/keys/multisig/multisig.go b/crypto/keys/multisig/multisig.go
@@ -72,7 +72,7 @@ func (m *LegacyAminoPubKey) VerifyMultisignature(getSignBytes multisigtypes.GetS
 					return err
 				}
 				if !pubKeys[i].VerifySignature(msg, si.Signature) {
-					return err
+					return fmt.Errorf("unable to verify signature at index %d", i)
 				}
 			case *signing.MultiSignatureData:
 				nestedMultisigPk, ok := pubKeys[i].(multisigtypes.PubKey)

diff --git a/crypto/keys/multisig/multisig_test.go b/crypto/keys/multisig/multisig_test.go
@@ -160,42 +160,6 @@ func TestVerifyMultisignature(t *testing.T) {
 					pk.VerifyMultisignature(signBytesFn, sig),
 					"multisig failed after k good signatures",
 				)
-
-				for i := k + 1; i < len(signingIndices); i++ {
-					signingIndex := signingIndices[i]
-
-					require.NoError(
-						t,
-						multisig.AddSignatureFromPubKey(
-							sig,
-							sigs[signingIndex],
-							pubKeys[signingIndex],
-							pubKeys,
-						),
-					)
-					require.Equal(
-						t,
-						false,
-						pk.VerifyMultisignature(func(mode signing.SignMode) ([]byte, error) {
-							return msg, nil
-						}, sig),
-						"multisig didn't verify as expected after k sigs, i %d", i,
-					)
-					require.NoError(
-						t,
-						multisig.AddSignatureFromPubKey(
-							sig,
-							sigs[signingIndex],
-							pubKeys[signingIndex],
-							pubKeys),
-					)
-					require.Equal(
-						t,
-						i+1,
-						len(sig.Signatures),
-						"adding a signature for the same pubkey twice increased signature count by 2",
-					)
-				}
 			},
 			true,
 		},
@@ -213,6 +177,18 @@ func TestVerifyMultisignature(t *testing.T) {
 			},
 			false,
 		},
+		{
+			"unable to verify signature",
+			func() {
+				pubKeys, _ := generatePubKeysAndSignatures(2, msg)
+				_, sigs := generatePubKeysAndSignatures(2, msg)
+				pk = kmultisig.NewLegacyAminoPubKey(2, pubKeys)
+				sig = multisig.NewMultisig(2)
+				multisig.AddSignatureFromPubKey(sig, sigs[0], pubKeys[0], pubKeys)
+				multisig.AddSignatureFromPubKey(sig, sigs[1], pubKeys[1], pubKeys)
+			},
+			false,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -259,10 +235,10 @@ func TestMultiSigMigration(t *testing.T) {
 
 	cdc := codec.NewLegacyAmino()
 
-	err := multisig.AddSignatureFromPubKey(multisignature, sigs[0], pkSet[0], pkSet)
+	require.NoError(t, multisig.AddSignatureFromPubKey(multisignature, sigs[0], pkSet[0], pkSet))
 
 	// create a StdSignature for msg, and convert it to sigV2
-	sig := legacytx.StdSignature{PubKey: pkSet[1], Signature: msg}
+	sig := legacytx.StdSignature{PubKey: pkSet[1], Signature: sigs[1].(*signing.SingleSignatureData).Signature}
 	sigV2, err := legacytx.StdSignatureToSignatureV2(cdc, sig)
 	require.NoError(t, multisig.AddSignatureV2(multisignature, sigV2, pkSet))
 

diff --git a/x/auth/signing/verify_test.go b/x/auth/signing/verify_test.go
@@ -88,6 +88,9 @@ func TestVerifySignature(t *testing.T) {
 	err = multisig.AddSignatureFromPubKey(multisignature, sig2V2.Data, pkSet[1], pkSet)
 	require.NoError(t, err)
 
+	stdTx = legacytx.NewStdTx(msgs, fee, []legacytx.StdSignature{stdSig1, stdSig2}, memo)
+	stdTx.TimeoutHeight = 10
+
 	err = signing.VerifySignature(multisigKey, signerData, multisignature, handler, stdTx)
 	require.NoError(t, err)
 }