fix: check multisig key list to prevent unexpected key deletion (#737)

* fix: reject multisig key addition about duplicate name * test: add multisig unit test * fix: optimize flag replacing logic * replace custom iteration with keyring built-in function * add test for multisig threshold
Finschia · Oct 21, 2022 · 1e53873 · 1e53873
1 parent aa713c4
commit 1e53873
Show file tree

Hide file tree

Showing 3 changed files with 116 additions and 17 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -116,6 +116,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (x/foundation) [\#732](https://github.com/line/lbm-sdk/pull/732) add verification on accounts into x/foundation Grants cli
 * (x/foundation) [\#730](https://github.com/line/lbm-sdk/pull/730) prune stale x/foundation proposals at voting period end
 * (cli) [\#734](https://github.com/line/lbm-sdk/pull/734) add restrictions on the number of args in the CLIs
+* (client) [\#737](https://github.com/line/lbm-sdk/pull/737) check multisig key list to prevent unexpected key deletion
 
 ### Breaking Changes
 * (proto) [\#564](https://github.com/line/lbm-sdk/pull/564) change gRPC path to original cosmos path

diff --git a/client/keys/add.go b/client/keys/add.go
@@ -8,7 +8,7 @@ import (
 	"math"
 	"sort"
 
-	bip39 "github.com/cosmos/go-bip39"
+	"github.com/cosmos/go-bip39"
 	"github.com/spf13/cobra"
 
 	"github.com/line/lbm-sdk/client"
@@ -95,15 +95,17 @@ func runAddCmdPrepare(cmd *cobra.Command, args []string) error {
 
 /*
 input
-	- bip39 mnemonic
-	- bip39 passphrase
-	- bip44 path
-	- local encryption password
+  - bip39 mnemonic
+  - bip39 passphrase
+  - bip44 path
+  - local encryption password
+
 output
-	- armor encrypted private key (saved to file)
+  - armor encrypted private key (saved to file)
 */
 func runAddCmd(ctx client.Context, cmd *cobra.Command, args []string, inBuf *bufio.Reader) error {
 	var err error
+	var multisigThreshold int
 
 	name := args[0]
 	interactive, _ := cmd.Flags().GetBool(flagInteractive)
@@ -118,6 +120,18 @@ func runAddCmd(ctx client.Context, cmd *cobra.Command, args []string, inBuf *buf
 	if err != nil {
 		return err
 	}
+	multisigKeys, _ := cmd.Flags().GetStringSlice(flagMultisig)
+	if len(multisigKeys) != 0 {
+		multisigThreshold, _ = cmd.Flags().GetInt(flagMultiSigThreshold)
+		if err = validateMultisigThreshold(multisigThreshold, len(multisigKeys)); err != nil {
+			return err
+		}
+
+		err = verifyMultisigTarget(kb, multisigKeys, name)
+		if err != nil {
+			return err
+		}
+	}
 
 	if dryRun, _ := cmd.Flags().GetBool(flags.FlagDryRun); dryRun {
 		// use in memory keybase
@@ -141,13 +155,8 @@ func runAddCmd(ctx client.Context, cmd *cobra.Command, args []string, inBuf *buf
 			}
 		}
 
-		multisigKeys, _ := cmd.Flags().GetStringSlice(flagMultisig)
 		if len(multisigKeys) != 0 {
 			pks := make([]cryptotypes.PubKey, len(multisigKeys))
-			multisigThreshold, _ := cmd.Flags().GetInt(flagMultiSigThreshold)
-			if err := validateMultisigThreshold(multisigThreshold, len(multisigKeys)); err != nil {
-				return err
-			}
 
 			for i, keyname := range multisigKeys {
 				k, err := kb.Key(keyname)
@@ -327,3 +336,17 @@ func printCreate(cmd *cobra.Command, info keyring.Info, showMnemonic bool, mnemo
 
 	return nil
 }
+
+func verifyMultisigTarget(kb keyring.Keyring, multisigKeys []string, newkey string) error {
+	if _, err := kb.Key(newkey); err == nil {
+		return errors.New("you cannot specify a new key as one of the names of the keys that make up a multisig")
+	}
+
+	for _, k := range multisigKeys {
+		if _, err := kb.Key(k); err != nil {
+			return errors.New("part of the multisig target key does not exist")
+		}
+	}
+
+	return nil
+}
diff --git a/client/keys/add_test.go b/client/keys/add_test.go
@@ -7,7 +7,8 @@ import (
 	"io"
 	"testing"
 
-	bip39 "github.com/cosmos/go-bip39"
+	"github.com/cosmos/go-bip39"
+	"github.com/spf13/pflag"
 	"github.com/stretchr/testify/require"
 
 	"github.com/line/ostracon/libs/cli"
@@ -32,8 +33,9 @@ func Test_runAddCmdBasic(t *testing.T) {
 	kb, err := keyring.New(sdk.KeyringServiceName(), keyring.BackendTest, kbHome, mockIn)
 	require.NoError(t, err)
 
-	clientCtx := client.Context{}.WithKeyringDir(kbHome).WithInput(mockIn)
-	ctx := context.WithValue(context.Background(), client.ClientContextKey, &clientCtx)
+	clientCtx := client.Context{}.WithKeyringDir(kbHome).WithInput(mockIn).WithKeyring(kb)
+	clientCtxPtr := &clientCtx
+	ctx := context.WithValue(context.Background(), client.ClientContextKey, clientCtxPtr)
 
 	t.Cleanup(func() {
 		_ = kb.Delete("keyname1")
@@ -62,7 +64,6 @@ func Test_runAddCmdBasic(t *testing.T) {
 	})
 
 	require.NoError(t, cmd.ExecuteContext(ctx))
-	require.Error(t, cmd.ExecuteContext(ctx))
 
 	mockIn.Reset("y\n")
 	require.NoError(t, cmd.ExecuteContext(ctx))
@@ -76,7 +77,81 @@ func Test_runAddCmdBasic(t *testing.T) {
 	})
 
 	require.NoError(t, cmd.ExecuteContext(ctx))
-	require.Error(t, cmd.ExecuteContext(ctx))
+
+	// In Multisig
+	tcs := []struct {
+		args []string
+		err  string
+	}{
+		{[]string{
+			"keyname1",
+			fmt.Sprintf("--%s=%s", flags.FlagHome, kbHome),
+			fmt.Sprintf("--%s=%s", cli.OutputFlag, OutputFormatText),
+			fmt.Sprintf("--%s=%s", flags.FlagKeyAlgorithm, string(hd.Secp256k1Type)),
+			fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest),
+			fmt.Sprintf("--%s=%s", flagMultisig, "keyname1,keyname2"),
+		},
+			"you cannot specify a new key as one of the names of the keys that make up a multisig",
+		},
+		{[]string{
+			"keyname-multi",
+			fmt.Sprintf("--%s=%s", flags.FlagHome, kbHome),
+			fmt.Sprintf("--%s=%s", cli.OutputFlag, OutputFormatText),
+			fmt.Sprintf("--%s=%s", flags.FlagKeyAlgorithm, string(hd.Secp256k1Type)),
+			fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest),
+			fmt.Sprintf("--%s=%s", flagMultisig, "keyname1,keyname11"),
+		},
+			"part of the multisig target key does not exist",
+		},
+		{[]string{
+			"keyname-multi",
+			fmt.Sprintf("--%s=%s", flags.FlagHome, kbHome),
+			fmt.Sprintf("--%s=%s", cli.OutputFlag, OutputFormatText),
+			fmt.Sprintf("--%s=%s", flags.FlagKeyAlgorithm, string(hd.Secp256k1Type)),
+			fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest),
+			fmt.Sprintf("--%s=%s", flagMultisig, "keyname1,keyname2"),
+			fmt.Sprintf("--%s=%d", flagMultiSigThreshold, 3),
+		},
+			"threshold k of n multisignature",
+		},
+		{[]string{
+			"keyname-multi",
+			fmt.Sprintf("--%s=%s", flags.FlagHome, kbHome),
+			fmt.Sprintf("--%s=%s", cli.OutputFlag, OutputFormatText),
+			fmt.Sprintf("--%s=%s", flags.FlagKeyAlgorithm, string(hd.Secp256k1Type)),
+			fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest),
+			fmt.Sprintf("--%s=%s", flagMultisig, "keyname1,keyname2"),
+			fmt.Sprintf("--%s=%d", flagMultiSigThreshold, -1),
+		},
+			"threshold must be a positive integer",
+		},
+		{[]string{
+			"keyname-multi",
+			fmt.Sprintf("--%s=%s", flags.FlagHome, kbHome),
+			fmt.Sprintf("--%s=%s", cli.OutputFlag, OutputFormatText),
+			fmt.Sprintf("--%s=%s", flags.FlagKeyAlgorithm, string(hd.Secp256k1Type)),
+			fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest),
+			fmt.Sprintf("--%s=%s", flagMultisig, "keyname1,keyname2"),
+			fmt.Sprintf("--%s=%d", flagMultiSigThreshold, 2),
+		},
+			"",
+		},
+	}
+
+	for _, tc := range tcs {
+		cmd.SetArgs(tc.args)
+		if tc.err != "" {
+			require.Contains(t, cmd.ExecuteContext(ctx).Error(), tc.err)
+		} else {
+			require.NoError(t, cmd.ExecuteContext(ctx))
+		}
+
+		cmd.Flags().Visit(func(f *pflag.Flag) {
+			if f.Name == flagMultisig {
+				f.Value.(pflag.SliceValue).Replace([]string{})
+			}
+		})
+	}
 
 	cmd.SetArgs([]string{
 		"keyname5",
@@ -85,7 +160,7 @@ func Test_runAddCmdBasic(t *testing.T) {
 		fmt.Sprintf("--%s=%s", cli.OutputFlag, OutputFormatText),
 		fmt.Sprintf("--%s=%s", flags.FlagKeyAlgorithm, string(hd.Secp256k1Type)),
 	})
-
+	mockIn.Reset("\n")
 	require.NoError(t, cmd.ExecuteContext(ctx))
 
 	// In recovery mode