fix(dpp): bind unshielding_amount to sighash in client builders#3362
fix(dpp): bind unshielding_amount to sighash in client builders#3362QuantumExplorer wants to merge 3 commits intov3.1-devfrom
Conversation
The server-side proof verification (shielded_proof.rs) binds both the destination and unshielding_amount to the Orchard sighash, but the client-side builders only bound the destination. This mismatch caused every Unshield and ShieldedWithdrawal transition built with rs-dpp or rs-sdk-ffi to fail proof verification on the platform. Fixed in 4 locations: - rs-dpp builder: unshield.rs, shielded_withdrawal.rs - rs-sdk-ffi FFI: bundle_build.rs (unshield + withdrawal paths) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughExtra sighash input now includes the unshielding/withdrawal amount (little-endian) appended to existing output bytes in the shielded bundle builders; a security audit document for the shielded pool was also added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rs (1)
847-850: Consider centralizing extra_sighash_data packing.This byte-layout logic is now duplicated across unshield/withdrawal paths; a small helper (e.g.,
fn pack_extra_sighash_data(prefix: &[u8], required: u64) -> Vec<u8>) would reduce future mismatch risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rs` around lines 847 - 850, Extract the byte-packing logic that builds extra_sighash_data (currently: output_address.to_bytes() then extend with required.to_le_bytes()) into a small helper like fn pack_extra_sighash_data(prefix: &[u8], required: u64) -> Vec<u8>, place it next to the related crypto helpers, and replace the inline construction in bundle_build.rs (and the duplicate in the unshield/withdrawal code paths and shielded_proof.rs if present) to call pack_extra_sighash_data(&output_address.to_bytes(), required) so all callers use the same implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rs`:
- Around line 847-850: Extract the byte-packing logic that builds
extra_sighash_data (currently: output_address.to_bytes() then extend with
required.to_le_bytes()) into a small helper like fn
pack_extra_sighash_data(prefix: &[u8], required: u64) -> Vec<u8>, place it next
to the related crypto helpers, and replace the inline construction in
bundle_build.rs (and the duplicate in the unshield/withdrawal code paths and
shielded_proof.rs if present) to call
pack_extra_sighash_data(&output_address.to_bytes(), required) so all callers use
the same implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab341bda-5b65-4418-a835-e1f5460960da
📒 Files selected for processing (3)
packages/rs-dpp/src/shielded/builder/shielded_withdrawal.rspackages/rs-dpp/src/shielded/builder/unshield.rspackages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3362 +/- ##
=========================================
Coverage 75.87% 75.87%
=========================================
Files 2912 2912
Lines 283860 283934 +74
=========================================
+ Hits 215375 215438 +63
- Misses 68485 68496 +11
🚀 New features to boost your workflow:
|
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "82514d02092e595b39772d5a59bbbda0f216813ed0396721dbe4f4a17ad664d2"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Correct and well-targeted security fix. The PR consistently appends required.to_le_bytes() (= amount + fee = value_balance = state transition's unshielding_amount) to extra_sighash_data in all 4 client-side bundle builders, aligning them with the server-side verification. The codex-general blocking findings about the FFI two-step flow are false positives — the referenced code is unchanged by this PR, and the fix strictly improves a previously always-broken flow. Main gaps are test coverage and documentation.
Reviewed commit: 09a2ad7
🟡 3 suggestion(s) | 💬 2 nitpick(s)
4 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk-ffi/src/shielded/transitions/builders.rs`:
- [SUGGESTION] lines 136-194: FFI two-step flow doesn't expose computed `required` to callers
The bundle builders (`build_unshield_bundle`, `build_withdrawal_bundle`) sign with `required = amount + fee`, but the returned `DashSDKOrchardBundleParams` struct doesn't include `value_balance`. The transition builders (`build_unshield`, `build_withdrawal`) and broadcast paths (`unshield_funds`, `withdraw_shielded`) take a separate `unshielding_amount` parameter that must equal `required` for verification to pass. A caller naturally passing the same net amount to both steps would get a sighash mismatch.
This is a pre-existing API design concern, not a regression from this PR (before the fix, the flow always failed). However, this PR makes it actionable — consider either exposing `value_balance` in the bundle params struct or adding a `debug_assert` in `try_from_bundle` that the passed amount matches the bundle's implicit value_balance.
In `packages/rs-dpp/src/shielded/builder/unshield.rs`:
- [SUGGESTION] lines 113-193: No happy-path test verifying extra_sighash_data construction
Both `unshield.rs` and `shielded_withdrawal.rs` only test error paths (fee below minimum, insufficient funds). There is no test that constructs the bundle and verifies the `extra_sighash_data` includes `output_address || required.to_le_bytes()` matching the server expectation. A round-trip test (build → serialize → reconstruct_and_verify) would prevent regression of this exact class of mismatch.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_common/mod.rs`:
- [SUGGESTION] lines 65-68: Doc comment omits ShieldedWithdrawal extra_sighash_data format
The doc comment for `reconstruct_and_verify_bundle` documents the `extra_sighash_data` format for Shield, ShieldedTransfer, and Unshield but omits ShieldedWithdrawal. Since this exact omission pattern contributed to the original mismatch, completing the documentation reduces future regression risk.
| // Unshield extra_data = output_address || unshielding_amount (le bytes) | ||
| // Must match server-side sighash in shielded_proof.rs | ||
| let mut extra_sighash_data = output_address.to_bytes(); | ||
| extra_sighash_data.extend_from_slice(&required.to_le_bytes()); |
There was a problem hiding this comment.
💬 Nitpick: Comment says 'unshielding_amount' but code binds required (amount + fee)
The comment // Unshield extra_data = output_address || unshielding_amount (le bytes) is semantically correct (the state transition field unshielding_amount equals required), but a reader unfamiliar with the invariant might think a different variable is being bound. Same pattern appears in shielded_withdrawal.rs:91-94 and both FFI locations in bundle_build.rs:847-850,1022-1025.
| // Unshield extra_data = output_address || unshielding_amount (le bytes) | |
| // Must match server-side sighash in shielded_proof.rs | |
| let mut extra_sighash_data = output_address.to_bytes(); | |
| extra_sighash_data.extend_from_slice(&required.to_le_bytes()); | |
| // Unshield extra_data = output_address || value_balance (le bytes) | |
| // value_balance = unshield_amount + fee, becomes v0.unshielding_amount in the state transition | |
| let mut extra_sighash_data = output_address.to_bytes(); | |
| extra_sighash_data.extend_from_slice(&required.to_le_bytes()); |
source: ['claude-general', 'claude-ffi-engineer']
Clarify that `required` = value_balance = unshield_amount + fee, which becomes v0.unshielding_amount in the state transition. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
SECURITY_AUDIT_ZK_2026_03_17.md (2)
19-23: Consider noting that line references reflect pre-fix state.The line numbers (e.g.,
unshield.rs:84,bundle_build.rs:848,1021) reference the code before this PR's fix is applied. Since this document is committed alongside the fix, readers may be confused when line numbers don't match. Consider either:
- Updating line numbers to reflect post-fix locations, or
- Adding a note that "Line references are from the original audit commit (95bdf2c) before the fix was applied."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SECURITY_AUDIT_ZK_2026_03_17.md` around lines 19 - 23, The document references pre-fix line numbers for several files (e.g., rs-dpp/src/shielded/builder/unshield.rs, rs-dpp/src/shielded/builder/shielded_withdrawal.rs, rs-sdk-ffi/src/shielded/crypto/bundle_build.rs, and the server trait in rs-drive-abci) which will mismatch after the PR; update the audit file to either adjust the line numbers to their post-fix locations or add a clear note such as "Line references are from the original audit commit (95bdf2c08) before the fix was applied" so readers can map references correctly.
27-30: Add language identifier to fenced code block.The code block on lines 27-30 lacks a language specifier. Since this is pseudocode/notation rather than actual code, consider using
textorplaintextto satisfy linters and improve rendering consistency.📝 Proposed fix
-``` +```text Client (signing): sighash = SHA256(domain || commitment || output_address) Server (verifying): sighash = SHA256(domain || commitment || output_address || unshielding_amount_le_bytes)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@SECURITY_AUDIT_ZK_2026_03_17.mdaround lines 27 - 30, The fenced code block
containing the sighash pseudocode ("Client (signing): sighash = SHA256(domain
|| commitment || output_address)" and "Server (verifying): sighash =
SHA256(domain || commitment || output_address || unshielding_amount_le_bytes)")
is missing a language identifier; update that triple-backtick block to include a
language tag such as text or plaintext (e.g., ```text) so linters and renderers
treat it as plain text.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In@SECURITY_AUDIT_ZK_2026_03_17.md:
- Around line 19-23: The document references pre-fix line numbers for several
files (e.g., rs-dpp/src/shielded/builder/unshield.rs,
rs-dpp/src/shielded/builder/shielded_withdrawal.rs,
rs-sdk-ffi/src/shielded/crypto/bundle_build.rs, and the server trait in
rs-drive-abci) which will mismatch after the PR; update the audit file to either
adjust the line numbers to their post-fix locations or add a clear note such as
"Line references are from the original audit commit (95bdf2c) before the fix
was applied" so readers can map references correctly.- Around line 27-30: The fenced code block containing the sighash pseudocode
("Client (signing): sighash = SHA256(domain || commitment || output_address)"
and "Server (verifying): sighash = SHA256(domain || commitment || output_address
|| unshielding_amount_le_bytes)") is missing a language identifier; update that
triple-backtick block to include a language tag such as text or plaintext (e.g.,
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID:
30bf0d8b-d9ce-43cd-a37a-9dbfe0845a4e📒 Files selected for processing (4)
SECURITY_AUDIT_ZK_2026_03_17.mdpackages/rs-dpp/src/shielded/builder/shielded_withdrawal.rspackages/rs-dpp/src/shielded/builder/unshield.rspackages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rs🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-dpp/src/shielded/builder/shielded_withdrawal.rs
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Trivial incremental: improved sighash comments to use 'value_balance' terminology per prior review feedback, and removed an accidentally committed security audit document. All changes are comment-only — no logic modifications.
Reviewed commit: 6414576
2 participants
Issue Being Fixed
Security audit identified a critical sighash mismatch between client-side Orchard bundle signing and server-side proof verification for Unshield and ShieldedWithdrawal transitions.
Server (
shielded_proof.rs): bindsoutput_address || unshielding_amountto the sighashClient (builders + FFI): only bound
output_address— missing theunshielding_amountThis caused every Unshield and ShieldedWithdrawal transition built with
rs-dpporrs-sdk-ffito fail proof verification on the platform. Integration tests passed because they construct bundles with the server-side sighash computation directly.What was done
Appended
unshielding_amount.to_le_bytes()toextra_sighash_datain all 4 client-side locations:rs-dpp/src/shielded/builder/unshield.rs— DPP builderrs-dpp/src/shielded/builder/shielded_withdrawal.rs— DPP builderrs-sdk-ffi/src/shielded/crypto/bundle_build.rs— FFI unshield pathrs-sdk-ffi/src/shielded/crypto/bundle_build.rs— FFI withdrawal pathThe
requiredvariable (unshield_amount + fee) equalsvalue_balancewhich becomesunshielding_amountin the state transition — exactly what the server expects.Test plan
cargo test -p dpp --features shielded-client -- builder)cargo check -p dpp && cargo check -p rs-sdk-ffi)🤖 Generated with Claude Code
Summary by CodeRabbit