Skip to content

Commit

Permalink
[nice to have] multisig preventing signing with wrong key (#286)
Browse files Browse the repository at this point in the history
## 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: cosmos/cosmos-sdk#12548

## Testing performed to validate your change
unit tests
  • Loading branch information
BrandonWeng authored Jun 14, 2023
1 parent 8e7f327 commit 19db353
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 9 deletions.
44 changes: 37 additions & 7 deletions x/auth/client/cli/tx_sign.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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
}
19 changes: 17 additions & 2 deletions x/auth/client/testutil/suite.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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() {
Expand Down

0 comments on commit 19db353

Please sign in to comment.