Skip to content

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

Open
xoloki wants to merge 3 commits intomainfrom
missing-dkg-shares-malicious
Open

Verify missing dkg shares reports and mark offending signers as malicious#166
xoloki wants to merge 3 commits intomainfrom
missing-dkg-shares-malicious

Conversation

@xoloki
Copy link
Copy Markdown
Owner

@xoloki xoloki commented May 7, 2025

Fixes #163

@xoloki xoloki self-assigned this May 7, 2025
@xoloki xoloki added this to sBTC May 7, 2025
@xoloki xoloki added this to the Audit and Hardening milestone May 7, 2025
@github-project-automation github-project-automation bot moved this to Needs Triage in sBTC May 7, 2025
@djordon djordon moved this from Needs Triage to In Progress in sBTC May 8, 2025
@xoloki xoloki marked this pull request as ready for review May 8, 2025 15:39
@xoloki xoloki requested review from djordon and jferrant May 8, 2025 15:40
@xoloki xoloki moved this from In Progress to In Review in sBTC May 8, 2025
Comment on lines +722 to +723
// signer_id reported missing shares but they were present
self.malicious_dkg_signer_ids.insert(*signer_id);
Copy link
Copy Markdown
Contributor

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

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.

@xoloki xoloki force-pushed the missing-dkg-shares-malicious branch from 500afb1 to 6c212ff Compare May 14, 2025 17:13
@xoloki xoloki force-pushed the missing-dkg-shares-malicious branch from 6c212ff to 81e34b3 Compare May 15, 2025 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[GENERAL] OS-STS-SUG-00 | Code Maturity

2 participants