fix: nested multisig signatures using CLI (#20438)

(cherry picked from commit b9ca318) # Conflicts: # x/auth/client/tx.go # x/tx/CHANGELOG.md
cosmos · Jun 17, 2024 · 15b66bf · 15b66bf
1 parent ea16396
commit 15b66bf
Show file tree

Hide file tree

Showing 7 changed files with 217 additions and 18 deletions.
diff --git a/client/keys/output_test.go b/client/keys/output_test.go
@@ -44,6 +44,30 @@ func TestBech32KeysOutput(t *testing.T) {
 	require.Equal(t, "{Name:multisig Type:multi Address:cosmos1nf8lf6n4wa43rzmdzwe6hkrnw5guekhqt595cw PubKey:{\"@type\":\"/cosmos.crypto.multisig.LegacyAminoPubKey\",\"threshold\":1,\"public_keys\":[{\"@type\":\"/cosmos.crypto.secp256k1.PubKey\",\"key\":\"AurroA7jvfPd1AadmmOvWM2rJSwipXfRf8yD6pLbA2DJ\"}]} Mnemonic:}", fmt.Sprintf("%+v", out))
 }
 
+// TestBech32KeysOutputNestedMsig tests that the output of a nested multisig key is correct
+func TestBech32KeysOutputNestedMsig(t *testing.T) {
+	sk := secp256k1.PrivKey{Key: []byte{154, 49, 3, 117, 55, 232, 249, 20, 205, 216, 102, 7, 136, 72, 177, 2, 131, 202, 234, 81, 31, 208, 46, 244, 179, 192, 167, 163, 142, 117, 246, 13}}
+	tmpKey := sk.PubKey()
+	nestedMultiSig := kmultisig.NewLegacyAminoPubKey(1, []types.PubKey{tmpKey})
+	multisigPk := kmultisig.NewLegacyAminoPubKey(2, []types.PubKey{tmpKey, nestedMultiSig})
+	k, err := keyring.NewMultiRecord("multisig", multisigPk)
+	require.NotNil(t, k)
+	require.NoError(t, err)
+
+	pubKey, err := k.GetPubKey()
+	require.NoError(t, err)
+
+	accAddr := sdk.AccAddress(pubKey.Address())
+	expectedOutput, err := NewKeyOutput(k.Name, k.GetType(), accAddr, multisigPk, addresscodec.NewBech32Codec("cosmos"))
+	require.NoError(t, err)
+
+	out, err := MkAccKeyOutput(k, addresscodec.NewBech32Codec("cosmos"))
+	require.NoError(t, err)
+
+	require.Equal(t, expectedOutput, out)
+	require.Equal(t, "{Name:multisig Type:multi Address:cosmos1nffp6v2j7wva4y4975exlrv8x5vh39axxt3swz PubKey:{\"@type\":\"/cosmos.crypto.multisig.LegacyAminoPubKey\",\"threshold\":2,\"public_keys\":[{\"@type\":\"/cosmos.crypto.secp256k1.PubKey\",\"key\":\"AurroA7jvfPd1AadmmOvWM2rJSwipXfRf8yD6pLbA2DJ\"},{\"@type\":\"/cosmos.crypto.multisig.LegacyAminoPubKey\",\"threshold\":1,\"public_keys\":[{\"@type\":\"/cosmos.crypto.secp256k1.PubKey\",\"key\":\"AurroA7jvfPd1AadmmOvWM2rJSwipXfRf8yD6pLbA2DJ\"}]}]} Mnemonic:}", fmt.Sprintf("%+v", out))
+}
+
 func TestProtoMarshalJSON(t *testing.T) {
 	require := require.New(t)
 	pubkeys := generatePubKeys(3)

diff --git a/crypto/keys/multisig/multisig.go b/crypto/keys/multisig/multisig.go
@@ -169,6 +169,11 @@ func packPubKeys(pubKeys []cryptotypes.PubKey) ([]*types.Any, error) {
 			return nil, err
 		}
 		anyPubKeys[i] = any
+
+		// sets the compat.aminoBz value
+		if err := anyPubKeys[i].UnmarshalAmino(pubKeys[i].Bytes()); err != nil {
+			return nil, err
+		}
 	}
 	return anyPubKeys, nil
 }
diff --git a/x/auth/README.md b/x/auth/README.md
@@ -447,6 +447,22 @@ simd tx multisign transaction.json k1k2k3 k1sig.json k2sig.json k3sig.json
 
 Where `k1k2k3` is the multisig account address, `k1sig.json` is the signature of the first signer, `k2sig.json` is the signature of the second signer, and `k3sig.json` is the signature of the third signer.
 
