-
-
Notifications
You must be signed in to change notification settings - Fork 23
FIX + IMPROV: Negative Hops Amplification Attack Fix, validateMessage improvements, Weak PoW Mitigation. #33
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses critical security vulnerabilities in the P2P messaging system, including a negative hops amplification attack, loose message validation, and Sybil attack vectors through weak proof-of-work.
- Fixes negative hops relay amplification by validating
hops >= 0before relaying messages - Strengthens message validation with strict type checking and integer validation for all numeric fields
- Implements per-connection rate limiting (1000 new peers per 5 seconds) to mitigate Sybil attacks exploiting weak PoW
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/config/constants.js | Adds rate limiting constants (RATE_LIMIT_WINDOW and RATE_LIMIT_MAX_NEW_PEERS) for Sybil attack mitigation |
| src/state/diagnostics.js | Adds rateLimitedConnections counter to track rate limit enforcement |
| src/p2p/messaging.js | Implements negative hops validation, per-socket rate limiting for new peers, and enhanced validateMessage with strict type checks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sending the id as an array of integers instead of the hex string creates valid messages with valid signatures btw because adding array + string (for the id + nonce hash) is a valid javascript thing to do and Those arrays are used as Map keys and could break some stuff (because they hash by object, not content), but that currently does not work because HyperLogLog fails when calling |
Negative Hops Relay Amplification
Relay condition validation allowed negative values, by sending -100 you can cause 102 relay hops. Observed during testing that setting -100 would cause a massive increase in duplicate messages received back from testnet.
validateMessage is loosely validating
This one doesn't seem impacting but adding additional validation here seems good. Could reduce performance impact of attempted abuse by rejecting earlier.
Weak PoW Mitigation
Assuming you can't change the PoW to 00000 from 0000, I was able to generate 100,000 Identities and broadcast these into a testnet easily with a cheap VPS.
An easy win fix here seems to be introducing a rate limit on the number of peers accepted over a single connection, This is a suggestion and could be tailored to network rate. Setting this to 1000 new peers over a 5s window limit the impact without raising the PoW or currently limiting the dopamine network.
Sybil attack PoC for this can be seen in base branch of my fork repo as
troll_parallel_v2.jsGive all of these a proper test, I'm terrible at Javascript.