Skip to content

Commit

Permalink
cmd: hardening threshold parameter checks (#3242)
Browse files Browse the repository at this point in the history 
Ensure the threshold parameter given as input to the `create dkg` and `create cluster` commands is sound.

category: bug
ticket: #3241
  • Loading branch information
@aadomn
aadomn authored Sep 9, 2024
1 parent 2545dd7 commit 98b84e1
Show file tree
Hide file tree
Showing 4 changed files with 98 additions and 17 deletions.
18 changes: 18 additions & 0 deletions cmd/createcluster.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"encoding/json"
"fmt"
"io"
"math"
"net/url"
"os"
"path"
Expand Down Expand Up @@ -51,6 +52,7 @@ const (
zeroAddress = "0x0000000000000000000000000000000000000000"
defaultNetwork = "mainnet"
minNodes = 3
minThreshold = 2
)

type clusterConfig struct {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}

Expand Down
47 changes: 46 additions & 1 deletion cmd/createcluster_internal_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -218,7 +222,7 @@ func TestCreateCluster(t *testing.T) {
Config: clusterConfig{
Name: "test_cluster",
NumNodes: 3,
Threshold: 4,
Threshold: 3,
NumDVs: 5,
Network: "goerli",
},
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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{},
Expand All @@ -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{},
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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},
Expand Down
16 changes: 11 additions & 5 deletions cmd/createdkg.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"context"
crand "crypto/rand"
"encoding/json"
"math"
"os"
"path"

Expand Down Expand Up @@ -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))
}
Expand Down
34 changes: 23 additions & 11 deletions cmd/createdkg_internal_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
},
Expand All @@ -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",
},
Expand All @@ -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",
},
Expand All @@ -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",
},
}

Expand All @@ -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",
},
}

Expand All @@ -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")
}
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit 98b84e1

Please sign in to comment.