Audit Suggestion 5: Unnecessary checks increase complexity

qbft/messages.go

@@ -3,6 +3,7 @@ package qbft
 import (
 	"bytes"
 	"crypto/sha256"
+
 	"github.com/bloxapp/ssv-spec/types"
 	"github.com/pkg/errors"
 )
@@ -53,15 +54,15 @@ type Message struct {
 	Root                     [32]byte `ssz-size:"32"`
 	DataRound                Round
 	RoundChangeJustification [][]byte `ssz-max:"13,65536"` // 2^16
-	PrepareJustification     [][]byte `ssz-max:"13,65536"` // 2^16
+	ProposalJustification    [][]byte `ssz-max:"13,65536"` // 2^16
 }
 
 func (msg *Message) GetRoundChangeJustifications() ([]*SignedMessage, error) {
 	return unmarshalJustifications(msg.RoundChangeJustification)
 }
 
-func (msg *Message) GetPrepareJustifications() ([]*SignedMessage, error) {
-	return unmarshalJustifications(msg.PrepareJustification)
+func (msg *Message) GetProposalJustifications() ([]*SignedMessage, error) {
+	return unmarshalJustifications(msg.ProposalJustification)
 }
 
 func unmarshalJustifications(data [][]byte) ([]*SignedMessage, error) {
@@ -121,7 +122,7 @@ func (msg *Message) Validate() error {
 	if _, err := msg.GetRoundChangeJustifications(); err != nil {
 		return err
 	}
-	if _, err := msg.GetPrepareJustifications(); err != nil {
+	if _, err := msg.GetProposalJustifications(); err != nil {
 		return err
 	}
 	if msg.MsgType > RoundChangeMsgType {
@@ -245,7 +246,7 @@ func (signedMsg *SignedMessage) DeepCopy() *SignedMessage {
 
 		Root:                     signedMsg.Message.Root,
 		DataRound:                signedMsg.Message.DataRound,
-		PrepareJustification:     signedMsg.Message.PrepareJustification,
+		ProposalJustification:    signedMsg.Message.ProposalJustification,
 		RoundChangeJustification: signedMsg.Message.RoundChangeJustification,
 	}
 	copy(ret.Message.Identifier, signedMsg.Message.Identifier)

qbft/proposal.go

@@ -2,6 +2,7 @@ package qbft
 
 import (
 	"bytes"
+
 	"github.com/bloxapp/ssv-spec/types"
 	"github.com/pkg/errors"
 )
@@ -82,13 +83,13 @@ func isValidProposal(
 
 	// get justifications
 	roundChangeJustification, _ := signedProposal.Message.GetRoundChangeJustifications() // no need to check error, checked on signedProposal.Validate()
-	prepareJustification, _ := signedProposal.Message.GetPrepareJustifications()         // no need to check error, checked on signedProposal.Validate()
+	proposalJustification, _ := signedProposal.Message.GetProposalJustifications()       // no need to check error, checked on signedProposal.Validate()
 
 	if err := isProposalJustification(
 		state,
 		config,
+		proposalJustification,
 		roundChangeJustification,
-		prepareJustification,
 		state.Height,
 		signedProposal.Message.Round,
 		signedProposal.FullData,
@@ -115,83 +116,90 @@ func isProposalJustification(
 	fullData []byte,
 	valCheck ProposedValueCheckF,
 ) error {
+	// Check proposed data
 	if err := valCheck(fullData); err != nil {
 		return errors.Wrap(err, "proposal fullData invalid")
 	}
 
+	// If first round -> no validation to do
 	if round == FirstRound {
 		return nil
-	} else {
-		// check all round changes are valid for height and round
-		// 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")
-			}
-		}
+	}
 
-		// check there is a quorum
-		if !HasQuorum(state.Share, roundChangeMsgs) {
-			return errors.New("change round has no quorum")
+	// Check unique signers of RC messages reach quorum
+	if !HasQuorum(state.Share, roundChangeMsgs) {
+		return errors.New("change round has no quorum")
+	}
+
+	// Validate each round change without validating the nested prepare messages
+	for _, rc := range roundChangeMsgs {
+		if err := validRoundChange(state, config, rc, height, round, fullData); err != nil {
+			return errors.Wrap(err, "change round msg not valid")
 		}
+	}
 
-		// previouslyPreparedF returns true if any on the round change messages have a prepared round and fullData
-		previouslyPrepared, err := func(rcMsgs []*SignedMessage) (bool, error) {
-			for _, rc := range rcMsgs {
-				if rc.Message.RoundChangePrepared() {
-					return true, nil
-				}
-			}
-			return false, nil
-		}(roundChangeMsgs)
-		if err != nil {
-			return errors.Wrap(err, "could not calculate if previously prepared")
+	// Check if at least one RC message is prepared
+	hasPreparedRoundChange := false
+	for _, rc := range roundChangeMsgs {
+		if rc.Message.RoundChangePrepared() {
+			hasPreparedRoundChange = true
+			break
 		}
+	}
+	if !hasPreparedRoundChange {
+		return nil
+	}
 
-		if !previouslyPrepared {
-			return nil
-		} else {
+	// If at least one Round-Change message is prepared,
+	// we validate the proposed quorum of prepare messages against the highest prepared
 
-			// check prepare quorum
-			if !HasQuorum(state.Share, prepareMsgs) {
-				return errors.New("prepares has no quorum")
-			}
+	// check prepare quorum
+	if !HasQuorum(state.Share, prepareMsgs) {
+		return errors.New("prepares has no quorum")
+	}
 
-			// get a round change data for which there is a justification for the highest previously prepared round
-			rcm, err := highestPrepared(roundChangeMsgs)
-			if err != nil {
-				return errors.Wrap(err, "could not get highest prepared")
-			}
-			if rcm == nil {
-				return errors.New("no highest prepared")
-			}
+	// get a round change data for which there is a justification for the highest previously prepared round
+	rcm, err := highestPrepared(roundChangeMsgs)
+	if err != nil {
+		return errors.Wrap(err, "could not get highest prepared")
+	}
+	if rcm == nil {
+		return errors.New("no highest prepared")
+	}
 
-			// proposed fullData must equal highest prepared fullData
-			r, err := HashDataRoot(fullData)
-			if err != nil {
-				return errors.Wrap(err, "could not hash input data")
-			}
-			if !bytes.Equal(r[:], rcm.Message.Root[:]) {
-				return errors.New("proposed data doesn't match highest prepared")
-			}
+	checkHighest := func(rcm *SignedMessage, expectedRoot [32]byte) error {
+		// proposed root must equal highest prepared root
+		if !bytes.Equal(expectedRoot[:], rcm.Message.Root[:]) {
+			return errors.New("proposed data doesn't match highest prepared")
+		}
 
-			// 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")
-				}
+		// 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")
 			}
+		}
+		return nil
+	}
+
+	highestPreparedRCs := highestPreparedSet(roundChangeMsgs)
+	r, err := HashDataRoot(fullData)
+	if err != nil {
+		return errors.Wrap(err, "could not hash input data")
+	}
+	for _, rcm := range highestPreparedRCs {
+		if checkHighest(rcm, r) == nil {
 			return nil
 		}
 	}
+	return errors.New("No highest prepared round-change matches prepared messages")
 }
 
 func proposer(state *State, config IConfig, round Round) types.OperatorID {
@@ -234,8 +242,8 @@ func CreateProposal(state *State, config IConfig, fullData []byte, roundChanges,
 		Identifier: state.ID,
 
 		Root:                     r,
-		RoundChangeJustification: roundChangesData,
-		PrepareJustification:     preparesData,
+		RoundChangeJustification: preparesData,
+		ProposalJustification:    roundChangesData,
 	}
 	sig, err := config.GetSigner().SignRoot(msg, types.QBFTSignatureType, state.Share.SharePubKey)
 	if err != nil {