From 98b84e11548e7267e2440fd6f582a9e8fcdfe970 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20Adomnic=C4=83i?= Date: Mon, 9 Sep 2024 11:40:08 +0200 Subject: [PATCH] cmd: hardening threshold parameter checks (#3242) Ensure the threshold parameter given as input to the `create dkg` and `create cluster` commands is sound. category: bug ticket: #3241 --- cmd/createcluster.go | 18 ++++++++++++ cmd/createcluster_internal_test.go | 47 +++++++++++++++++++++++++++++- cmd/createdkg.go | 16 ++++++---- cmd/createdkg_internal_test.go | 34 ++++++++++++++------- 4 files changed, 98 insertions(+), 17 deletions(-) diff --git a/cmd/createcluster.go b/cmd/createcluster.go index 06827883a..9c1599c0c 100644 --- a/cmd/createcluster.go +++ b/cmd/createcluster.go @@ -10,6 +10,7 @@ import ( "encoding/json" "fmt" "io" + "math" "net/url" "os" "path" @@ -51,6 +52,7 @@ const ( zeroAddress = "0x0000000000000000000000000000000000000000" defaultNetwork = "mainnet" minNodes = 3 + minThreshold = 2 ) type clusterConfig struct { @@ -144,6 +146,7 @@ func runCreateCluster(ctx context.Context, w io.Writer, conf clusterConfig) erro } conf.NumNodes = len(def.Operators) + conf.Threshold = def.Threshold } if err = validateCreateConfig(ctx, conf); err != nil { @@ -366,6 +369,21 @@ func validateCreateConfig(ctx context.Context, conf clusterConfig) error { } } + // Don't allow cluster size to be less than 3. + if conf.NumNodes < minNodes { + return errors.New("number of operators is below minimum", z.Int("operators", conf.NumNodes), z.Int("min", minNodes)) + } + + // Check for threshold parameter + minThreshold := int(math.Ceil(float64(conf.NumNodes*2) / 3)) + if conf.Threshold < minThreshold { + return errors.New("threshold cannot be smaller than BFT quorum", z.Int("threshold", conf.Threshold), z.Int("min", minThreshold)) + } + if conf.Threshold > conf.NumNodes { + return errors.New("threshold cannot be greater than number of operators", + z.Int("threshold", conf.Threshold), z.Int("operators", conf.NumNodes)) + } + return nil } diff --git a/cmd/createcluster_internal_test.go b/cmd/createcluster_internal_test.go index 0d066474f..95be0bad4 100644 --- a/cmd/createcluster_internal_test.go +++ b/cmd/createcluster_internal_test.go @@ -39,6 +39,10 @@ func TestCreateCluster(t *testing.T) { def, err := loadDefinition(context.Background(), defPath) require.NoError(t, err) + defPathTwoNodes := "../cluster/examples/cluster-definition-001.json" + defTwoNodes, err := loadDefinition(context.Background(), defPathTwoNodes) + require.NoError(t, err) + tests := []struct { Name string Config clusterConfig @@ -218,7 +222,7 @@ func TestCreateCluster(t *testing.T) { Config: clusterConfig{ Name: "test_cluster", NumNodes: 3, - Threshold: 4, + Threshold: 3, NumDVs: 5, Network: "goerli", }, @@ -246,6 +250,43 @@ func TestCreateCluster(t *testing.T) { }, }, }, + { + Name: "threshold greater than the number of operators", + Config: clusterConfig{ + NumNodes: 4, + Threshold: 5, + NumDVs: 1, + Network: defaultNetwork, + }, + expectedErr: "threshold cannot be greater than number of operators", + }, + { + Name: "threshold smaller than BFT quorum", + Config: clusterConfig{ + NumNodes: 4, + Threshold: 2, + NumDVs: 1, + Network: defaultNetwork, + }, + expectedErr: "threshold cannot be smaller than BFT quorum", + }, + { + Name: "test with number of nodes below minimum", + Config: clusterConfig{ + Name: "test_cluster", + NumNodes: 2, + Threshold: 2, + NumDVs: 1, + Network: "goerli", + }, + defFileProvider: func() []byte { + data, err := json.Marshal(defTwoNodes) + require.NoError(t, err) + + return data + }, + expectedErr: "number of operators is below minimum", + }, } for _, test := range tests { t.Run(test.Name, func(t *testing.T) { @@ -555,6 +596,7 @@ func TestMultipleAddresses(t *testing.T) { err := runCreateCluster(context.Background(), io.Discard, clusterConfig{ NumDVs: 4, NumNodes: 4, + Threshold: 3, Network: defaultNetwork, FeeRecipientAddrs: []string{}, WithdrawalAddrs: []string{}, @@ -566,6 +608,7 @@ func TestMultipleAddresses(t *testing.T) { err := runCreateCluster(context.Background(), io.Discard, clusterConfig{ NumDVs: 1, NumNodes: 4, + Threshold: 3, Network: defaultNetwork, FeeRecipientAddrs: []string{feeRecipientAddr}, WithdrawalAddrs: []string{}, @@ -639,6 +682,7 @@ func TestKeymanager(t *testing.T) { SplitKeysDir: keyDir, SplitKeys: true, NumNodes: minNodes, + Threshold: minThreshold, KeymanagerAddrs: addrs, KeymanagerAuthTokens: authTokens, Network: eth2util.Goerli.Name, @@ -720,6 +764,7 @@ func TestPublish(t *testing.T) { conf := clusterConfig{ Name: t.Name(), NumNodes: minNodes, + Threshold: minThreshold, NumDVs: 1, Network: eth2util.Goerli.Name, WithdrawalAddrs: []string{zeroAddress}, diff --git a/cmd/createdkg.go b/cmd/createdkg.go index 765300e95..e0809c0aa 100644 --- a/cmd/createdkg.go +++ b/cmd/createdkg.go @@ -6,6 +6,7 @@ import ( "context" crand "crypto/rand" "encoding/json" + "math" "os" "path" @@ -181,16 +182,21 @@ func validateWithdrawalAddrs(addrs []string, network string) error { // validateDKGConfig returns an error if any of the provided config parameter is invalid. func validateDKGConfig(threshold, numOperators int, network string, depositAmounts []int) error { + // Don't allow cluster size to be less than 3. + if numOperators < minNodes { + return errors.New("number of operators is below minimum", z.Int("operators", numOperators), z.Int("min", minNodes)) + } + + // Ensure threshold setting is sound + minThreshold := int(math.Ceil(float64(numOperators*2) / 3)) + if threshold < minThreshold { + return errors.New("threshold cannot be smaller than BFT quorum", z.Int("threshold", threshold), z.Int("min", minThreshold)) + } if threshold > numOperators { return errors.New("threshold cannot be greater than length of operators", z.Int("threshold", threshold), z.Int("operators", numOperators)) } - // Don't allow cluster size to be less than 4. - if numOperators < minNodes { - return errors.New("insufficient operator ENRs", z.Int("count", numOperators), z.Int("min", minNodes)) - } - if !eth2util.ValidNetwork(network) { return errors.New("unsupported network", z.Str("network", network)) } diff --git a/cmd/createdkg_internal_test.go b/cmd/createdkg_internal_test.go index fbf53da4e..a6d522db5 100644 --- a/cmd/createdkg_internal_test.go +++ b/cmd/createdkg_internal_test.go @@ -57,7 +57,8 @@ func TestCreateDkgInvalid(t *testing.T) { OperatorENRs: append([]string{ "-JG4QDKNYm_JK-w6NuRcUFKvJAlq2L4CwkECelzyCVrMWji4YnVRn8AqQEL5fTQotPL2MKxiKNmn2k6XEINtq-6O3Z2GAYGvzr_LgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQKlO7fSaBa3h48CdM-qb_Xb2_hSrJOy6nNjR0mapAqMboN0Y3CCDhqDdWRwgg4u", }, validENRs...), - Network: defaultNetwork, + Threshold: 3, + Network: defaultNetwork, }, errMsg: "invalid ENR: missing 'enr:' prefix", }, @@ -66,7 +67,8 @@ func TestCreateDkgInvalid(t *testing.T) { OperatorENRs: append([]string{ "enr:JG4QDKNYm_JK-w6NuRcUFKvJAlq2L4CwkECelzyCVrMWji4YnVRn8AqQEL5fTQotPL2MKxiKNmn2k6XEINtq-6O3Z2GAYGvzr_LgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQKlO7fSaBa3h48CdM-qb_Xb2_hSrJOy6nNjR0mapAqMboN0Y3CCDhqDdWRwgg4u", }, validENRs...), - Network: defaultNetwork, + Threshold: 3, + Network: defaultNetwork, }, errMsg: "invalid ENR: invalid enr record, too few elements", }, @@ -75,7 +77,8 @@ func TestCreateDkgInvalid(t *testing.T) { OperatorENRs: append([]string{ "enrJG4QDKNYm_JK-w6NuRcUFKvJAlq2L4CwkECelzyCVrMWji4YnVRn8AqQEL5fTQotPL2MKxiKNmn2k6XEINtq-6O3Z2GAYGvzr_LgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQKlO7fSaBa3h48CdM-qb_Xb2_hSrJOy6nNjR0mapAqMboN0Y3CCDhqDdWRwgg4u", }, validENRs...), - Network: defaultNetwork, + Threshold: 3, + Network: defaultNetwork, }, errMsg: "invalid ENR: missing 'enr:' prefix", }, @@ -84,17 +87,18 @@ func TestCreateDkgInvalid(t *testing.T) { OperatorENRs: append([]string{ "JG4QDKNYm_JK-w6NuRcUFKvJAlq2L4CwkECelzyCVrMWji4YnVRn8AqQEL5fTQotPL2MKxiKNmn2k6XEINtq-6O3Z2GAYGvzr_LgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQKlO7fSaBa3h48CdM-qb_Xb2_hSrJOy6nNjR0mapAqMboN0Y3CCDhqDdWRwgg4u", }, validENRs...), - Network: defaultNetwork, + Threshold: 3, + Network: defaultNetwork, }, errMsg: "invalid ENR: missing 'enr:' prefix", }, { conf: createDKGConfig{OperatorENRs: []string{""}}, - errMsg: "insufficient operator ENRs", + errMsg: "number of operators is below minimum", }, { conf: createDKGConfig{}, - errMsg: "insufficient operator ENRs", + errMsg: "number of operators is below minimum", }, } @@ -120,7 +124,7 @@ func TestRequireOperatorENRFlag(t *testing.T) { { name: "operator ENRs less than threshold", args: []string{"dkg", "--operator-enrs=enr:-JG4QG472ZVvl8ySSnUK9uNVDrP_hjkUrUqIxUC75aayzmDVQedXkjbqc7QKyOOS71VmlqnYzri_taV8ZesFYaoQSIOGAYHtv1WsgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQKwwq_CAld6oVKOrixE-JzMtvvNgb9yyI-_rwq4NFtajIN0Y3CCDhqDdWRwgg4u", "--fee-recipient-addresses=0xa6430105220d0b29688b734b8ea0f3ca9936e846", "--withdrawal-addresses=0xa6430105220d0b29688b734b8ea0f3ca9936e846"}, - err: "insufficient operator ENRs", + err: "number of operators is below minimum", }, } @@ -146,9 +150,10 @@ func TestExistingClusterDefinition(t *testing.T) { feeRecipientArg := "--fee-recipient-addresses=" + validEthAddr withdrawalArg := "--withdrawal-addresses=" + validEthAddr outputDirArg := "--output-dir=" + charonDir + thresholdArg := "--threshold=2" cmd := newCreateCmd(newCreateDKGCmd(runCreateDKG)) - cmd.SetArgs([]string{"dkg", enrArg, feeRecipientArg, withdrawalArg, outputDirArg}) + cmd.SetArgs([]string{"dkg", enrArg, feeRecipientArg, withdrawalArg, outputDirArg, thresholdArg}) require.EqualError(t, cmd.Execute(), "existing cluster-definition.json found. Try again after deleting it") } @@ -179,18 +184,25 @@ func TestValidateWithdrawalAddr(t *testing.T) { } func TestValidateDKGConfig(t *testing.T) { - t.Run("invalid threshold", func(t *testing.T) { + t.Run("threshold exceeds numOperators", func(t *testing.T) { threshold := 5 numOperators := 4 err := validateDKGConfig(threshold, numOperators, "", nil) require.ErrorContains(t, err, "threshold cannot be greater than length of operators") }) - t.Run("insufficient ENRs", func(t *testing.T) { + t.Run("threshold equals 1", func(t *testing.T) { threshold := 1 + numOperators := 3 + err := validateDKGConfig(threshold, numOperators, "", nil) + require.ErrorContains(t, err, "threshold cannot be smaller than BFT quorum") + }) + + t.Run("insufficient ENRs", func(t *testing.T) { + threshold := 2 numOperators := 2 err := validateDKGConfig(threshold, numOperators, "", nil) - require.ErrorContains(t, err, "insufficient operator ENRs") + require.ErrorContains(t, err, "number of operators is below minimum") }) t.Run("invalid network", func(t *testing.T) {