fix: Validation for prepared round changes before insertion (#528)

If we have prepared a value, we include it in a round change messages and include the quorum of prepare justifications proving that value was prepared in the round_change_justifications field. These round change messages have to have the round_change_justifications prepare messages validated to ensure that these messages actually prove that the value was prepared. Otherwise, you could claim to prepare some value but not properly be able to support/prove it.


  Implement the missing validation and change existing code a bit to be able to reuse `is_valid_prepare_justification_for_round_and_root`.

Author: dknopik

anchor/common/qbft/src/lib.rs

@@ -1,4 +1,7 @@
-use std::{collections::HashMap, sync::Arc};
+use std::{
+    collections::{HashMap, HashSet},
+    sync::Arc,
+};
 
 // Re-Exports for Manager
 pub use config::{Config, ConfigBuilder};
@@ -229,6 +232,16 @@ where
         self.config.committee_members().contains(operator_id)
     }
 
+    /// Checks if we have a quorum of unique committee operators from these messages.
+    fn check_quorum<'a>(&self, msgs: impl IntoIterator<Item = &'a SignedSSVMessage>) -> bool {
+        let unique_operators = msgs
+            .into_iter()
+            .flat_map(|justification| justification.operator_ids())
+            .filter(|operator_id| self.check_committee(operator_id))
+            .collect::<HashSet<_>>();
+        unique_operators.len() >= self.config.quorum_size()
+    }
+
     // Perform base QBFT relevant message verification. This verfiication is applicable to all QBFT
     // message types
     // Return type expresses that we either have
