From 19db35343f1149918f125fa3e986c5d6f3a46dc8 Mon Sep 17 00:00:00 2001 From: Brandon Weng <18161326+BrandonWeng@users.noreply.github.com> Date: Wed, 14 Jun 2023 11:41:11 -0400 Subject: [PATCH] [nice to have] multisig preventing signing with wrong key (#286) ## Describe your changes and provide context Doesn't seem to be absolutely necessary since it would just fail when broadcasting, more of an UI improvement. We can wait until 3.0.4 is out and then merging Copying from: https://github.com/cosmos/cosmos-sdk/pull/12548 ## Testing performed to validate your change unit tests --- x/auth/client/cli/tx_sign.go | 44 +++++++++++++++++++++++++++------ x/auth/client/testutil/suite.go | 19 ++++++++++++-- 2 files changed, 54 insertions(+), 9 deletions(-) diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index 670a5f986..b656f9401 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -4,12 +4,14 @@ import ( "fmt" "os" + "github.com/cosmos/cosmos-sdk/crypto/keyring" "github.com/spf13/cobra" "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" + "github.com/cosmos/cosmos-sdk/types/errors" authclient "github.com/cosmos/cosmos-sdk/x/auth/client" ) @@ -215,7 +217,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 { @@ -235,14 +236,33 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { 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(txF.Keybase(), multisig, clientCtx.GenerateOnly) if err != nil { - // Bech32 decode error, maybe it's a name, we try to fetch from keyring - multisigAddr, _, _, err = client.GetFromFields(txFactory.Keybase(), multisig, clientCtx.GenerateOnly) - 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 := multisigkey.GetPubKey() + multisigLegacyPub := multisigPubKey.(*kmultisig.LegacyAminoPubKey) + + fromRecord, err := clientCtx.Keyring.Key(fromName) + if err != nil { + return fmt.Errorf("error getting account from keybase: %w", err) + } + fromPubKey := fromRecord.GetPubKey() + + 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 { @@ -316,3 +336,13 @@ func marshalSignatureJSON(txConfig client.TxConfig, txBldr client.TxBuilder, sig return txConfig.TxJSONEncoder()(parsedTx) } + +func getMultisigRecord(clientCtx client.Context, name string) (keyring.Info, error) { + kb := clientCtx.Keyring + multisigRecord, err := kb.Key(name) + if err != nil { + return nil, errors.Wrap(err, "error getting keybase multisig account") + } + + return multisigRecord, nil +} diff --git a/x/auth/client/testutil/suite.go b/x/auth/client/testutil/suite.go index 42c42cd9a..afa73fb19 100644 --- a/x/auth/client/testutil/suite.go +++ b/x/auth/client/testutil/suite.go @@ -60,6 +60,10 @@ func (s *IntegrationTestSuite) SetupSuite() { account2, _, err := kb.NewMnemonic("newAccount2", keyring.English, sdk.FullFundraiserPath, keyring.DefaultBIP39Passphrase, hd.Secp256k1) 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{account1.GetPubKey(), account2.GetPubKey()}) _, err = kb.SaveMultisig("multi", multi) s.Require().NoError(err) @@ -737,6 +741,10 @@ func (s *IntegrationTestSuite) TestCLIMultisignSortSignatures() { multisigInfo, 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) + resp, err := bankcli.QueryBalancesExec(val1.ClientCtx, multisigInfo.GetAddress()) s.Require().NoError(err) @@ -789,12 +797,19 @@ func (s *IntegrationTestSuite) TestCLIMultisignSortSignatures() { sign1File := testutil.WriteToNewTempFile(s.T(), account1Signature.String()) - // Sign with account1 + // Sign with account2 account2Signature, err := TxSignExec(val1.ClientCtx, account2.GetAddress(), multiGeneratedTxFile.Name(), "--multisig", multisigInfo.GetAddress().String()) s.Require().NoError(err) sign2File := testutil.WriteToNewTempFile(s.T(), account2Signature.String()) + // Sign with dummy account + dummyAddr := dummyAcc.GetAddress() + s.Require().NoError(err) + _, err = TxSignExec(val1.ClientCtx, dummyAddr, multiGeneratedTxFile.Name(), "--multisig", multisigInfo.GetAddress().String()) + s.Require().Error(err) + s.Require().Contains(err.Error(), "signing key is not a part of multisig key") + multiSigWith2Signatures, err := TxMultiSignExec(val1.ClientCtx, multisigInfo.GetName(), multiGeneratedTxFile.Name(), sign1File.Name(), sign2File.Name()) s.Require().NoError(err) @@ -849,7 +864,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() {