Skip to content

Commit 9e40627

Browse files
committed
fix(qbft): validate round change justification consistency with data_round
Fix critical consensus vulnerability where malicious nodes could include unvalidated prepare messages in round changes claiming no preparation (data_round=0). Changes: - Add validation in validate_proposal_justifications() to reject round changes with data_round=0 but non-empty prepare justifications - Add validation in received_round_change() for the same vulnerability - Add comprehensive test verifying both malicious and valid cases According to EEA QBFT v1 specification: - If data_round == 0: No prepare justifications should be present - If data_round > 0: Prepare justifications MUST be validated This prevents consensus safety violations where unvalidated prepare messages could bypass validation and corrupt justification logic.
1 parent d737a23 commit 9e40627

File tree

2 files changed

+216
-0
lines changed

2 files changed

+216
-0
lines changed

anchor/common/qbft/src/lib.rs

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -638,6 +638,16 @@ where
638638
return false;
639639
}
640640
}
641+
} else {
642+
// If data_round == 0 (no preparation claimed), validate that no prepare justifications are provided
643+
// This prevents malicious nodes from including unvalidated prepare messages
644+
if !round_change.round_change_justification.is_empty() {
645+
warn!(
646+
"Round change claims no preparation (data_round=0) but includes {} prepare justifications",
647+
round_change.round_change_justification.len()
648+
);
649+
return false;
650+
}
641651
}
642652
}
643653

@@ -964,6 +974,17 @@ where
964974
return;
965975
}
966976
}
977+
} else {
978+
// If data_round == 0 (no preparation claimed), validate that no prepare justifications are provided
979+
// This prevents malicious nodes from including unvalidated prepare messages
980+
if !qbft_msg.round_change_justification.is_empty() {
981+
debug!(
982+
from = *operator_id,
983+
justifications = qbft_msg.round_change_justification.len(),
984+
"ROUNDCHANGE claims no preparation (data_round=0) but includes prepare justifications"
985+
);
986+
return;
987+
}
967988
}
968989

969990
debug!(from = ?operator_id, state = ?self.state, "ROUNDCHANGE received");

anchor/common/qbft/src/tests.rs