@@ -465,7 +478,7 @@ where
         // received proposal
         if round > Round::default() {
             // validate the justifications
-            if !self.validate_justifications(&wrapped_msg) {
+            if !self.validate_proposal_justifications(&wrapped_msg) {
                 warn!(from = ?operator_id, "Justification validation failed for proposal");
                 return;
             }
@@ -517,27 +530,45 @@ where
         self.send_prepare(wrapped_msg.qbft_message.root);
     }
 
-    // Validate the round change and prepare justifications. Returns true if the justifications
-    // correctly justify the proposal
+    // Validate the round change and prepare justifications for proposal.
+    // Returns true if the justifications correctly justify the proposal
     //
     // A QBFT Message contains fields to a list of round change justifications and prepare
     // justifications. We must go through each of these individually and verify the validity of each
     // one
-    fn validate_justifications(&self, msg: &WrappedQbftMessage) -> bool {
+    //
+    // Proposal
+    // - round change justifications
+    //  - list of round change messages
+    //      - each round change message has list of prepare messages if it prepared a value
+    // - prepare justifications
+    //  - list of prepare messages to
+    fn validate_proposal_justifications(&self, msg: &WrappedQbftMessage) -> bool {
         // Record if any of the round change messages have a value that was prepared
-        let mut previously_prepared = false;
         let mut max_prepared_round = 0;
         let mut max_prepared_msg = None;
 
         // Make sure we have a quorum of round change messages
-        if msg.qbft_message.round_change_justification.len() < self.config.quorum_size() {
+        if !self.check_quorum(&msg.qbft_message.round_change_justification) {
             warn!("Did not receive a quorum of round change messages");
             return false;
         }
 
         // There was a quorum of round change justifications. We need to go though and verify each
         // one. Each will be a SignedSSVMessage
         for signed_round_change in &msg.qbft_message.round_change_justification {
+            // Check for multi-signers - round change messages should only have 1 signer
+            if signed_round_change.operator_ids().len() > 1 {
+                return false;
+            }
+
+            // make sure all signers in committee
+            for signer in signed_round_change.operator_ids() {
+                if !self.check_committee(signer) {
+                    return false;
+                }
+            }
+
             // The qbft message is represented as a Vec<u8> in the signed message, deserialize this
             // into a proper QbftMessage
             let round_change: QbftMessage =
@@ -552,34 +583,69 @@ where
                 return false;
             }
 
-            // Convert to a wrapped message and perform verification
-            let wrapped = WrappedQbftMessage {
-                signed_message: signed_round_change.clone(),
-                qbft_message: round_change.clone(),
-            };
-
-            if self.validate_message(&wrapped).is_none() {
-                warn!("ROUNDCHANGE message validation failed");
+            // make sure the round change matches the round of the message
+            if round_change.round != msg.qbft_message.round {
                 return false;
             }
 
-            // If the data_round > 1, that means we have prepared a value in previous rounds
-            if round_change.data_round > 1 {
-                previously_prepared = true;
+            // For round change justifications, we need special validation that doesn't check
+            // against current round since they're justifications from the proposal's round
+            // Check height
+            if round_change.height != *self.instance_height as u64 {
+                return false;
+            }
 
+            // If the data_round > 0, that means we have prepared a value in previous rounds
+            // We also have to go through all of the prepare justifications in the round change to
+            // ensure that they are well formed and properly justify the prepared value
+            if round_change.data_round > 0 {
                 // also track the max prepared value and round
                 if round_change.data_round > max_prepared_round {
                     max_prepared_round = round_change.data_round;
-                    max_prepared_msg = Some(round_change);
+                    max_prepared_msg = Some(round_change.clone());
+                }
+
+                // Check that prepared round is not greater than current round
+                if round_change.data_round > round_change.round {
+                    warn!(
+                        "Round change has prepared round {} > round {}",
+                        round_change.data_round, round_change.round
+                    );
+                    return false;
+                }
+
+                // Verify that if round change has full data, it matches the root
+                if msg.qbft_message.root != round_change.root {
+                    warn!("Proposal root doesn't match round change prepared root");
+                    return false;
+                }
+
+                if !self.check_quorum(&round_change.round_change_justification) {
+                    warn!(
+                        num_justifications = round_change.round_change_justification.len(),
+                        "Not enough prepare messages for quorum"
+                    );
+                    return false;
+                }
+
+                // go through all of the round changes prepare justifications
+                for signed_prepare in &round_change.round_change_justification {
+                    if !self.is_valid_prepare_justification_for_round_and_root(
+                        signed_prepare,
+                        round_change.data_round.into(),
+                        &round_change.root,
+                    ) {
+                        return false;
+                    }
                 }
             }
         }
 
         // If there was a value that was also previously prepared, we must also verify all of the
         // prepare justifications
-        if previously_prepared {
+        if let Some(max_prepared_msg) = max_prepared_msg {
             // Make sure we have a quorum of prepare messages
-            if msg.qbft_message.prepare_justification.len() < self.config.quorum_size() {
+            if !self.check_quorum(&msg.qbft_message.prepare_justification) {
                 warn!(
                     num_justifications = msg.qbft_message.prepare_justification.len(),
                     "Not enough prepare messages for quorum"
@@ -588,48 +654,59 @@ where
             }
 
             // Make sure that the roots match
-            if msg.qbft_message.root
-                != max_prepared_msg
-                    .clone()
-                    .expect("Exists as we have a previously prepared value")
-                    .root
-            {
+            if msg.qbft_message.root != max_prepared_msg.root {
                 warn!("Highest prepared does not match proposed data");
                 return false;
             }
 
             // Validate each prepare message matches highest prepared round/value
             for signed_prepare in &msg.qbft_message.prepare_justification {
-                // The qbft message is represented as Vec<u8> in the signed message, deserialize
-                // this into a qbft message
-                let prepare = match QbftMessage::from_ssz_bytes(signed_prepare.ssv_message().data())
-                {
-                    Ok(data) => data,
-                    Err(_) => return false,
-                };
-
-                // Make sure this is a prepare message
-                if prepare.qbft_message_type != QbftMessageType::Prepare {
-                    warn!("Expected a prepare message");
+                if !self.is_valid_prepare_justification_for_round_and_root(
+                    signed_prepare,
+                    max_prepared_msg.data_round.into(),
+                    &max_prepared_msg.root,
+                ) {
                     return false;
                 }
+            }
+        }
+        true
+    }
 
-                let wrapped = WrappedQbftMessage {
-                    signed_message: signed_prepare.clone(),
-                    qbft_message: prepare.clone(),
-                };
+    fn is_valid_prepare_justification_for_round_and_root(
+        &self,
+        justification: &SignedSSVMessage,
+        round: Round,
+        root: &Hash256,
+    ) -> bool {
+        // The qbft message is represented as Vec<u8> in the signed message, deserialize this into
+        // a qbft message
+        let Ok(prepare) = QbftMessage::from_ssz_bytes(justification.ssv_message().data()) else {
+            warn!("Failed to decode prepare justification message");
+            return false;
+        };
 
-                if self.validate_message(&wrapped).is_none() {
-                    warn!("PREPARE message validation failed");
-                    return false;
-                }
+        // Make sure this is a prepare message
+        if prepare.qbft_message_type != QbftMessageType::Prepare {
+            warn!("Expected a prepare message");
+            return false;
+        }
 
-                if prepare.root != msg.qbft_message.root {
-                    warn!("Proposed data mismatch");
-                    return false;
-                }
-            }
+        if prepare.height != *self.instance_height as u64 {
+            warn!("PREPARE height incorrect");
+            return false;
         }
+
+        if prepare.round != round.get() as u64 {
+            warn!("PREPARE round incorrect");
+            return false;
+        }
+
+        if &prepare.root != root {
+            warn!("Proposed data mismatch");
+            return false;
+        }
+
         true
     }
 
@@ -851,6 +928,44 @@ where
             return;
         }
 
+        let qbft_msg = &wrapped_msg.qbft_message;
+        // If this is a "prepared" round change, we have to check the justifications.
+        if qbft_msg.data_round > 0 {
+            if !self.check_quorum(&qbft_msg.round_change_justification) {
+                debug!(
+                    from = *operator_id,
+                    justifications = qbft_msg.round_change_justification.len(),
+                    quorum = self.config.quorum_size(),
+                    "prepared ROUNDCHANGE has no quorum"
+                );
+                return;
+            }
+
+            if qbft_msg.data_round > qbft_msg.round {
+                debug!(
+                    from = *operator_id,
+                    data_round = qbft_msg.data_round,
+                    round = qbft_msg.round,
+                    "ROUNDCHANGE has prepared round after round"
+                );
+                return;
+            }
+
+            for justification in qbft_msg.round_change_justification.iter() {
+                if !self.is_valid_prepare_justification_for_round_and_root(
+                    justification,
+                    qbft_msg.data_round.into(),
+                    &qbft_msg.root,
+                ) {
+                    debug!(
+                        from = *operator_id,
+                        "ROUNDCHANGE has invalid prepare justification"
+                    );
+                    return;
+                }
+            }
+        }
+
         debug!(from = ?operator_id, state = ?self.state, "ROUNDCHANGE received");
 
         // Store the round changed message