-
Notifications
You must be signed in to change notification settings - Fork 254
SRLabs: Introduce fuzzing harness for pallet-domains #3693
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
🛡️ Immunefi PR ReviewsWe noticed that your project isn't set up for automatic code reviews. If you'd like this PR reviewed by the Immunefi team, you can request it manually using the link below: Once submitted, we'll take care of assigning a reviewer and follow up here. |
@R9295 thank you for this PR! It will likely not get fully reviewed until early next week. In the meantime could you please cleanup the commit history. See our contribution guide for what we are expecting. Thanks! |
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.
Thank you for this PR, it's an important part of our security testing.
I've made a large number of comments, but most of them are code style nitpicks (starting with Nit:
). Feel free to ignore those, one of the developers can do them later.
The most important revisions are:
- adding comments to new functions and structs, so reviewers know what they're meant to do
- reducing the size of the PR by doing re-exports in the standard Rust way
crates/pallet-domains/Cargo.toml
Outdated
"pallet-subspace/runtime-benchmarks", | ||
"schnorrkel", | ||
"hex-literal", |
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.
Nit: please keep dependencies in alphabetical order, and add any new dependencies in order
(This was likely an accidental revert in a merge commit.)
crates/pallet-domains/Cargo.toml
Outdated
domain-pallet-executive = {workspace = true, optional = true} | ||
pallet-timestamp = {workspace = true, optional = true} | ||
pallet-block-fees = {workspace = true, optional = true} | ||
prop-test = {workspace = true, optional = true} |
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.
This dependency is not part of the fuzz feature. This seems like a copy-paste mistake?
@@ -35,6 +35,12 @@ sp-subspace-mmr.workspace = true | |||
sp-version = { workspace = true, features = ["serde"] } | |||
subspace-core-primitives.workspace = true | |||
subspace-runtime-primitives.workspace = true | |||
domain-pallet-executive = {workspace = true, optional = true} |
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.
A new section would make this clearer:
domain-pallet-executive = {workspace = true, optional = true} | |
# fuzz feature optional dependencies | |
domain-pallet-executive = {workspace = true, optional = true} |
@@ -0,0 +1,216 @@ | |||
// Copyright 2025 Security Research Labs GmbH |
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.
The rest of the crate is 0BSD, do you mind updating this copyright to match?
https://opensource.org/license/0bsd
license = "0BSD" |
I also have the same licence ask for the fuzzing crate.
use sp_core::H256; | ||
use sp_domains::{DomainId, OperatorId}; | ||
use sp_runtime::traits::One; | ||
|
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.
Nit: Most of the crate uses a single import block. This might require a rustfmt run after this change.
fuzz/staking/src/main.rs
Outdated
println!("skipping NominateOperator"); | ||
continue; | ||
} | ||
let old_amount = amount.min(&21); |
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.
What does this do, and why is it needed?
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.
Good catch, it should be amount.max
fuzz/staking/src/main.rs
Outdated
println!("skipping WithdrawStake"); | ||
continue; | ||
} | ||
// TODO: |
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.
Nit: remove this or complete it?
.1; | ||
let slash_reason = match slash_reason % 2 { | ||
0 => SlashedReason::InvalidBundle(0), | ||
_ => SlashedReason::BadExecutionReceipt(H256::from([0u8; 32])), |
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.
Nit: I try to avoid wildcard matches, so the compiler will tell us that we need to update them if the surrounding code changes. What we really want is a compiler error when there is a new SlashedReason
.
The standard way of doing it would be:
- add an enum without fields that matches each existing SlashedReason, and converts to a dummy SlashedReason
- add a function that converts a u8 to that enum
- convert from the u8 to the fieldless enum to the dummy SlashedReason here
fuzz/staking/src/main.rs
Outdated
let config = OperatorConfig { | ||
signing_key: pair.public(), | ||
minimum_nominator_stake, | ||
nomination_tax: sp_runtime::Percent::from_percent(60), |
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.
Is this deliberately different from the DomainsConfig above?
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.
Good catch, we wanted the fuzzer to mutate these values too. I will update it
.unwrap() | ||
} | ||
|
||
fn main() { |
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.
This fuzzing harness isn't tested in CI, so any other PR could accidentally break it. We've had similar issues with the runtime benchmarks in the past.
Things to do (edited):
- before this PR merges, add a single fuzzer run in CI, to make sure it works
- add it to the rust-all job
Here's how that works for benchmarks:
subspace/.github/workflows/rust.yml
Lines 329 to 331 in 3d0d436
# Checks benchmark code generation, and runs each benchmark once. | |
# Fails if code generation fails, benchmarks panic, or generated weights are not valid Rust. | |
check-runtime-benchmarks: |
Co-authored-by: teor <[email protected]>
This PR introduces a fuzzing harness for pallet-domains, encoding invariants and valid user-processes when fuzzing for maximum impact.
Please note that the changes seem large due to the use of the macro to make functions public conditionally.
Code contributor checklist: