Improve Round-Change validation

qbft/instance.go

@@ -177,7 +177,7 @@ func (i *Instance) BaseMsgValidation(msg *SignedMessage) error {
 			i.State.Share.Committee,
 		)
 	case RoundChangeMsgType:
-		return validRoundChangeForData(i.State, i.config, msg, i.State.Height, msg.Message.Round, msg.FullData)
+		return validSignedRoundChangeForData(i.State, i.config, msg, i.State.Height, msg.Message.Round, msg.FullData)
 	default:
 		return errors.New("signed message type not supported")
 	}

qbft/prepare.go

@@ -2,6 +2,7 @@ package qbft
 
 import (
 	"bytes"
+
 	"github.com/bloxapp/ssv-spec/types"
 	"github.com/pkg/errors"
 )
@@ -46,6 +47,7 @@ func (i *Instance) uponPrepare(signedPrepare *SignedMessage, prepareMsgContainer
 
 // getRoundChangeJustification returns the round change justification for the current round.
 // The justification is a quorum of signed prepare messages that agree on state.LastPreparedValue
+// It assumes that the prepareMsgContainer has only messages with valid signatures
 func getRoundChangeJustification(state *State, config IConfig, prepareMsgContainer *MsgContainer) ([]*SignedMessage, error) {
 	if state.LastPreparedValue == nil {
 		return nil, nil
@@ -59,7 +61,9 @@ func getRoundChangeJustification(state *State, config IConfig, prepareMsgContain
 	prepareMsgs := prepareMsgContainer.MessagesForRound(state.LastPreparedRound)
 	ret := make([]*SignedMessage, 0)
 	for _, msg := range prepareMsgs {
-		if err := validSignedPrepareForHeightRoundAndRoot(
+		// Calls validPrepareForHeightRoundAndRoot instead of validSignedPrepareForHeightRoundAndRoot
+		// since signatures are assumed to be valid
+		if err := validPrepareForHeightRoundAndRoot(
 			config,
 			msg,
 			state.Height,
@@ -86,6 +90,28 @@ func validSignedPrepareForHeightRoundAndRoot(
 	round Round,
 	root [32]byte,
 	operators []*types.Operator) error {
+
+	if err := validPrepareForHeightRoundAndRoot(config, signedPrepare, height, round, root, operators); err != nil {
+		return err
+	}
+
+	if err := signedPrepare.Signature.VerifyByOperators(signedPrepare, config.GetSignatureDomainType(), types.QBFTSignatureType, operators); err != nil {
+		return errors.Wrap(err, "msg signature invalid")
+	}
+
+	return nil
+}
+
+// validPrepareForHeightRoundAndRoot verifies if a Prepare message is prepared for a certain round and root
+// similar to the dafny's validSignedPrepareForHeightRoundAndDigest predicate.
+// However, it doesn't verify the message signature
+func validPrepareForHeightRoundAndRoot(
+	config IConfig,
+	signedPrepare *SignedMessage,
+	height Height,
+	round Round,
+	root [32]byte,
+	operators []*types.Operator) error {
 	if signedPrepare.Message.MsgType != PrepareMsgType {
 		return errors.New("prepare msg type is wrong")
 	}
@@ -108,10 +134,6 @@ func validSignedPrepareForHeightRoundAndRoot(
 		return errors.New("msg allows 1 signer")
 	}
 
-	if err := signedPrepare.Signature.VerifyByOperators(signedPrepare, config.GetSignatureDomainType(), types.QBFTSignatureType, operators); err != nil {
-		return errors.Wrap(err, "msg signature invalid")
-	}
-
 	return nil
 }
 

qbft/proposal.go

@@ -2,6 +2,7 @@ package qbft
 
 import (
 	"bytes"
+
 	"github.com/bloxapp/ssv-spec/types"
 	"github.com/pkg/errors"
 )
@@ -93,6 +94,7 @@ func isValidProposal(
 		signedProposal.Message.Round,
 		signedProposal.FullData,
 		valCheck,
+		true,
 	); err != nil {
 		return errors.Wrap(err, "proposal not justified")
 	}
@@ -114,6 +116,7 @@ func isProposalJustification(
 	round Round,
 	fullData []byte,
 	valCheck ProposedValueCheckF,
+	verifySignatures bool,
 ) error {
 	if err := valCheck(fullData); err != nil {
 		return errors.Wrap(err, "proposal fullData invalid")
@@ -126,9 +129,16 @@ func isProposalJustification(
 		// no quorum, duplicate signers,  invalid still has quorum, invalid no quorum
 		// prepared
 		for _, rc := range roundChangeMsgs {
-			if err := validRoundChangeForData(state, config, rc, height, round, fullData); err != nil {
-				return errors.Wrap(err, "change round msg not valid")
+			if verifySignatures {
+				if err := validSignedRoundChangeForData(state, config, rc, height, round, fullData); err != nil {
+					return errors.Wrap(err, "change round msg not valid")
+				}
+			} else {
+				if err := validRoundChangeForData(state, config, rc, height, round, fullData); err != nil {
+					return errors.Wrap(err, "change round msg not valid")
+				}
 			}
+
 		}
 
 		// check there is a quorum
@@ -178,15 +188,28 @@ func isProposalJustification(
 
 			// validate each prepare message against the highest previously prepared fullData and round
 			for _, pm := range prepareMsgs {
-				if err := validSignedPrepareForHeightRoundAndRoot(
-					config,
-					pm,
-					height,
-					rcm.Message.DataRound,
-					rcm.Message.Root,
-					state.Share.Committee,
-				); err != nil {
-					return errors.New("signed prepare not valid")
+				if verifySignatures {
+					if err := validSignedPrepareForHeightRoundAndRoot(
+						config,
+						pm,
+						height,
+						rcm.Message.DataRound,
+						rcm.Message.Root,
+						state.Share.Committee,
+					); err != nil {
+						return errors.New("signed prepare not valid")
+					}
+				} else {
+					if err := validPrepareForHeightRoundAndRoot(
+						config,
+						pm,
+						height,
+						rcm.Message.DataRound,
+						rcm.Message.Root,
+						state.Share.Committee,
+					); err != nil {
+						return errors.New("signed prepare not valid")
+					}
 				}
 			}
 			return nil

qbft/round_change.go

@@ -2,6 +2,7 @@ package qbft
 
 import (
 	"bytes"
+
 	"github.com/bloxapp/ssv-spec/types"
 	"github.com/pkg/errors"
 )
@@ -200,12 +201,15 @@ func isReceivedProposalJustification(
 		newRound,
 		value,
 		valCheck,
+		false,
 	); err != nil {
 		return errors.Wrap(err, "proposal not justified")
 	}
 	return nil
 }
 
+// validRoundChangeForData is similar to the validSignedRoundChangeForData function
+// However, it does not verify signatures
 func validRoundChangeForData(
 	state *State,
 	config IConfig,
@@ -227,6 +231,72 @@ func validRoundChangeForData(
 		return errors.New("msg allows 1 signer")
 	}
 
+	if err := signedMsg.Message.Validate(); err != nil {
+		return errors.Wrap(err, "roundChange invalid")
+	}
+
+	// Addition to formal spec
+	// We add this extra tests on the msg itself to filter round change msgs with invalid justifications, before they are inserted into msg containers
+	if signedMsg.Message.RoundChangePrepared() {
+		r, err := HashDataRoot(fullData)
+		if err != nil {
+			return errors.Wrap(err, "could not hash input data")
+		}
+
+		// validate prepare message justifications
+		prepareMsgs, _ := signedMsg.Message.GetRoundChangeJustifications() // no need to check error, checked on signedMsg.Message.Validate()
+		for _, pm := range prepareMsgs {
+			if err := validPrepareForHeightRoundAndRoot(
+				config,
+				pm,
+				state.Height,
+				signedMsg.Message.DataRound,
+				signedMsg.Message.Root,
+				state.Share.Committee); err != nil {
+				return errors.Wrap(err, "round change justification invalid")
+			}
+		}
+
+		if !bytes.Equal(r[:], signedMsg.Message.Root[:]) {
+			return errors.New("H(data) != root")
+		}
+
+		if !HasQuorum(state.Share, prepareMsgs) {
+			return errors.New("no justifications quorum")
+		}
+
+		if signedMsg.Message.DataRound > round {
+			return errors.New("prepared round > round")
+		}
+
+		return nil
+	}
+	return nil
+}
+
+// validSignedRoundChangeForData is based on dafny's validRoundChange function
+// with the addition that nested Prepare messages are also validated
+func validSignedRoundChangeForData(
+	state *State,
+	config IConfig,
+	signedMsg *SignedMessage,
+	height Height,
+	round Round,
+	fullData []byte,
+) error {
+	if signedMsg.Message.MsgType != RoundChangeMsgType {
+		return errors.New("round change msg type is wrong")
+	}
+	if signedMsg.Message.Height != height {
+		return errors.New("wrong msg height")
+	}
+	if signedMsg.Message.Round != round {
+		return errors.New("wrong msg round")
+	}
+	if len(signedMsg.GetSigners()) != 1 {
+		return errors.New("msg allows 1 signer")
+	}
+
 	if err := signedMsg.Signature.VerifyByOperators(signedMsg, config.GetSignatureDomainType(), types.QBFTSignatureType, state.Share.Committee); err != nil {
 		return errors.Wrap(err, "msg signature invalid")
 	}