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

Replace this.sender API with something that is secure by default #1425

Closed
mitschabaude opened this issue Feb 10, 2024 · 0 comments · Fixed by #1464
Closed

Replace this.sender API with something that is secure by default #1425

mitschabaude opened this issue Feb 10, 2024 · 0 comments · Fixed by #1464
Labels
breaking Issues that will lead to breaking changes v1 Prerequisite for o1js v1.0

Comments

@mitschabaude
Copy link
Member

mitschabaude commented Feb 10, 2024

Why

this.sender is an unconstrained value. The prover can choose its value. I originally thought of it as a convenient shortcut to letting a user that creates a transaction pass in their address. Also, this behaviour is documented: https://docs.minaprotocol.com/zkapps/o1js-reference/classes/SmartContract#sender

But I severely underestimated how important it is for many applications that they are interacting with some particular address. In this scenario, the natural pattern is to compare this.sender with the address that you require:

this.sender.assertEquals(addressAllowedToInteract);

Which is a critical security bug. That I have seen multiple developers create in the wild.

I also understand now that documentation can't be enough to prevent this kind of usage. After all, our entire goal is to make o1js be intuitive and discoverable, so that you don't always have to look up the docs. If the intuitive usage of an API is insecure, the API must change.

How

The way to use this.sender securely is to also require a signature by the sender:

AccountUpdate.createSigned(this.sender);

That way, while it's not guaranteed that this.sender is really the transaction's fee payer, it is guaranteed that the transaction's creator owns the this.sender account, which seems enough.

One idea is to keep the this.sender API and always let it add that extra account update. However, I think we shouldn't do that, because account updates have a cost -- there's a limit to them -- and because we don't like magic behaviour.

Instead, I propose to remove this.sender and add two different APIs which cover the two use cases for it:

  • this.sender.getAndRequireSignature() -- explicitly require a signature, for use cases where you really need to know that the sender owns this account
  • this.sender.getUnconstrained() -- explicitly get the sender as an unconstrained value, for (the likely majority of) zkapps where you could just as well accept the "sender" as method input, and its fine for the user to choose an address they don't actually own, if they want
@mitschabaude mitschabaude added the breaking Issues that will lead to breaking changes label Feb 10, 2024
@mitschabaude mitschabaude changed the title Replace footgun this.sender API with something that is secure by default Replace this.sender API with something that is secure by default Feb 10, 2024
@garwalsh garwalsh added the v1 Prerequisite for o1js v1.0 label Feb 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Issues that will lead to breaking changes v1 Prerequisite for o1js v1.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants