From 515843d16dfcd938bdea4506c157b0c2452353b1 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Tue, 12 Jul 2022 09:55:53 -0400 Subject: [PATCH] Revert "fix: Prevent signing from wrong key in multisig (#12446)" This reverts commit 28f4fb9c39c206de38dfd4afa2f4379a8ebce247. --- client/keys/show_test.go | 2 +- crypto/keyring/output_test.go | 2 +- crypto/keyring/record.go | 11 -------- crypto/keyring/record_test.go | 45 +++------------------------------ x/auth/client/cli/tx_sign.go | 44 ++++++-------------------------- x/auth/client/testutil/suite.go | 21 +++------------ 6 files changed, 17 insertions(+), 108 deletions(-) diff --git a/client/keys/show_test.go b/client/keys/show_test.go index c1d2a9855c13..4045b0d3c482 100644 --- a/client/keys/show_test.go +++ b/client/keys/show_test.go @@ -31,7 +31,7 @@ func Test_multiSigKey_Properties(t *testing.T) { require.Equal(t, "myMultisig", k.Name) require.Equal(t, keyring.TypeMulti, k.GetType()) - pub, err := k.GetMultisigPubKey() + pub, err := k.GetPubKey() require.NoError(t, err) require.Equal(t, "D3923267FA8A3DD367BB768FA8BDC8FF7F89DA3F", pub.Address().String()) diff --git a/crypto/keyring/output_test.go b/crypto/keyring/output_test.go index 61f0d82e4bc3..c7a2e4d65945 100644 --- a/crypto/keyring/output_test.go +++ b/crypto/keyring/output_test.go @@ -20,7 +20,7 @@ func TestBech32KeysOutput(t *testing.T) { k, err := NewMultiRecord("multisig", multisigPk) require.NotNil(t, k) require.NoError(t, err) - pubKey, err := k.GetMultisigPubKey() + pubKey, err := k.GetPubKey() require.NoError(t, err) accAddr := sdk.AccAddress(pubKey.Address()) expectedOutput, err := NewKeyOutput(k.Name, k.GetType(), accAddr, multisigPk) diff --git a/crypto/keyring/record.go b/crypto/keyring/record.go index 6bfbb1561f77..1b1885648e5e 100644 --- a/crypto/keyring/record.go +++ b/crypto/keyring/record.go @@ -5,7 +5,6 @@ import ( codectypes "github.com/cosmos/cosmos-sdk/codec/types" "github.com/cosmos/cosmos-sdk/crypto/hd" - "github.com/cosmos/cosmos-sdk/crypto/keys/multisig" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" "github.com/cosmos/cosmos-sdk/types" ) @@ -60,16 +59,6 @@ func NewMultiRecord(name string, pk cryptotypes.PubKey) (*Record, error) { return newRecord(name, pk, recordMultiItem) } -// GetMultisigPubKey fetches a public key of the multi type record -func (k *Record) GetMultisigPubKey() (*multisig.LegacyAminoPubKey, error) { - pk, ok := k.PubKey.GetCachedValue().(*multisig.LegacyAminoPubKey) - if !ok { - return nil, errors.New("unable to cast any to multisig.LegacyAminoPubKey") - } - - return pk, nil -} - // GetPubKey fetches a public key of the record func (k *Record) GetPubKey() (cryptotypes.PubKey, error) { pk, ok := k.PubKey.GetCachedValue().(cryptotypes.PubKey) diff --git a/crypto/keyring/record_test.go b/crypto/keyring/record_test.go index 1740ea117885..2ec0401d87ff 100644 --- a/crypto/keyring/record_test.go +++ b/crypto/keyring/record_test.go @@ -10,8 +10,6 @@ import ( codectypes "github.com/cosmos/cosmos-sdk/codec/types" "github.com/cosmos/cosmos-sdk/crypto/hd" "github.com/cosmos/cosmos-sdk/crypto/keys/ed25519" - "github.com/cosmos/cosmos-sdk/crypto/keys/multisig" - "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" ) @@ -19,11 +17,10 @@ import ( type RecordTestSuite struct { suite.Suite - appName string - cdc codec.Codec - priv cryptotypes.PrivKey - pub cryptotypes.PubKey - multiPub *multisig.LegacyAminoPubKey + appName string + cdc codec.Codec + priv cryptotypes.PrivKey + pub cryptotypes.PubKey } func (s *RecordTestSuite) SetupSuite() { @@ -31,14 +28,6 @@ func (s *RecordTestSuite) SetupSuite() { s.cdc = getCodec() s.priv = cryptotypes.PrivKey(ed25519.GenPrivKey()) s.pub = s.priv.PubKey() - - multiKey := secp256k1.GenPrivKeyFromSecret([]byte("myMultiKey")) - pk := multisig.NewLegacyAminoPubKey( - 1, - []cryptotypes.PubKey{multiKey.PubKey()}, - ) - s.multiPub = pk - } func (s *RecordTestSuite) TestOfflineRecordMarshaling() { @@ -58,32 +47,6 @@ func (s *RecordTestSuite) TestOfflineRecordMarshaling() { s.Require().True(s.pub.Equals(pk2)) } -func (s *RecordTestSuite) TestMultiRecordMarshaling() { - dir := s.T().TempDir() - mockIn := strings.NewReader("") - - kb, err := New(s.appName, BackendTest, dir, mockIn, s.cdc) - s.Require().NoError(err) - - k, err := NewMultiRecord("testrecord", s.multiPub) - s.Require().NoError(err) - - ks, ok := kb.(keystore) - s.Require().True(ok) - - bz, err := ks.cdc.Marshal(k) - s.Require().NoError(err) - - k2, err := ks.protoUnmarshalRecord(bz) - s.Require().NoError(err) - s.Require().Equal(k.Name, k2.Name) - s.Require().True(k.PubKey.Equal(k2.PubKey)) - - pub2, err := k2.GetMultisigPubKey() - s.Require().NoError(err) - s.Require().True(s.multiPub.Equals(pub2)) -} - func (s *RecordTestSuite) TestLocalRecordMarshaling() { dir := s.T().TempDir() mockIn := strings.NewReader("") diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index 4a11e9695faf..39626e5ed1ac 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -224,6 +224,7 @@ 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 { @@ -231,7 +232,7 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { } printSignatureOnly, _ := cmd.Flags().GetBool(flagSigOnly) - multisigkey, _ := cmd.Flags().GetString(flagMultisig) + multisig, _ := cmd.Flags().GetString(flagMultisig) if err != nil { return err } @@ -242,44 +243,15 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { } overwrite, _ := f.GetBool(flagOverwrite) - if multisigkey != "" { - var ( - multisigName string - multisigAddr sdk.AccAddress - ) - // Bech32 decode error, maybe it's a name, we try to fetch from keyring - multisigAddr, multisigName, _, err = client.GetFromFields(clientCtx, txF.Keybase(), multisigkey) - if err != nil { - return fmt.Errorf("error getting account from keybase: %w", err) - } - multisigRecord, err := clientCtx.Keyring.Key(multisigName) - if err != nil { - return err - } - multisigPub, err := multisigRecord.GetMultisigPubKey() - if err != nil { - return err - } - - fromRecord, err := clientCtx.Keyring.Key(fromName) + if multisig != "" { + multisigAddr, err := sdk.AccAddressFromBech32(multisig) if err != nil { - return err - } - fromPubKey, err := fromRecord.GetPubKey() - if err != nil { - return err - } - - var found bool - for _, pubkey := range multisigPub.GetPubKeys() { - if pubkey.Equals(fromPubKey) { - found = true + // 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) } } - if !found { - return fmt.Errorf("from 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 17affcd34aaf..2508744e2cc6 100644 --- a/x/auth/client/testutil/suite.go +++ b/x/auth/client/testutil/suite.go @@ -74,10 +74,6 @@ 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) @@ -942,10 +938,6 @@ 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,7 +995,7 @@ func (s *IntegrationTestSuite) TestCLIMultisignSortSignatures() { sign1File := testutil.WriteToNewTempFile(s.T(), account1Signature.String()) - // Sign with account2 + // Sign with account1 addr2, err := account2.GetAddress() s.Require().NoError(err) account2Signature, err := TxSignExec(val1.ClientCtx, addr2, multiGeneratedTxFile.Name(), "--multisig", addr.String()) @@ -1011,13 +1003,6 @@ 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(), "from 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) @@ -1072,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(), "error getting account from keybase") + s.Require().Contains(err.Error(), "tx intended signer does not match the given signer") } func (s *IntegrationTestSuite) TestCLIMultisign() { @@ -1139,7 +1124,7 @@ func (s *IntegrationTestSuite) TestCLIMultisign() { addr2, err := account2.GetAddress() s.Require().NoError(err) - // Sign with account2 + // Sign with account1 account2Signature, err := TxSignExec(val1.ClientCtx, addr2, multiGeneratedTxFile.Name(), "--multisig", addr.String()) s.Require().NoError(err)