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

Sprint 1 - Report by nullity #8

Open
nullity00 opened this issue Jun 14, 2023 · 1 comment
Open

Sprint 1 - Report by nullity #8

nullity00 opened this issue Jun 14, 2023 · 1 comment

Comments

@nullity00
Copy link
Contributor

nullity00 commented Jun 14, 2023

yAcademy - Rate Limiting Nullifier Review

Review Resources:

Auditors:

Table of Contents

Review Summary

Rate Limiting Nullifier

RLN (Rate-Limiting Nullifier) is a zk-gadget/protocol that enables spam prevention mechanism for anonymous environments.

The circuits of RLN were reviewed over 15 days. The code review was performed between 31st May and 14th June, 2023. The repository was under active development during the review, but the review was limited to the latest commit at the start of the review. This was commit 37073131b9 for the circom-rln repo.

Scope

The scope of the review consisted of the following circuits at the specific commit:

  • rln.circom
  • utils.circom
  • withdraw.circom

After the findings were presented to the RLN 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, RLN and users of the contracts agree to use the code at their own risk.

Code Evaluation Matrix

Category Mark Description
Cryptography Good To hash the secret values, Poseidon hash function has been used. This uses fewer constraints per bit compared to other functions lowering down the time consumed
Libraries Good The circuits use the defacto circomlib which has been audited multiple times
Circuit Dependence Graph Good The signals in the circuit are properly constrained with a well formed CDG
Documentation Low The documentation is outdated and needs refactoring
Proof Systems Good The docs recommend generating proofs using Groth16 using a BN254 curve, which has security level of 128 bits

Findings Explanation

Findings are broken down into sections by their respective impact:

  • Critical, High, Medium, Low impact
    • These are findings that range from attacks that may cause loss of funds, impact control/ownership of the contracts, or cause any unintended consequences/actions that are outside the scope of the requirements
  • Informational
    • Findings including recommendations and best practices

Critical Findings

None.

High Findings

None

Medium Findings

None.

Low Findings

1. Low - Incosistency between contract and the circuit on the number of bits for userMessageLimit

RLN.sol

uint256 messageLimit = amount / MINIMAL_DEPOSIT;

rln.circom

template RLN(DEPTH, LIMIT_BIT_SIZE) {
...
    // messageId range check
    RangeCheck(LIMIT_BIT_SIZE)(messageId, userMessageLimit);
...
}
component main { public [x, externalNullifier] } = RLN(20, 16);

In RLN.sol, the messageLimit can take upto 2**256 - 1 values whereas messageId & userMessageLimit values in circuits is restricted to 2**16 - 1 .

Recommended solution

  • RLN.sol
function register(uint256 identityCommitment, uint256 amount) external {
        ...
        uint256 messageLimit = amount / MINIMAL_DEPOSIT;
        require( messageLimit <= type(uint16).max , "Max amount of message limit is 65535");
        token.safeTransferFrom(msg.sender, address(this), amount);
        ...
    }

Informational findings

  • Missing range checks for x & externalNullifier
  • Restoring the polynomial for Shamir's Secret Sharing Scheme gives f(x) = 0 for same messages sent more than once . As in this case x1 = x2 = x3 .... The protocol does fall short of preventing spam here but would be a perfect fit for low-limit messaging platforms eg. voting.
  • The prime field in circom is ~ 2**254 whereas solidity supports 256-bit integers. There is a possibility of users registering using the identityCommitment & another hash A such that A mod p = identityCommitment where p is the prime field of circom. Here, the user registers twice using the same identitySecret.
@curryrasul
Copy link

Hi, thanks for your report!

Incosistency between contract and the circuit on the number of bits for userMessageLimit

That's a good find! I think we'll add this to the contract.

Missing range checks for x & externalNullifier

We don't need to check them in the circuit, and they cannot be not in the range as both values are result of Poseidon hash-function.

he prime field in circom is ~ 2**254 whereas solidity supports 256-bit integer ...

identityCommitment cannot be 256 bit, as it's the result of Poseidon hash-function that's defined over p (prime field of Circom/bn254)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants