-
Notifications
You must be signed in to change notification settings - Fork 425
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
fix: Groth16Verifier solidity scalar size check. (security vulnerability) #480
Merged
OBrezhniev
merged 1 commit into
iden3:master
from
Stumble:yumin/fix-groth16-verifier-solidity-tmpl
Apr 8, 2024
Merged
fix: Groth16Verifier solidity scalar size check. (security vulnerability) #480
OBrezhniev
merged 1 commit into
iden3:master
from
Stumble:yumin/fix-groth16-verifier-solidity-tmpl
Apr 8, 2024
Conversation
This file contains 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
The Groth16Verifier contract is not correctly generating codes to checking that public signals that with the scalar field's range. Signals should be less than r instead of q in the contract's context.
Stumble
changed the title
fix: Groth16Verifier solidity scalar size check.
fix: Groth16Verifier solidity scalar size check. (security vulnerability)
Apr 5, 2024
9 tasks
xavi-pinsach
approved these changes
Apr 8, 2024
arnaucube
added a commit
to privacy-scaling-explorations/sonobe
that referenced
this pull request
Apr 9, 2024
arnaucube
added a commit
to privacy-scaling-explorations/sonobe
that referenced
this pull request
Apr 11, 2024
arnaucube
added a commit
to privacy-scaling-explorations/sonobe
that referenced
this pull request
Apr 12, 2024
arnaucube
added a commit
to privacy-scaling-explorations/sonobe
that referenced
this pull request
Apr 14, 2024
arnaucube
added a commit
to privacy-scaling-explorations/sonobe
that referenced
this pull request
Apr 16, 2024
arnaucube
added a commit
to privacy-scaling-explorations/sonobe
that referenced
this pull request
Apr 16, 2024
arnaucube
added a commit
to privacy-scaling-explorations/sonobe
that referenced
this pull request
Apr 17, 2024
github-merge-queue bot
pushed a commit
to privacy-scaling-explorations/sonobe
that referenced
this pull request
Apr 25, 2024
* Add solidity verifier of the nova+cyclefold, and add method to prepare the calldata from Decider's proof. Missing conversion of the point coordinates into limbs (ark compatible) * chore: adding comments linking to the contract's signature * chore: update .gitignore * chore: add num-bigint as dev dependency * fix: work with abs path for storing generated sol code * chore: update comment * feat: solidity verifier working on single and multi-input circuits * feat: multi-input folding verification working + fixing encoding of additive identity in calldata * chore: make bigint a dependency * refactor: import utils functions from utils.rs and make them available from anywhere * chore: make utils and evm available publicly * fix: pub mod instead * chore: make relevant method public and add `get_decider_template_for_cyclefold_decider` to exported objects * solidity-verifiers: move tests to their corresponding files * small update: Cyclefold -> CycleFold at the missing places * abstract nova-cyclefold solidity verifiers tests to avoid code duplication, and abstract also the computed setup params (FS & Decider) to compute them only once for all related tests to save test time * small polish after rebase to last main branch changes * rm unneeded Option for KZGData::g1_crs_batch_points * add checks modifying z_0 & z_i to nova_cyclefold_solidity_verifier test * add light-test feature to decider_eth_circuit to use it in solidity-verifier tests without the big circuit * solidity-verifiers: groth16 template: port the fix from iden3/snarkjs#480 & iden3/snarkjs#479 * add print warning msg for light-test in DeciderEthCircuit * solidity-verifiers: update limbs logic to nonnative last version, parametrize limbs params solidity-verifiers: * update solidity limbs logic to last nonnative impl version, and to last u_i.x impl * parametrize limbs params * add light-test feature: replace the '#[cfg(not(test))]' by the 'light-test' feature that by default is not enabled, so when running the github actions we enable the feature 'light-tests', and then we can have a full-test that runs the test without the 'light-tests' flag, but we don't run this big test every time. The choice of a feature is to allow us to control this from other-crates tests (for example for the solidity-verifier separated crate tests, to avoid running the full heavy circuit in the solidity tests) * move solidity constants into template constants for auto compute of params * polishing * revm use only needed feature This is to avoid c depencency for c-kzg which is behind the c-kzg flag and not needed. * nova_cyclefold_decider.sol header * rearrange test helpers position, add error for min number of steps * in solidity-verifiers: 'data'->'vk/verifier key' * add From for NovaCycleFoldVerifierKey from original vks to simplify dev flow, also conditionally template the batchCheck related structs and methods from the KZG10 solidity template --------- Co-authored-by: dmpierre <pdaixmoreux@gmail.com>
Hello @Stumble! We just released version of snarkjs with your fix merged! Thank you for the contribution! |
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.
The Groth16Verifier contract is not correctly generating codes to checking that public signals are within the scalar field's range. Signals should be less than
r
instead ofq
in the contract's context.See #479
EDIT:
This may actually be a security vulnerability for small public signals that is less than
q - r
. And becauseq-r = 147946756881789318990833708069417712966
is not that small, many use cases may fall into this range.Exploit:
A simple circom circuit accepting a value less than 100.
The generated solidity code's verifyProof function returns true for both
1
and1 + r
as the public input, falsely claiming that1 + 21888242871839275222246405745257275088548364400416034343698204186575808495617
is less than 100.I can image that, without adding additional (correct) checkings, contracts may be vulnerable to this issue, when public signals are used as proof of balance, age or anything that is not random enough.
You can test it on Remix. The original and aliased proof will be accepted,
Original proof
Aliased proof
Generated solidity contract: