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 Audit Report | curiousapple #3

Open
0xcuriousapple opened this issue Jun 14, 2023 · 1 comment
Open

Sprint-1 Audit Report | curiousapple #3

0xcuriousapple opened this issue Jun 14, 2023 · 1 comment

Comments

@0xcuriousapple
Copy link

0xcuriousapple commented Jun 14, 2023

yAcademy Rate-Limit Nullifier Review

Auditors:

  • curiousapple

Table of Contents

  1. Review Summary
  2. Scope
  3. Findings Explanation
  4. Critical Findings
  5. High Findings
  6. Medium Findings
    [M-1] Effective stake at risk is only fees and not the complete stake as intended
  7. Low Findings
    [L-1] Unused public inputs could be optimized out
    [L-2] Different bit size for messageLimit between circuits and contracts
  8. Quality Findings
    [Q-1] Unneccary import and inheritance of ownable inside RLN contract
  9. Informational Findings
    [I-1] By sending the same message continuously, users can still spam the network
    [I-2] The penalty enforced on the user doesn't scale with the magnitude of their spam
  10. Final remarks

Review Summary

Rate-Limit Nullifier

RLN intends to develop a layer for spam resistance in anonymous networks.
As a first step users are required to register themselves and stake, onchain.
Then users can continue to send messages as they do in anonymous networks, without any onchain interaction.
However, if they go above their limit, anyone can prove using ZK proof, that they did so and slash their stake.
RLN V1 was already in production, the current audit is for its V2 version
V2 introduced the novel feature of message limit per epoch, allowing users to send multiple messages per epoch

The circuits and contracts, both were reviewed over a total of 14 days.
The code review was performed by 1 auditor between 31st May and 14th June 2023.

Circum circuits are present at circom-rln, and latest commit 37073131b9c5910228ad6bdf0fc50080e507166a was considered for review.
Smart contracts are present at rln-contracts and latest commit dfcdb2beff91583d02636570242cfdd8469b61c1 was considered for review.

Scope

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

circom-rln

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

rln-contracts

  • RLN.sol

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, TODO_protocol_name and users of the contracts agree to use the code at their own risk.

Code Evaluation Matrix (Contracts)

Category Mark Description
Access Control NA NA
Mathematics NA NA
Complexity Low Is kept simple
Libraries NA -
Decentralization Good No owner controls
Code stability Good No change in scope during audit
Documentation Good -
Monitoring Good No delay for communication on channel
Testing and verification Low No tests for contracts

Code Evaluation Matrix (Circuits)

Category Mark Description
Completeness Satisfied -
Soundness Average Lack of explicit constraint for address in withdraw circuit
Zero-knowledge Satisfied -
Complexity Low Is kept simple
Code stability Good No change in scope during audit
Documentation Good -
Monitoring Good No delay for communication on channel
Testing and verification Good -

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
  • Gas savings
    • Findings that can improve the gas efficiency of the contracts
  • Informational
    • Findings including recommendations and best practices

Critical Findings

None.

High Findings

None.

Medium Findings

[M-1] Effective stake at risk is only fees and not the complete stake as intended

RLN contracts enforce users to stake and register themselves before being part of the network.
The idea here is if a user misbehaves and spams the network, anyone can slash them in return for their stake.
However, since networks are anonymous, users can slash themselves using another identity and only lose fees.

Impact

Users lose only part of the stake for their misbehavior

Recommendation

Consider using different incentive mechanism for slash.

Low Findings

[L-1] Unused public inputs could be optimized out

Withdraw circuit has address as public input with the intention of constraining address in proofs to prevent frontrunning.
However, it's not explicitly constrained.

Impact

Depending on the library used, this could lead to being optimized and not included in constraints, exposing the user's stake.

Recommendation

Consider constraining address explicitly

[L-2] Different bit size for messageLimit between circuits and contracts

RLN.sol allows messageLimit to scale upto 2 ** 256 -1, however, circuits enforce it upto 2 ** 16 -1 only.

Impact

If users stake with huge amount such that they satisfy the constraint of amount / MINIMAL_DEPOSIT >= 2 ** 16, their stake can not be slashed anymore

Recommendation

Consider adding an explicit check on bit size inside contracts as well.

Quality Findings

[Q-1] Unneccary import and inheritance of ownable inside RLN contract

Consider removing the ownable inheritance

Informational Findings

[I-1] By sending the same message continuously, users can still spam the network

It is true that if the users change their msg and go above the msg limit, they expose their secret, however, they can still send the same messages continuously.
The protocol team cleared that this is not an issue, since nodes will filter the same msg out.

[I-2] The penalty enforced on the user doesn't scale with the magnitude of their spam

There is no difference between a penalty for spamming once or spamming x1000 times.
Hence the cost of attack does not scale with the impact
All attacker needs to do is do a minimum deposit and then he/she can spam the network with any number of messages at once, and all they are penalized for is a minimum deposit.
This incentive structure puts an innocent and malicious rule breaker in the same bucket.

Final remarks

No critical or high issues were found.
For the medium one as well, we were informed that the protocol team is already aware of it.
Overall, the contracts and circuits are well-designed.

@curryrasul
Copy link

Hi, thanks for your report!

Unused public inputs could be optimized out

Good find! We'll add this to our circuit.

Different bit size for messageLimit between circuits and contracts

Good find! We'll add range check to the contract!

Unneccary import and inheritance of ownable inside RLN contract

Thanks! We should remove Ownable

The penalty enforced on the user doesn't scale with the magnitude of their spam

You cannot spam more than once. As soon as user exceed the limit, they can be slashed immediately (at least on the node level)

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