Lines changed: 195 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -356,3 +356,198 @@ fn test_round_change_validation_skips_round_one_prepared_values() {
356356
incorrectly skips prepare justification checking for round 1 preparations."
357357
);
358358
}
359+
360+
#[test]
361+
/// Test that verifies QBFT rejects round change messages with invalid justification patterns
362+
///
363+
/// This test verifies the fix for a critical consensus vulnerability where malicious nodes
364+
/// could include unvalidated prepare messages in round changes claiming no preparation.
365+
///
366+
/// According to EEA QBFT v1 specification:
367+
/// - If data_round == 0: No prepare justifications should be present
368+
/// - If data_round > 0: Prepare justifications MUST be validated
369+
fn test_round_change_justification_validation_vulnerability_fix() {
370+
if ENABLE_TEST_LOGGING {
371+
let env_filter = EnvFilter::new("debug");
372+
let _ = tracing_subscriber::fmt()
373+
.compact()
374+
.with_env_filter(env_filter)
375+
.try_init();
376+
}
377+
378+
use ssv_types::{
379+
consensus::QbftMessage,
380+
message::{MsgType, RSA_SIGNATURE_SIZE, SSVMessage, SignedSSVMessage},
381+
};
382+
383+
// Create QBFT instance
384+
let config = ConfigBuilder::<DefaultLeaderFunction>::new(
385+
1.into(),
386+
InstanceHeight::default(),
387+
(1..=4).map(OperatorId::from).collect(), // 4 nodes, quorum = 3
388+
)
389+
.with_operator_id(OperatorId::from(1))
390+
.build()
391+
.expect("config should be valid");
392+
393+
let test_data = TestData(999);
394+
let mut qbft_instance = Qbft::new(
395+
config,
396+
test_data.clone(),
397+
Box::new(NoDataValidation),
398+
MessageId::from([0; 56]),
399+
|_| {},
400+
);
401+
402+
println!("Testing vulnerability fix for round change justification validation...");
403+
404+
// TEST 1: Malicious round change with data_round=0 but includes prepare justifications
405+
// This should be REJECTED after our fix
406+
println!("TEST 1: Round change with data_round=0 but includes prepare justifications");
407+
408+
// Create a malicious prepare message to include in justifications
409+
let malicious_prepare = QbftMessage {
410+
qbft_message_type: QbftMessageType::Prepare,
411+
height: 0,
412+
round: 1,
413+
identifier: [0; 56].to_vec().into(),
414+
root: test_data.hash(),
415+
data_round: 0,
416+
round_change_justification: vec![],
417+
prepare_justification: vec![],
418+
};
419+
420+
let malicious_prepare_ssv = SSVMessage::new(
421+
MsgType::SSVConsensusMsgType,
422+
MessageId::from([0; 56]),
423+
malicious_prepare.as_ssz_bytes(),
424+
)
425+
.expect("should create malicious prepare SSVMessage");
426+
427+
let signed_malicious_prepare = SignedSSVMessage::new(
428+
vec![vec![0; RSA_SIGNATURE_SIZE]],
429+
vec![OperatorId::from(2)],
430+
malicious_prepare_ssv,
431+
vec![],
432+
)
433+
.expect("should create signed malicious prepare");
434+
435+
// Create round change with data_round=0 BUT includes the malicious prepare in justifications
436+
let malicious_round_change = QbftMessage {
437+
qbft_message_type: QbftMessageType::RoundChange,
438+
height: 0,
439+
round: 2,
440+
identifier: [0; 56].to_vec().into(),
441+
root: Hash256::default(), // No preparation claimed
442+
data_round: 0, // Claims NO preparation
443+
round_change_justification: vec![signed_malicious_prepare], // BUT includes justifications!
444+
prepare_justification: vec![],
445+
};
446+
447+
let malicious_rc_ssv = SSVMessage::new(
448+
MsgType::SSVConsensusMsgType,
449+
MessageId::from([0; 56]),
450+
malicious_round_change.as_ssz_bytes(),
451+
)
452+
.expect("should create malicious round change SSVMessage");
453+
454+
let signed_malicious_rc = SignedSSVMessage::new(
455+
vec![vec![0; RSA_SIGNATURE_SIZE]],
456+
vec![OperatorId::from(2)],
457+
malicious_rc_ssv,
458+
vec![],
459+
)
460+
.expect("should create signed malicious round change");
461+
462+
let wrapped_malicious_rc = WrappedQbftMessage {
463+
signed_message: signed_malicious_rc,
464+
qbft_message: malicious_round_change,
465+
};
466+
467+
// Advance to round 2 to receive the round change
468+
qbft_instance.current_round = Round::from(2);
469+
470+
let initial_round_changes = qbft_instance
471+
.round_change_container
472+
.get_messages_for_round(2.into())
473+
.len();
474+
475+
// Try to receive the malicious round change - should be REJECTED
476+
qbft_instance.received_round_change(OperatorId::from(2), Round::from(2), wrapped_malicious_rc);
477+
478+
let final_round_changes = qbft_instance
479+
.round_change_container
480+
.get_messages_for_round(2.into())
481+
.len();
482+
483+
// Verify the malicious round change was rejected (not stored)
484+
assert_eq!(
485+
initial_round_changes, final_round_changes,
486+
"VULNERABILITY: Malicious round change with data_round=0 but including prepare justifications was accepted! \
487+
This violates QBFT safety by allowing unvalidated prepare messages to bypass consensus checks."
488+
);
489+
490+
println!("✓ TEST 1 PASSED: Round change with data_round=0 and justifications correctly rejected");
491+
492+
// TEST 2: Valid round change with data_round=0 and NO justifications
493+
// This should be ACCEPTED
494+
println!("TEST 2: Valid round change with data_round=0 and no justifications");
495+
496+
let valid_round_change = QbftMessage {
497+
qbft_message_type: QbftMessageType::RoundChange,
498+
height: 0,
499+
round: 2,
500+
identifier: [0; 56].to_vec().into(),
501+
root: Hash256::default(),
502+
data_round: 0, // No preparation claimed
503+
round_change_justification: vec![], // Correctly empty
504+
prepare_justification: vec![],
505+
};
506+
507+
let valid_rc_ssv = SSVMessage::new(
508+
MsgType::SSVConsensusMsgType,
509+
MessageId::from([0; 56]),
510+
valid_round_change.as_ssz_bytes(),
511+
)
512+
.expect("should create valid round change SSVMessage");
513+
514+
let signed_valid_rc = SignedSSVMessage::new(
515+
vec![vec![0; RSA_SIGNATURE_SIZE]],
516+
vec![OperatorId::from(3)],
517+
valid_rc_ssv,
518+
vec![],
519+
)
520+
.expect("should create signed valid round change");
521+
522+
let wrapped_valid_rc = WrappedQbftMessage {
523+
signed_message: signed_valid_rc,
524+
qbft_message: valid_round_change,
525+
};
526+
527+
let initial_valid_count = qbft_instance
528+
.round_change_container
529+
.get_messages_for_round(2.into())
530+
.len();
531+
532+
// Try to receive the valid round change - should be ACCEPTED
533+
qbft_instance.received_round_change(OperatorId::from(3), Round::from(2), wrapped_valid_rc);
534+
535+
let final_valid_count = qbft_instance
536+
.round_change_container
537+
.get_messages_for_round(2.into())
538+
.len();
539+
540+
// Verify the valid round change was accepted (stored)
541+
assert_eq!(
542+
final_valid_count,
543+
initial_valid_count + 1,
544+
"Valid round change with data_round=0 and empty justifications should be accepted"
545+
);
546+
547+
println!("✓ TEST 2 PASSED: Valid round change with data_round=0 and empty justifications correctly accepted");
548+
549+
println!("SUCCESS: QBFT round change justification validation vulnerability has been fixed!");
550+
println!("- Malicious round changes with data_round=0 but non-empty justifications are rejected");
551+
println!("- Valid round changes with data_round=0 and empty justifications are accepted");
552+
println!("- This prevents consensus safety violations from unvalidated prepare message injection");
553+
}

0 commit comments

Comments
 (0)