fix(shielded-pool): reject two equal non-dummy nullifiers in private_transfer - #127
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reject two equal non-dummy nullifiers in
private_transferDefense-in-depth against spending one input twice within a single transfer.
The gap
private_transfertakes up to two nullifiers. If both slots hold the same non-dummy nullifier:mark_as_usedruns per nullifier, but the second write is idempotent: marking an already-marked nullifier is a no-op.Nothing between those two points compared the nullifiers to each other, so one input was spent twice. Pool admission doesn't catch it either —
validate_unsignedpushes each nullifier intoand_provides, and two equal ones collapse to a single tag rather than flagging a duplicate.The ZK proof is expected to bind the two inputs distinct. This does not remove that expectation; it stops the chain from depending on it, so a circuit that ever failed to enforce distinctness could not translate into a double-spend on-chain.
The fix
After the used-set check in
execute, compare the non-dummy nullifiers directly. With at most two inputs, no set is needed: filter out the dummy (all-zero) nullifiers, and if two real ones remain, reject withNullifierAlreadyUsedwhen they are equal.Dummy inputs (all-zero) are excluded — two dummies are legitimately equal and already handled by the all-dummy rejection downstream.
Verification
Unit
execute_two_equal_nullifiers_fails— passes. Mutation-tested: neutralizing the guard makes the test fail (the transfer goes through), confirming it catches a real condition.Production build (node without
skip-proof-verification) compiles clean; clippy and fmt pass.Dev-node adversarial suite (3/3) via unsigned extrinsic, with the transfer VK loaded so the tx clears
validate_unsignedand actually reachesexecute:NullifierAlreadyUsedThe node runs
skip-proof-verificationon purpose: the guard sits behind the proof check, so this exercises the defense-in-depth layer on the assumption the proof did not bind the inputs distinct — exactly the case it protects against.Version
pallet-shielded-pool0.16.0→0.17.0; CHANGELOG entry under Security.