diff --git a/qbft/instance.go b/qbft/instance.go index c357cdfdb..8de6fb0dd 100644 --- a/qbft/instance.go +++ b/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") } diff --git a/qbft/prepare.go b/qbft/prepare.go index 49c4ef0f9..10a17778e 100644 --- a/qbft/prepare.go +++ b/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 } diff --git a/qbft/proposal.go b/qbft/proposal.go index fa62d49fd..0ab10922c 100644 --- a/qbft/proposal.go +++ b/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 diff --git a/qbft/round_change.go b/qbft/round_change.go index 9232e60f8..561d95cd1 100644 --- a/qbft/round_change.go +++ b/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") }