Skip to content
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

Missing checks to ensure 'zero' is less than SNARK_SCALAR_FIELD #23

Closed
kcharbo3 opened this issue Mar 7, 2022 · 0 comments · Fixed by #24
Closed

Missing checks to ensure 'zero' is less than SNARK_SCALAR_FIELD #23

kcharbo3 opened this issue Mar 7, 2022 · 0 comments · Fixed by #24
Assignees
Labels
bug 🐛 Something isn't working

Comments

@kcharbo3
Copy link

kcharbo3 commented Mar 7, 2022

Description
During initialization of the IncrementalBinaryTree.sol, the user can enter any value for the uint256 zero field. This becomes an issue if the user uses a value greater than the SNARK_SCALAR_FIELD. A zero leaf can be inside an array of proofSiblings when proving existence of a leaf which may cause an issue when the IncrementalBinaryTree.verify function is called during the IncrementalBinaryTree.remove function. The verify function requires that all proofSiblings are less than the SNARK_SCALAR_FIELD:
require(proofSiblings[i] < SNARK_SCALAR_FIELD, "IncrementalBinaryTree: sibling node must be < SNARK_SCALAR_FIELD" );
https://github.com/appliedzkp/zk-kit/blob/main/packages/incremental-merkle-tree.sol/contracts/IncrementalBinaryTree.sol#L127

So, if the 'zero' leaf is greater than the SNARK_SCALAR_FIELD, this verify function will unintentionally fail.

Possible Fix
Add a require(zero < SNARK_SCALAR_FIELD, "...") statement in the IncrementalBinaryTree.init() function.
https://github.com/appliedzkp/zk-kit/blob/main/packages/incremental-merkle-tree.sol/contracts/IncrementalBinaryTree.sol#L29

@cedoor cedoor self-assigned this Mar 9, 2022
@cedoor cedoor added the bug 🐛 Something isn't working label Mar 9, 2022
@cedoor cedoor linked a pull request Mar 9, 2022 that will close this issue
13 tasks
@cedoor cedoor closed this as completed Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants