Open markus0x1 opened 1 year ago
Hi, thanks for your report!
Unused public inputs ban be optimized out
Good find! We'll add this "dummy constraint" to the withdraw circuit.
Specification uses incorrect definition of identity commitment
Spec/docs is out of date, we'll update it soon. Thanks for your review.
yAcademy Rate-Limit Nullifier Review
Review Resources:
Repo
Docs
Specs
Auditors:
Table of Contents
Review Summary
Rate-Limit Nullifier
The main goal of RLN v2 circuits is to make it possible to have a custom amount of messages (signals) per epoch without using a separate circuit or high-degree polynomials for Shamir's Secret Sharing.
The circuits of the Rate-Limit Nullifier Github were reviewed over 15 days. The code review was performed by 1 auditor between 31st May, 2023 and 14th June, 2023. The repository was static during the review.
Scope
The scope of the review consisted of the following circuits within the repo:
The scope of the review consisted of the following contracts at the specific commit:
37073131b9c5910228ad6bdf0fc50080e507166a
After the findings were presented to the Rate-Limit Nullifier team, fixes were made and included in several PRs.
This review is a code review to identify potential vulnerabilities in the code. The reviewers did not investigate security practices or operational security and assumed that privileged accounts could be trusted. The reviewers did not evaluate the security of the code relative to a standard or specification. The review may not have identified all potential attack vectors or areas of vulnerability.
yAcademy and the auditors make no warranties regarding the security of the code and do not warrant that the code is free from defects. yAcademy and the auditors do not represent nor imply to third parties that the code has been audited nor that the code is free from defects. By deploying or using the code, Rate-Limit Nullifier and users of the contracts agree to use the code at their own risk.
Code Evaluation Matrix
Findings Explanation
Findings are broken down into sections by their respective impact:
Low Findings
REPORTED BY markus, elpacos:
1. Low - Unused public inputs ban be optimized out
As described in the 0xParc ZK Bug Tracker the circom optimizer can remove public inputs that are unused.
Technical Details
The
Withdraw
circuit has a public inputaddress
that is not used in any constraints. Hence, the circom optimizer might remove this variable. But the address has to be part of the proof to prevent users from front-running a withdraw transaction.Impact
Low. Most libraries (snarkjs, arkworks) create constraints for all public inputs. We were unable to replicate this bug with snarkjs and arkworks.
Recommendation
Add a dummy constraint that uses the public input
Developer Response
2. Low - Specification uses incorrect definition of identity commitment
REPORTED BY markus:
The V2 Specification uses the
identity_secret
to compute theidentity_commitment
instead of theidentity_secret_hash
. Theidentity_secret
is already used by the Semaphore circuits and should not get revealed in a Slashing event.Technical Details
RLN stays compatible with Semaphore circuits by deriving the secret ("
identity_secret_hash
") as the hash of the semaphore secretsidentity_nullifier
andidentity_trapdoor
.RLN V2 improves upon the V1 Protocol by allowing to set different rate-limits for users. Hence, the definition of the user identity changes from the V1 definition:
The RLN-Diff flow wrongfully derives the
identity_commitment
from theidentity_secret
directly instead of theidentity_secret_hash
.Impact
Medium. Using the
identity_secret
as secret value is problematic since a slasher can now compromise the semaphore identity. The official sdk implements the correct definition of the identity commitment. But an incorrect specification can lead to future implementation bugs.Recommendation
Short term:
Modify the following part of the V2 Specification:
Long-term:
Rename the variable
identity_secret
in the circuit to avoid further confusion with a variable of the same name derived from Semaphore.Developer Response