From ff65fe707302a99811fbdb28424b86383e853021 Mon Sep 17 00:00:00 2001 From: likhita-809 Date: Wed, 13 Jul 2022 11:02:28 +0530 Subject: [PATCH 1/3] wip --- x/auth/client/cli/tx_sign.go | 39 ++++++++++++++++++++++++++------- x/auth/client/testutil/suite.go | 6 ++--- 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index 39626e5ed1ac..5217ee5801cb 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -9,7 +9,8 @@ import ( "github.com/cosmos/cosmos-sdk/client" "github.com/cosmos/cosmos-sdk/client/flags" "github.com/cosmos/cosmos-sdk/client/tx" - sdk "github.com/cosmos/cosmos-sdk/types" + kmultisig "github.com/cosmos/cosmos-sdk/crypto/keys/multisig" + authclient "github.com/cosmos/cosmos-sdk/x/auth/client" ) @@ -224,7 +225,6 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { return err } - txFactory := tx.NewFactoryCLI(clientCtx, cmd.Flags()) txCfg := clientCtx.TxConfig txBuilder, err := txCfg.WrapTxBuilder(newTx) if err != nil { @@ -241,17 +241,40 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { if err != nil { return fmt.Errorf("error getting account from keybase: %w", err) } - + fromRecord, err := clientCtx.Keyring.Key(fromName) + if err != nil { + return fmt.Errorf("error getting account from keybase: %w", err) + } + fromPubKey, err := fromRecord.GetPubKey() + if err != nil { + return err + } overwrite, _ := f.GetBool(flagOverwrite) if multisig != "" { - multisigAddr, err := sdk.AccAddressFromBech32(multisig) + // Bech32 decode error, maybe it's a name, we try to fetch from keyring + multisigAddr, multisigName, _, err := client.GetFromFields(clientCtx, txF.Keybase(), multisig) if err != nil { - // Bech32 decode error, maybe it's a name, we try to fetch from keyring - multisigAddr, _, _, err = client.GetFromFields(clientCtx, txFactory.Keybase(), multisig) - if err != nil { - return fmt.Errorf("error getting account from keybase: %w", err) + return fmt.Errorf("error getting account from keybase: %w", err) + } + multisigkey, err := getMultisigRecord(clientCtx, multisigName) + if err != nil { + return err + } + multisigPubKey, err := multisigkey.GetPubKey() + if err != nil { + return err + } + multisigLegacyPub := multisigPubKey.(*kmultisig.LegacyAminoPubKey) + + var found bool + for _, pubkey := range multisigLegacyPub.GetPubKeys() { + if pubkey.Equals(fromPubKey) { + found = true } } + if !found { + 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/testutil/suite.go b/x/auth/client/testutil/suite.go index 2508744e2cc6..a69c7e1484ec 100644 --- a/x/auth/client/testutil/suite.go +++ b/x/auth/client/testutil/suite.go @@ -995,7 +995,7 @@ func (s *IntegrationTestSuite) TestCLIMultisignSortSignatures() { sign1File := testutil.WriteToNewTempFile(s.T(), account1Signature.String()) - // Sign with account1 + // Sign with account2 addr2, err := account2.GetAddress() s.Require().NoError(err) account2Signature, err := TxSignExec(val1.ClientCtx, addr2, multiGeneratedTxFile.Name(), "--multisig", addr.String()) @@ -1057,7 +1057,7 @@ func (s *IntegrationTestSuite) TestSignWithMultisig() { // as the main point of this test is to test the `--multisig` flag with an address // that is not in the keyring. _, err = TxSignExec(val1.ClientCtx, addr1, multiGeneratedTx2File.Name(), "--multisig", multisigAddr.String()) - s.Require().Contains(err.Error(), "tx intended signer does not match the given signer") + s.Require().Contains(err.Error(), "error getting account from keybase") } func (s *IntegrationTestSuite) TestCLIMultisign() { @@ -1124,7 +1124,7 @@ func (s *IntegrationTestSuite) TestCLIMultisign() { addr2, err := account2.GetAddress() s.Require().NoError(err) - // Sign with account1 + // Sign with account2 account2Signature, err := TxSignExec(val1.ClientCtx, addr2, multiGeneratedTxFile.Name(), "--multisig", addr.String()) s.Require().NoError(err) From a1f75d4a6692c8f41d07acc7e20d059b18e6d35d Mon Sep 17 00:00:00 2001 From: likhita-809 Date: Wed, 13 Jul 2022 11:18:08 +0530 Subject: [PATCH 2/3] add test and changelog --- CHANGELOG.md | 1 + x/auth/client/testutil/suite.go | 15 +++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e0aad63f8c1..451293231ea7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -79,6 +79,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes +* [#12548](https://github.com/cosmos/cosmos-sdk/pull/12548) Prevent signing from wrong key while using multisig. * [#12416](https://github.com/cosmos/cosmos-sdk/pull/12416) Prevent zero gas transactions in the `DeductFeeDecorator` AnteHandler decorator. * (x/mint) [#12384](https://github.com/cosmos/cosmos-sdk/pull/12384) Ensure `GoalBonded` must be positive when performing `x/mint` parameter validation. * (x/auth) [#12261](https://github.com/cosmos/cosmos-sdk/pull/12261) Deprecate pagination in GetTxsEventRequest/Response in favor of page and limit to align with tendermint `SignClient.TxSearch` diff --git a/x/auth/client/testutil/suite.go b/x/auth/client/testutil/suite.go index a69c7e1484ec..2af8d212ccea 100644 --- a/x/auth/client/testutil/suite.go +++ b/x/auth/client/testutil/suite.go @@ -74,6 +74,10 @@ func (s *IntegrationTestSuite) SetupSuite() { pub2, err := account2.GetPubKey() s.Require().NoError(err) + // Create a dummy account for testing purpose + _, _, err = kb.NewMnemonic("dummyAccount", keyring.English, sdk.FullFundraiserPath, keyring.DefaultBIP39Passphrase, hd.Secp256k1) + s.Require().NoError(err) + multi := kmultisig.NewLegacyAminoPubKey(2, []cryptotypes.PubKey{pub1, pub2}) _, err = kb.SaveMultisig("multi", multi) s.Require().NoError(err) @@ -938,6 +942,10 @@ func (s *IntegrationTestSuite) TestCLIMultisignSortSignatures() { multisigRecord, err := val1.ClientCtx.Keyring.Key("multi") s.Require().NoError(err) + // Generate dummy account which is not a part of multisig. + dummyAcc, err := val1.ClientCtx.Keyring.Key("dummyAccount") + s.Require().NoError(err) + addr, err := multisigRecord.GetAddress() s.Require().NoError(err) resp, err := bankcli.QueryBalancesExec(val1.ClientCtx, addr) @@ -1003,6 +1011,13 @@ func (s *IntegrationTestSuite) TestCLIMultisignSortSignatures() { sign2File := testutil.WriteToNewTempFile(s.T(), account2Signature.String()) + // Sign with dummy account + dummyAddr, err := dummyAcc.GetAddress() + s.Require().NoError(err) + _, err = TxSignExec(val1.ClientCtx, dummyAddr, multiGeneratedTxFile.Name(), "--multisig", addr.String()) + s.Require().Error(err) + s.Require().Contains(err.Error(), "signing key is not a part of multisig key") + multiSigWith2Signatures, err := TxMultiSignExec(val1.ClientCtx, multisigRecord.Name, multiGeneratedTxFile.Name(), sign1File.Name(), sign2File.Name()) s.Require().NoError(err) From d22c5fea2746d248a74441616e61f5d308d62126 Mon Sep 17 00:00:00 2001 From: likhita-809 Date: Wed, 13 Jul 2022 20:09:46 +0530 Subject: [PATCH 3/3] address review comments --- x/auth/client/cli/tx_sign.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index 5217ee5801cb..53fa016e6ac2 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -241,14 +241,7 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { if err != nil { return fmt.Errorf("error getting account from keybase: %w", err) } - fromRecord, err := clientCtx.Keyring.Key(fromName) - if err != nil { - return fmt.Errorf("error getting account from keybase: %w", err) - } - fromPubKey, err := fromRecord.GetPubKey() - if err != nil { - return err - } + overwrite, _ := f.GetBool(flagOverwrite) if multisig != "" { // Bech32 decode error, maybe it's a name, we try to fetch from keyring @@ -266,6 +259,15 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { } multisigLegacyPub := multisigPubKey.(*kmultisig.LegacyAminoPubKey) + fromRecord, err := clientCtx.Keyring.Key(fromName) + if err != nil { + return fmt.Errorf("error getting account from keybase: %w", err) + } + fromPubKey, err := fromRecord.GetPubKey() + if err != nil { + return err + } + var found bool for _, pubkey := range multisigLegacyPub.GetPubKeys() { if pubkey.Equals(fromPubKey) {