Skip to content

Verify missing dkg shares reports and mark offending signers as malicious #166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
xoloki wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Verify missing dkg shares reports and mark offending signers as malicious #166

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
89d8963
verify that reported missing dkg public shares are actually missing, …
xoloki May 7, 2025
1e0bc06
mark signers as malicious when fire coordinator gets a report of miss…
xoloki May 8, 2025
81e34b3
collapse else/if to else if
xoloki May 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
247 changes: 245 additions & 2 deletions src/state_machine/coordinator/fire.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -717,10 +717,50 @@ impl<Aggregator: AggregatorTrait> Coordinator<Aggregator> {
}
}
}
DkgFailure::MissingPublicShares(_) => {
DkgFailure::MissingPublicShares(missing_signer_ids) => {
for id in missing_signer_ids {
if let Some(shares) = self.dkg_public_shares.get(id) {
if shares.comms.is_empty() {
self.malicious_dkg_signer_ids.insert(*id);
} else {
// signer_id reported missing shares but they were present
self.malicious_dkg_signer_ids.insert(*signer_id);
Comment on lines +726 to +727
Copy link
Copy Markdown
Collaborator

@djordon djordon May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, it's not clear that we want to do this. Just because the coordinator and the peer don't have shares doesn't mean that the peer was being malicious. An attacker, who is trying to remove a peer from the network, can try to prevent the peer from receiving messages. This is easy to do in systems where messages aren't gossiped, since all the attacker needs to do is not send a peer some shares but ensure that the coordinator gets my message. Even in gossiped protocols, a minority of signers may be able to prevent a peer from receiving messages.

On the flip side, we do not want signers to falsely claim that peers are being malicious, so I get why this was reported.

I'm guessing the reason why we didn't do this to begin with was because we cannot ascertain whether missing shares implies malicious behavior. Also, it's not clear if the malicious party was either the attacker or the attackee.

Copy link
Copy Markdown
Owner Author

@xoloki xoloki May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, which is why I didn’t originally do this. I’m okay with not merging this change, I just wanted to have PRs up for every reported issue.

}
} else {
self.malicious_dkg_signer_ids.insert(*id);
}
}
dkg_failures.insert(*signer_id, dkg_failure.clone());
}
DkgFailure::MissingPrivateShares(_) => {
DkgFailure::MissingPrivateShares(missing_signer_ids) => {
for id in missing_signer_ids {
if let Some(shares) = self.dkg_private_shares.get(id) {
if shares.shares.is_empty() {
self.malicious_dkg_signer_ids.insert(*id);
} else if let Some(signer_key_ids) =
self.config.signer_key_ids.get(signer_id)
{
let mut missing_private_shares = false;
for dst_key_id in signer_key_ids {
for (_src_party_id, party_shares) in &shares.shares {
if party_shares.get(dst_key_id).is_none() {
missing_private_shares = true;
}
}
}
if missing_private_shares {
self.malicious_dkg_signer_ids.insert(*id);
} else {
// signer_id reported missing shares but they were present
self.malicious_dkg_signer_ids.insert(*signer_id);
}
} else {
error!("Got MissingPrivateShares from {signer_id} but we don't have key_ids for them");
}
} else {
self.malicious_dkg_signer_ids.insert(*id);
}
}
dkg_failures.insert(*signer_id, dkg_failure.clone());
}
}
Expand Down Expand Up @@ -1986,6 +2026,209 @@ pub mod test {
(minimum_coordinators, minimum_signers)
}

#[test]
fn malicious_missing_public_shares_dkg_v1() {
malicious_missing_public_shares_dkg::<v1::Aggregator, v1::Signer>(5, 2);
}

#[test]
fn malicious_missing_public_shares_dkg_v2() {
malicious_missing_public_shares_dkg::<v2::Aggregator, v2::Signer>(5, 2);
}

fn malicious_missing_public_shares_dkg<Aggregator: AggregatorTrait, SignerType: SignerTrait>(
num_signers: u32,
keys_per_signer: u32,
) {
let (mut coordinators, mut signers) =
setup::<FireCoordinator<Aggregator>, SignerType>(num_signers, keys_per_signer);

// We have started a dkg round
let message = coordinators
.first_mut()
.unwrap()
.start_dkg_round(None)
.unwrap();
assert!(coordinators.first().unwrap().aggregate_public_key.is_none());
assert_eq!(coordinators.first().unwrap().state, State::DkgPublicGather);

// Send the DkgBegin message to all signers and share their responses with the coordinators and signers, but mutate one signer's DkgPublicShares so it is marked malicious
let (outbound_messages, operation_results) = feedback_mutated_messages(
&mut coordinators,
&mut signers,
&[message],
|signer, msgs| {
if signer.signer_id != 0 {
return msgs;
}
msgs.iter()
.map(|packet| {
let Message::DkgPublicShares(shares) = &packet.msg else {
return packet.clone();
};
// clear the comms
Packet {
msg: Message::DkgPublicShares(DkgPublicShares {
dkg_id: shares.dkg_id,
signer_id: shares.signer_id,
comms: Vec::new(),
}),
sig: vec![],
}
})
.collect()
},
);

assert!(operation_results.is_empty());
for coordinator in &coordinators {
assert_eq!(coordinator.state, State::DkgPrivateGather);
}

assert_eq!(outbound_messages.len(), 1);
assert!(
matches!(outbound_messages[0].msg, Message::DkgPrivateBegin(_)),
"Expected DkgPrivateBegin message"
);

// Send the DkgPrivateBegin message to all signers and gather responses by sharing with all other signers and coordinators
let (outbound_messages, operation_results) =
feedback_messages(&mut coordinators, &mut signers, &outbound_messages);

assert!(operation_results.is_empty());
assert_eq!(outbound_messages.len(), 1);
assert!(
matches!(outbound_messages[0].msg, Message::DkgEndBegin(_)),
"Expected DkgEndBegin message"
);

// Send the DkgEndBegin message to all signers and share their responses with the coordinators and signers
let (outbound_messages, operation_results) =
feedback_messages(&mut coordinators, &mut signers, &outbound_messages);
assert!(outbound_messages.is_empty());
assert_eq!(operation_results.len(), 1);
let OperationResult::DkgError(DkgError::DkgEndFailure(failure_map)) = &operation_results[0]
else {
panic!("Expected OperationResult::DkgError(DkgError::DkgEndFailure");
};

for (_signer_id, dkg_failure) in failure_map {
let DkgFailure::MissingPublicShares(bad_share_map) = dkg_failure else {
panic!("Expected DkgFailure::MissingPublicShares");
};
for bad_signer_id in bad_share_map {
assert_eq!(*bad_signer_id, 0u32);
}
}

for coordinator in &coordinators {
assert!(coordinator.malicious_dkg_signer_ids.contains(&0u32));
}
}

#[test]
fn malicious_missing_private_shares_dkg_v1() {
malicious_missing_private_shares_dkg::<v1::Aggregator, v1::Signer>(5, 2);
}

#[test]
fn malicious_missing_private_shares_dkg_v2() {
malicious_missing_private_shares_dkg::<v2::Aggregator, v2::Signer>(5, 2);
}

fn malicious_missing_private_shares_dkg<
Aggregator: AggregatorTrait,
SignerType: SignerTrait,
>(
num_signers: u32,
keys_per_signer: u32,
) {
let (mut coordinators, mut signers) =
setup::<FireCoordinator<Aggregator>, SignerType>(num_signers, keys_per_signer);

// We have started a dkg round
let message = coordinators
.first_mut()
.unwrap()
.start_dkg_round(None)
.unwrap();
assert!(coordinators.first().unwrap().aggregate_public_key.is_none());
assert_eq!(coordinators.first().unwrap().state, State::DkgPublicGather);

// Send the DkgBegin message to all signers and gather responses by sharing with all other signers and coordinators
let (outbound_messages, operation_results) =
feedback_messages(&mut coordinators, &mut signers, &[message]);

assert_eq!(outbound_messages.len(), 1);
assert!(
matches!(outbound_messages[0].msg, Message::DkgPrivateBegin(_)),
"Expected DkgPrivateBegin message"
);

assert!(operation_results.is_empty());
for coordinator in &coordinators {
assert_eq!(coordinator.state, State::DkgPrivateGather);
}

// Send the DkgPrivateBegin message to all signers and share their responses with the coordinators and signers, but mutate one signer's DkgPublicShares so it is marked malicious
let (outbound_messages, operation_results) = feedback_mutated_messages(
&mut coordinators,
&mut signers,
&outbound_messages,
|signer, msgs| {
if signer.signer_id != 0 {
return msgs;
}
msgs.iter()
.map(|packet| {
let Message::DkgPrivateShares(shares) = &packet.msg else {
return packet.clone();
};
// clear the shares
Packet {
msg: Message::DkgPrivateShares(DkgPrivateShares {
dkg_id: shares.dkg_id,
signer_id: shares.signer_id,
shares: Vec::new(),
}),
sig: vec![],
}
})
.collect()
},
);

assert!(operation_results.is_empty());
assert_eq!(outbound_messages.len(), 1);
assert!(
matches!(outbound_messages[0].msg, Message::DkgEndBegin(_)),
"Expected DkgEndBegin message"
);

// Send the DkgEndBegin message to all signers and share their responses with the coordinators and signers
let (outbound_messages, operation_results) =
feedback_messages(&mut coordinators, &mut signers, &outbound_messages);
assert!(outbound_messages.is_empty());
assert_eq!(operation_results.len(), 1);
let OperationResult::DkgError(DkgError::DkgEndFailure(failure_map)) = &operation_results[0]
else {
panic!("Expected OperationResult::DkgError(DkgError::DkgEndFailure");
};

for (_signer_id, dkg_failure) in failure_map {
let DkgFailure::MissingPrivateShares(bad_share_map) = dkg_failure else {
panic!("Expected DkgFailure::MissingPrivateShares");
};
for bad_signer_id in bad_share_map {
assert_eq!(*bad_signer_id, 0u32);
}
}

for coordinator in &coordinators {
assert!(coordinator.malicious_dkg_signer_ids.contains(&0u32));
}
}

#[test]
fn minimum_signers_dkg_v1() {
minimum_signers_dkg::<v1::Aggregator, v1::Signer>(10, 2);
Expand Down
Loading