+##### Nested multisig transactions
+
+To allow transactions to be signed by nested multisigs, meaning that a participant of a multisig account can be another multisig account, the `--skip-signature-verification` flag must be used.
+
+```bash
+# First aggregate signatures of the multisig participant
+simd tx multi-sign transaction.json ms1 ms1p1sig.json ms1p2sig.json --signature-only --skip-signature-verification > ms1sig.json
+
+# Then use the aggregated signatures and the other signatures to sign the final transaction
+simd tx multi-sign transaction.json k1ms1 k1sig.json ms1sig.json --skip-signature-verification
+```
+
+Where `ms1` is the nested multisig account address, `ms1p1sig.json` is the signature of the first participant of the nested multisig account, `ms1p2sig.json` is the signature of the second participant of the nested multisig account, and `ms1sig.json` is the aggregated signature of the nested multisig account.
+
+`k1ms1` is a multisig account comprised of an individual signer and another nested multisig account (`ms1`). `k1sig.json` is the signature of the first signer of the individual member.
+
 More information about the `multi-sign` command can be found running `simd tx multi-sign --help`.
 
 #### `multisign-batch`

diff --git a/x/auth/client/cli/tx_multisign.go b/x/auth/client/cli/tx_multisign.go
@@ -48,6 +48,10 @@ If the --offline flag is on, the client will not reach out to an external node.
 Account number or sequence number lookups are not performed so you must
 set these parameters manually.
 
