Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mark invalid encrypted shares as 'invalid' #1151

Closed
wants to merge 10 commits into from
101 changes: 86 additions & 15 deletions eth/eventhandler/event_handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,7 @@ func TestHandleBlockEventsStream(t *testing.T) {
// Corrupt the encrypted last share key of the 4th operator
malformedSharesData[len(malformedSharesData)-1] ^= 1

valPubKey := validatorData2.masterPubKey.Serialize()
// Call the contract method
_, err = boundContract.SimcontractTransactor.RegisterValidator(
auth,
Expand Down Expand Up @@ -454,30 +455,39 @@ func TestHandleBlockEventsStream(t *testing.T) {
// Check that validator was registered for op1,
shares = eh.nodeStorage.Shares().List(nil)
require.Equal(t, 2, len(shares))
valShare := eh.nodeStorage.Shares().Get(nil, valPubKey)
require.NotNil(t, valShare)
require.False(t, valShare.Invalid)
// and nonce was bumped
nonce, err = eh.nodeStorage.GetNextNonce(nil, testAddr)
require.NoError(t, err)
require.Equal(t, registrystorage.Nonce(3), nonce)
})

// Share for 1st operator is malformed; check nonce is bumped correctly; validator wasn't added
// slashing protection data is not added
t.Run("test malformed ValidatorAdded and nonce is bumped", func(t *testing.T) {
malformedSharesData := sharesData3[:]
t.Run("test malformed ValidatorAdded, nonce is bumped, share is saved, marked as invalid", func(t *testing.T) {
validatorData, err := createNewValidator(ops)
require.NoError(t, err)

sharesData, err := generateSharesData(validatorData, ops, testAddr, 3)
require.NoError(t, err)
incorrectSharesData, err := generateSharesDataWrong(validatorData, ops, testAddr, 4)
require.NoError(t, err)

operatorCount := len(ops)
signatureOffset := phase0.SignatureLength
pubKeysOffset := phase0.PublicKeyLength*operatorCount + signatureOffset

// Corrupt the encrypted share key of the operator 1
malformedSharesData[pubKeysOffset+encryptedKeyLength-1] ^= 1
for i := 0; i < signatureOffset; i++ {
incorrectSharesData[i] = sharesData[i]
}

require.NoError(t, err)

valPubKey := validatorData.masterPubKey.Serialize()
// Call the contract method
_, err = boundContract.SimcontractTransactor.RegisterValidator(
auth,
validatorData3.masterPubKey.Serialize(),
valPubKey,
[]uint64{1, 2, 3, 4},
malformedSharesData,
incorrectSharesData,
big.NewInt(100_000_000),
simcontract.CallableCluster{
ValidatorCount: 1,
Expand All @@ -504,11 +514,15 @@ func TestHandleBlockEventsStream(t *testing.T) {
require.Equal(t, blockNum+1, lastProcessedBlock)
blockNum++

requireKeyManagerDataToNotExist(t, eh, 2, validatorData3)
// TODO. Fix the behavior of event_handler.go
requireKeyManagerDataToNotExist(t, eh, 2, validatorData)

// Check that validator was not registered
shares = eh.nodeStorage.Shares().List(nil)
require.Equal(t, 2, len(shares))
require.Equal(t, 3, len(shares))
valShare := eh.nodeStorage.Shares().Get(nil, valPubKey)
require.NotNil(t, valShare)
require.True(t, valShare.Invalid)
// and nonce was bumped
nonce, err = eh.nodeStorage.GetNextNonce(nil, testAddr)
require.NoError(t, err)
Expand Down Expand Up @@ -557,7 +571,7 @@ func TestHandleBlockEventsStream(t *testing.T) {

// Check that validator was registered
shares = eh.nodeStorage.Shares().List(nil)
require.Equal(t, 3, len(shares))
require.Equal(t, 4, len(shares))
// and nonce was bumped
nonce, err = eh.nodeStorage.GetNextNonce(nil, testAddr)
require.NoError(t, err)
Expand Down Expand Up @@ -607,7 +621,7 @@ func TestHandleBlockEventsStream(t *testing.T) {

// Check that validator was registered
shares = eh.nodeStorage.Shares().List(nil)
require.Equal(t, 4, len(shares))
require.Equal(t, 5, len(shares))
// and nonce was bumped
nonce, err = eh.nodeStorage.GetNextNonce(nil, testAddr2)
require.NoError(t, err)
Expand Down Expand Up @@ -736,7 +750,7 @@ func TestHandleBlockEventsStream(t *testing.T) {

// Check the validator was removed from the validator shares storage.
shares := eh.nodeStorage.Shares().List(nil)
require.Equal(t, 3, len(shares))
require.Equal(t, 4, len(shares))
valShare = eh.nodeStorage.Shares().Get(nil, valPubKey)
require.Nil(t, valShare)
requireKeyManagerDataToNotExist(t, eh, 3, validatorData1)
Expand Down Expand Up @@ -1464,7 +1478,64 @@ func generateSharesData(validatorData *testValidatorData, operators []*testOpera

pubkeys = append(pubkeys, validatorData.operatorsShares[i].pub.Serialize()...)
encryptedShares = append(encryptedShares, ciphertext...)
}

tosign := fmt.Sprintf("%s:%d", owner.String(), nonce)
msghash := crypto.Keccak256([]byte(tosign))
signed := validatorData.masterKey.Sign(string(msghash))
sig := signed.Serialize()

if !signed.VerifyByte(validatorData.masterPubKey, msghash) {
return nil, errors.New("couldn't sign correctly")
}

sharesData := append(pubkeys, encryptedShares...)
sharesDataSigned := append(sig, sharesData...)

return sharesDataSigned, nil
}

func generateSharesDataWrong(validatorData *testValidatorData, operators []*testOperator, owner ethcommon.Address, nonce int) ([]byte, error) {
AKorpusenko marked this conversation as resolved.
Show resolved Hide resolved
var pubkeys []byte
var encryptedShares []byte

for i, op := range operators {
pubKey := op.pub[:]
privKey := op.priv[:]

if i == 0 {
pb, sk, _ := rsaencryption.GenerateKeys()
pubKey = pb
privKey = sk
}
rsakey, err := rsaencryption.ConvertPemToPublicKey(pubKey)
if err != nil {
return nil, fmt.Errorf("cant convert publickey: %w", err)
}

rawshare := validatorData.operatorsShares[i].sec.SerializeToHexStr()
ciphertext, err := rsa.EncryptPKCS1v15(rand.Reader, rsakey, []byte(rawshare))
if err != nil {
return nil, errors.New("cant encrypt share")
}

rsapriv, err := rsaencryption.ConvertPemToPrivateKey(string(privKey))
if err != nil {
return nil, err
}

// check that we encrypt right
shareSecret := &bls.SecretKey{}
decryptedSharePrivateKey, err := rsaencryption.DecodeKey(rsapriv, ciphertext)
if err != nil {
return nil, err
}
if err = shareSecret.SetHexString(string(decryptedSharePrivateKey)); err != nil {
return nil, err
}

pubkeys = append(pubkeys, validatorData.operatorsShares[i].pub.Serialize()...)
encryptedShares = append(encryptedShares, ciphertext...)
}

tosign := fmt.Sprintf("%s:%d", owner.String(), nonce)
Expand Down
43 changes: 30 additions & 13 deletions eth/eventhandler/handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,12 +179,12 @@
}

validatorShare := eh.nodeStorage.Shares().Get(txn, event.PublicKey)
var malformedEventError *MalformedEventError

if validatorShare == nil {
createdShare, err := eh.handleShareCreation(txn, event, sharePublicKeys, encryptedKeys)

Check failure on line 185 in eth/eventhandler/handlers.go

View workflow job for this annotation

GitHub Actions / lint

SA4023(related information): the lhs of the comparison is the 2nd return value of this function call (staticcheck)
if err != nil {

Check failure on line 186 in eth/eventhandler/handlers.go

View workflow job for this annotation

GitHub Actions / lint

SA4023: this comparison is always true (staticcheck)
var malformedEventError *MalformedEventError
if errors.As(err, &malformedEventError) {
if errors.As(err, &malformedEventError) && malformedEventError != nil && !malformedEventError.IsInvalidEncryptedShare {
logger.Warn("malformed event", zap.Error(err))

return nil, err
Expand All @@ -209,7 +209,7 @@
}

isOperatorShare := validatorShare.BelongsToOperator(eh.operatorData.GetOperatorData().ID)
if isOperatorShare {
if isOperatorShare && !validatorShare.Invalid {
eh.metrics.ValidatorInactive(event.PublicKey)
ownShare = validatorShare
logger = logger.With(zap.Bool("own_validator", isOperatorShare))
Expand All @@ -220,7 +220,7 @@
}

// handleShareCreation is called when a validator was added/updated during registry sync
func (eh *EventHandler) handleShareCreation(

Check failure on line 223 in eth/eventhandler/handlers.go

View workflow job for this annotation

GitHub Actions / lint

SA4023(related information): (*github.com/bloxapp/ssv/eth/eventhandler.EventHandler).handleShareCreation never returns a nil interface value (staticcheck)
txn basedb.Txn,
validatorEvent *contract.ContractValidatorAdded,
sharePublicKeys [][]byte,
Expand All @@ -233,11 +233,23 @@
sharePublicKeys,
encryptedKeys,
)

var malformedEventError *MalformedEventError = nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

= nil is redundant

if err != nil {
return nil, fmt.Errorf("could not extract validator share from event: %w", err)
switch {
default:
return nil, fmt.Errorf("could not extract validator share from event: %w", err)

case errors.As(err, &malformedEventError):
if malformedEventError.IsInvalidEncryptedShare &&
share.BelongsToOperator(eh.operatorData.GetOperatorData().ID) {

share.Metadata.Invalid = true
}
}
}

if share.BelongsToOperator(eh.operatorData.GetOperatorData().ID) {
if malformedEventError == nil && share.BelongsToOperator(eh.operatorData.GetOperatorData().ID) {
if shareSecret == nil {
return nil, errors.New("could not decode shareSecret")
}
Expand All @@ -253,7 +265,7 @@
return nil, fmt.Errorf("could not save validator share: %w", err)
}

return share, nil
return share, malformedEventError
}

func validatorAddedEventToShare(
Expand Down Expand Up @@ -301,18 +313,21 @@
shareSecret = &bls.SecretKey{}
decryptedSharePrivateKey, err := rsaencryption.DecodeKey(operatorPrivateKey, encryptedKeys[i])
if err != nil {
return nil, nil, &MalformedEventError{
Err: fmt.Errorf("could not decrypt share private key: %w", err),
return &validatorShare, nil, &MalformedEventError{
Err: fmt.Errorf("could not decrypt share private key: %w", err),
IsInvalidEncryptedShare: true,
}
}
if err = shareSecret.SetHexString(string(decryptedSharePrivateKey)); err != nil {
return nil, nil, &MalformedEventError{
Err: fmt.Errorf("could not set decrypted share private key: %w", err),
return &validatorShare, nil, &MalformedEventError{
Err: fmt.Errorf("could not set decrypted share private key: %w", err),
IsInvalidEncryptedShare: true,
}
}
if !bytes.Equal(shareSecret.GetPublicKey().Serialize(), validatorShare.SharePubKey) {
return nil, nil, &MalformedEventError{
Err: errors.New("share private key does not match public key"),
return &validatorShare, nil, &MalformedEventError{
Err: fmt.Errorf("share private key does not match public key"),
IsInvalidEncryptedShare: true,
}
Comment on lines +318 to 333
Copy link
Contributor

@moshe-blox moshe-blox Feb 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please try to instead of returning malformed error, instead to just set the Invalid flag on the share and return no error?

with this you'd be able to just ask if the share is invalid rather than if there was a malformedError

}
}
Expand All @@ -321,6 +336,7 @@
validatorShare.DomainType = ssvtypes.GetDefaultDomain()
validatorShare.Committee = committee
validatorShare.Graffiti = []byte("ssv.network")
validatorShare.Metadata.Invalid = false

return &validatorShare, shareSecret, nil
}
Expand Down Expand Up @@ -520,7 +536,8 @@

// MalformedEventError is returned when event is malformed
type MalformedEventError struct {
Err error
Err error
IsInvalidEncryptedShare bool
}

func (e *MalformedEventError) Error() string {
Expand Down
1 change: 1 addition & 0 deletions protocol/v2/types/ssvshare.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,4 +103,5 @@ type Metadata struct {
BeaconMetadata *beaconprotocol.ValidatorMetadata
OwnerAddress common.Address
Liquidated bool
Invalid bool
Copy link
Contributor

@moshe-blox moshe-blox Feb 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can rename this to InvalidSecret so its more self-explanatory?

otherwise one might assume that something else in the event may have been invalid (such as invalid nonce)

}
Loading