From 5db395bcca7974b9d002e760f845ff7cee800559 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Wed, 19 Jun 2024 22:25:22 +0200 Subject: [PATCH] fix: nested multisig signatures using CLI (backport #20438) (#20692) Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Co-authored-by: marbar3778 Co-authored-by: Facundo --- CHANGELOG.md | 1 + client/keys/output_test.go | 24 +++++++++ crypto/keys/multisig/multisig.go | 5 ++ x/auth/README.md | 16 ++++++ x/auth/client/cli/tx_multisign.go | 21 ++++++-- x/auth/client/cli/tx_sign.go | 82 ++++++++++++++++++++++++++----- x/auth/client/tx.go | 10 ---- x/tx/CHANGELOG.md | 12 ++--- 8 files changed, 137 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b061e87d52a9..04757e74d396 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/client/keys/output_test.go b/client/keys/output_test.go index 283734b848b3..fc8e4bf3214c 100644 --- 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) + 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) diff --git a/crypto/keys/multisig/multisig.go b/crypto/keys/multisig/multisig.go index 10f7d2e04000..e28aa012d813 100644 --- 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 index 1d5e22b5d874..c51d10634ad4 100644 --- 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 index 3b919b96209b..d4195585aa2b 100644 --- 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 index fc993c9393ba..a5dec4d00fb7 100644 --- 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 index a2e829641f1f..027e27d3b05c 100644 --- a/x/auth/client/tx.go +++ b/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 { diff --git a/x/tx/CHANGELOG.md b/x/tx/CHANGELOG.md index 742122009eb3..d26a47f4b6bd 100644 --- a/x/tx/CHANGELOG.md +++ b/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