+If the --skip-signature-verification flag is on, the command will not verify the
+signatures in the provided signature files. This is useful when the multisig
+account is a signer in a nested multisig scenario.
+
 The current multisig implementation defaults to amino-json sign mode.
 The SIGN_MODE_DIRECT sign mode is not supported.'
 `,
@@ -58,6 +62,7 @@ The SIGN_MODE_DIRECT sign mode is not supported.'
 		Args: cobra.MinimumNArgs(3),
 	}
 
+	cmd.Flags().Bool(flagSkipSignatureVerification, false, "Skip signature verification")
 	cmd.Flags().Bool(flagSigOnly, false, "Print only the generated signature, then exit")
 	cmd.Flags().String(flags.FlagOutputDocument, "", "The document is written to the given file instead of STDOUT")
 	flags.AddTxFlagsToCmd(cmd)
@@ -105,6 +110,10 @@ func makeMultiSignCmd() func(cmd *cobra.Command, args []string) (err error) {
 			return err
 		}
 
+		// avoid signature verification if the sender of the tx is different than
+		// the multisig key (useful for nested multisigs).
+		skipSigVerify, _ := cmd.Flags().GetBool(flagSkipSignatureVerification)
+
 		multisigPub := pubKey.(*kmultisig.LegacyAminoPubKey)
 		multisigSig := multisig.NewMultisig(len(multisigPub.PubKeys))
 		if !clientCtx.Offline {
@@ -149,11 +158,13 @@ func makeMultiSignCmd() func(cmd *cobra.Command, args []string) (err error) {
 				}
 				txData := adaptableTx.GetSigningTxData()
 
-				err = signing.VerifySignature(cmd.Context(), sig.PubKey, txSignerData, sig.Data,
-					txCfg.SignModeHandler(), txData)
-				if err != nil {
-					addr, _ := sdk.AccAddressFromHexUnsafe(sig.PubKey.Address().String())
-					return fmt.Errorf("couldn't verify signature for address %s", addr)
+				if !skipSigVerify {
+					err = signing.VerifySignature(cmd.Context(), sig.PubKey, txSignerData, sig.Data,
+						txCfg.SignModeHandler(), txData)
+					if err != nil {
+						addr, _ := sdk.AccAddressFromHexUnsafe(sig.PubKey.Address().String())
+						return fmt.Errorf("couldn't verify signature for address %s %w", addr, err)
+					}
 				}
 
 				if err := multisig.AddSignatureV2(multisigSig, sig, multisigPub.GetPubKeys()); err != nil {

diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go
@@ -10,16 +10,18 @@ import (
 	"github.com/cosmos/cosmos-sdk/client/flags"
 	"github.com/cosmos/cosmos-sdk/client/tx"
 	kmultisig "github.com/cosmos/cosmos-sdk/crypto/keys/multisig"
+	cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	authclient "github.com/cosmos/cosmos-sdk/x/auth/client"
 )
 
 const (
-	flagMultisig        = "multisig"
-	flagOverwrite       = "overwrite"
-	flagSigOnly         = "signature-only"
-	flagNoAutoIncrement = "no-auto-increment"
-	flagAppend          = "append"
+	flagMultisig                  = "multisig"
+	flagOverwrite                 = "overwrite"
+	flagSigOnly                   = "signature-only"
+	flagSkipSignatureVerification = "skip-signature-verification"
+	flagNoAutoIncrement           = "no-auto-increment"
+	flagAppend                    = "append"
 )
 
 // GetSignBatchCommand returns the transaction sign-batch command.
@@ -228,11 +230,40 @@ func sign(clientCtx client.Context, txBuilder client.TxBuilder, txFactory tx.Fac
 }
 
 func multisigSign(clientCtx client.Context, txBuilder client.TxBuilder, txFactory tx.Factory, multisig string) error {
-	multisigAddr, _, _, err := client.GetFromFields(clientCtx, txFactory.Keybase(), multisig)
+	multisigAddr, multisigName, _, err := client.GetFromFields(clientCtx, txFactory.Keybase(), multisig)
 	if err != nil {
 		return fmt.Errorf("error getting account from keybase: %w", err)
 	}
 
+	fromRecord, err := clientCtx.Keyring.Key(clientCtx.FromName)
+	if err != nil {
+		return fmt.Errorf("error getting account from keybase: %w", err)
+	}
+
+	fromPubKey, err := fromRecord.GetPubKey()
+	if err != nil {
+		return err
+	}
+
+	multisigkey, err := clientCtx.Keyring.Key(multisigName)
+	if err != nil {
+		return err
+	}
+
+	multisigPubKey, err := multisigkey.GetPubKey()
+	if err != nil {
+		return err
+	}
+
+	isSigner, err := isMultisigSigner(clientCtx, multisigPubKey, fromPubKey)
+	if err != nil {
+		return err
+	}
+
+	if !isSigner {
+		return fmt.Errorf("signing key is not a part of multisig key")
+	}
+
 	if err = authclient.SignTxWithSignerAddress(
 		txFactory,
 		clientCtx,
@@ -248,6 +279,33 @@ func multisigSign(clientCtx client.Context, txBuilder client.TxBuilder, txFactor
 	return nil
 }
 
+// isMultisigSigner checks if the given pubkey is a signer in the multisig or in
+// any of the nested multisig signers.
+func isMultisigSigner(clientCtx client.Context, multisigPubKey, fromPubKey cryptotypes.PubKey) (bool, error) {
+	multisigLegacyPub := multisigPubKey.(*kmultisig.LegacyAminoPubKey)
+
+	var found bool
+	for _, pubkey := range multisigLegacyPub.GetPubKeys() {
+		if pubkey.Equals(fromPubKey) {
+			found = true
+			break
+		}
+
+		if nestedMultisig, ok := pubkey.(*kmultisig.LegacyAminoPubKey); ok {
+			var err error
+			found, err = isMultisigSigner(clientCtx, nestedMultisig, fromPubKey)
+			if err != nil {
+				return false, err
+			}
+			if found {
+				break
+			}
+		}
+	}
+
+	return found, nil
+}
+
 func setOutputFile(cmd *cobra.Command) (func(), error) {
 	outputDoc, _ := cmd.Flags().GetString(flags.FlagOutputDocument)
 	if outputDoc == "" {
@@ -373,7 +431,6 @@ func signTx(cmd *cobra.Command, clientCtx client.Context, txF tx.Factory, newTx
 		if err != nil {
 			return err
 		}
-		multisigLegacyPub := multisigPubKey.(*kmultisig.LegacyAminoPubKey)
 
 		fromRecord, err := clientCtx.Keyring.Key(fromName)
 		if err != nil {
@@ -384,15 +441,14 @@ func signTx(cmd *cobra.Command, clientCtx client.Context, txF tx.Factory, newTx
 			return err
 		}
 
-		var found bool
-		for _, pubkey := range multisigLegacyPub.GetPubKeys() {
-			if pubkey.Equals(fromPubKey) {
-				found = true
-			}
+		isSigner, err := isMultisigSigner(clientCtx, multisigPubKey, fromPubKey)
+		if err != nil {
+			return err
 		}
-		if !found {
+		if !isSigner {
 			return fmt.Errorf("signing key is not a part of multisig key")
 		}
+
 		err = authclient.SignTxWithSignerAddress(
 			txF, clientCtx, multisigAddr, fromName, txBuilder, clientCtx.Offline, overwrite)
 		if err != nil {

diff --git a/x/auth/client/tx.go b/x/auth/client/tx.go
@@ -78,6 +78,7 @@ func SignTxWithSignerAddress(txFactory tx.Factory, clientCtx client.Context, add
 		txFactory = txFactory.WithSignMode(signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON)
 	}
 
+<<<<<<< HEAD
 	// check whether the address is a signer
 	signers, err := txBuilder.GetTx().GetSigners()
 	if err != nil {
@@ -88,6 +89,8 @@ func SignTxWithSignerAddress(txFactory tx.Factory, clientCtx client.Context, add
 		return fmt.Errorf("%s: %s", errors.ErrorInvalidSigner, name)
 	}
 
+=======
+>>>>>>> b9ca31812 (fix: nested multisig signatures using CLI (#20438))
 	if !offline {
 		txFactory, err = populateAccountFromState(txFactory, clientCtx, addr)
 		if err != nil {

diff --git a/x/tx/CHANGELOG.md b/x/tx/CHANGELOG.md
@@ -31,6 +31,90 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ## [Unreleased]
 
+<<<<<<< HEAD
+=======
+### Improvements
+
+* [#20438](https://github.com/cosmos/cosmos-sdk/pull/20438) Add `--skip-signature-verification` flag to multisign command to allow nested multisigs.
+
+## [v0.13.3](https://github.com/cosmos/cosmos-sdk/releases/tag/x/tx/v0.13.3) - 2024-04-22
+
+### Improvements
+
+* [#20049](https://github.com/cosmos/cosmos-sdk/pull/20049) Sort JSON attributes for `inline_json` encoder.
+
+## [v0.13.2](https://github.com/cosmos/cosmos-sdk/releases/tag/x/tx/v0.13.2) - 2024-04-12
+
+### Features
+
+* [#19786](https://github.com/cosmos/cosmos-sdk/pull/19786)/[#19919](https://github.com/cosmos/cosmos-sdk/pull/19919) Add "inline_json" option to Amino JSON encoder.
+
+### Improvements
+
+* [#19845](https://github.com/cosmos/cosmos-sdk/pull/19845) Use hybrid resolver instead of only protov2 registry
+
+### Bug Fixes
+
+* [#19955](https://github.com/cosmos/cosmos-sdk/pull/19955) Don't shadow Amino marshalling error messages
+
+## [v0.13.1](https://github.com/cosmos/cosmos-sdk/releases/tag/x/tx/v0.13.1) - 2024-03-05
+
+### Features
+
+* [#19618](https://github.com/cosmos/cosmos-sdk/pull/19618) Add enum as string option to encoder.
+
+### Improvements
+
+* [#18857](https://github.com/cosmos/cosmos-sdk/pull/18857) Moved `FormatCoins` from `core/coins` to this package under `signing/textual`.
+
+### Bug Fixes
+
+* [#19265](https://github.com/cosmos/cosmos-sdk/pull/19265) Reject denoms that contain a comma.
+
+## [v0.13.0](https://github.com/cosmos/cosmos-sdk/releases/tag/x/tx/v0.13.0) - 2023-12-19
+
+### Improvements
+
+* [#18740](https://github.com/cosmos/cosmos-sdk/pull/18740) Support nested messages when fetching signers up to a default depth of 32.
+
+## v0.12.0
+
+### Improvements
+
+* [#18309](https://github.com/cosmos/cosmos-sdk/pull/18309) Update encoder so that amino types default to msg type url.
+
+## v0.11.0
+
+### Improvements
+
+* [#17787](https://github.com/cosmos/cosmos-sdk/pull/17787) Drop tip support.
+
+## v0.10.0
+
+### Features
+
+* [#17681](https://github.com/cosmos/cosmos-sdk/pull/17681) Add encoder `DefineTypeEncoding` method for defining custom type encodings.
+* [#17600](https://github.com/cosmos/cosmos-sdk/pull/17600) Add encoder `DefineScalarEncoding` method for defining custom scalar encodings.
+* [#17600](https://github.com/cosmos/cosmos-sdk/pull/17600) Add indent option to encoder.
+
+## v0.9.1
+
+### Improvements
+
+* [#16936](https://github.com/cosmos/cosmos-sdk/pull/16936) Remove extra whitespace when marshalling module accounts.
+
+## v0.9.0
+
+### Bug Fixes
+
+* [#16681](https://github.com/cosmos/cosmos-sdk/pull/16681): Catch and fix `(*Decoder).Decode` crash from invalid length prefix in Tx bytes.
+
+### Improvements
+
+* [#16846](https://github.com/cosmos/cosmos-sdk/pull/16846): Harmonize interface `signing.TypeResolver` with the rest of the codebase (orm and client/v2).
+* [#16684](https://github.com/cosmos/cosmos-sdk/pull/16684): Use `io.WriteString`+`fmt.Fprintf` to remove unnecessary `string`->`[]byte` roundtrip.
+
+>>>>>>> b9ca31812 (fix: nested multisig signatures using CLI (#20438))
 ## v0.8.0
 
 ### Improvements