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

Security and zkApps #921

Merged
merged 13 commits into from
May 16, 2024
Merged

Security and zkApps #921

merged 13 commits into from
May 16, 2024

Conversation

mitschabaude
Copy link
Contributor

@mitschabaude mitschabaude commented May 1, 2024

This adds a new section under "Creating zkApps", called "Security and zkApps".

Topics covered:

  • Advice for auditing zkApps
  • Attack model to understand
  • Example of introducing vulnerabilities in a zkApp
  • List of best security practices

I didn't get to write a final section on considerations in low-level circuit writing. This is its whole own can of worms and will need some work to present well.

In addition, this adds a small section on the o1js intro page to host our internal audit and future audits of o1js.

Copy link

vercel bot commented May 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 16, 2024 0:34am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
07-oracles ⬜️ Ignored (Inspect) Visit Preview May 16, 2024 0:34am

@mitschabaude mitschabaude changed the title Writing secure zkapps Security and zkApps May 9, 2024
@mitschabaude mitschabaude marked this pull request as ready for review May 9, 2024 13:34
@mitschabaude mitschabaude requested a review from a team as a code owner May 9, 2024 13:34
@@ -43,7 +43,7 @@ function knowsPreimage(preimage: Field) {
}

const expectedHash =
Field(0x1d444102d9e8da6d566467defcc446e8c1c3a3616d059facadbfd674afbc37ecn);
0x1d444102d9e8da6d566467defcc446e8c1c3a3616d059facadbfd674afbc37ecn;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
0x1d444102d9e8da6d566467defcc446e8c1c3a3616d059facadbfd674afbc37ecn;
'0x1d444102d9e8da6d566467defcc446e8c1c3a3616d059facadbfd674afbc37ecn';

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, bigint literal!


### Creating an insecure contract

We need to use either `assertCanMint()` or `assertCanBurn()`, but how do we know which one? Well, let's just add a parameter to the method that tells us whether this is a mint or a burn. Then let's call the appropriate method based on that parameter. Github Copilot fills this out nicely for us:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Github Copilot fills this out nicely" - LOL

@stevenpack
Copy link
Contributor

Screenshot 2024-05-14 at 12 05 56 PM

One ask from Evan

@stevenpack
Copy link
Contributor

Otherwise, LGTM

@mitschabaude mitschabaude merged commit 1ac0e70 into main May 16, 2024
4 checks passed
@mitschabaude mitschabaude deleted the docs/secure-zkapps branch May 16, 2024 12:35
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

Successfully merging this pull request may close these issues.

4 participants