fix: nested multisig signatures using CLI (backport #20438) (#20692)

Co-authored-by: Facundo Medica <[email protected]>
Co-authored-by: marbar3778 <[email protected]>
Co-authored-by: Facundo <[email protected]>

Author: 3 people

CHANGELOG.md

@@ -46,6 +46,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 * (x/authz,x/feegrant) [#20590](https://github.com/cosmos/cosmos-sdk/pull/20590) Provide updated keeper in depinject for authz and feegrant modules.
 * [#20631](https://github.com/cosmos/cosmos-sdk/pull/20631) Fix json parsing in the wait-tx command.
+* (x/auth) [#20438](https://github.com/cosmos/cosmos-sdk/pull/20438) Add `--skip-signature-verification` flag to multisign command to allow nested multisigs.
 
 ## [v0.50.7](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.7) - 2024-06-04
 

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)
+	require.NoError(t, err)
+
+	out, err := MkAccKeyOutput(k)
+	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)

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
 }

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`

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 {

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 {

x/auth/client/tx.go

@@ -78,16 +78,6 @@ func SignTxWithSignerAddress(txFactory tx.Factory, clientCtx client.Context, add
 		txFactory = txFactory.WithSignMode(signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON)
 	}
 
-	// check whether the address is a signer
-	signers, err := txBuilder.GetTx().GetSigners()
-	if err != nil {
-		return err
-	}
-
-	if !isTxSigner(addr, signers) {
-		return fmt.Errorf("%s: %s", errors.ErrorInvalidSigner, name)
-	}
-
 	if !offline {
 		txFactory, err = populateAccountFromState(txFactory, clientCtx, addr)
 		if err != nil {

x/tx/CHANGELOG.md

@@ -56,18 +56,18 @@ Ref: https://keepachangelog.com/en/1.0.0/
 ### Improvements
 
 * [#15871](https://github.com/cosmos/cosmos-sdk/pull/15871)
-  * `HandlerMap` now has a `DefaultMode()` getter method
-  * Textual types use `signing.ProtoFileResolver` instead of `protoregistry.Files`
+    * `HandlerMap` now has a `DefaultMode()` getter method
+    * Textual types use `signing.ProtoFileResolver` instead of `protoregistry.Files`
 
 ## v0.6.0
 
 ### API Breaking
 
 * [#15709](https://github.com/cosmos/cosmos-sdk/pull/15709):
-  * `GetSignersContext` has been renamed to `signing.Context`
-  * `GetSigners` now returns `[][]byte` instead of `[]string`
-  * `GetSignersOptions` has been renamed to `signing.Options` and requires `address.Codec`s for account and validator addresses
-  * `GetSignersOptions.ProtoFiles` has been renamed to `signing.Options.FileResolver`
+    * `GetSignersContext` has been renamed to `signing.Context`
+    * `GetSigners` now returns `[][]byte` instead of `[]string`
+    * `GetSignersOptions` has been renamed to `signing.Options` and requires `address.Codec`s for account and validator addresses
+    * `GetSignersOptions.ProtoFiles` has been renamed to `signing.Options.FileResolver`
 
 ### Bug Fixes