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

Allow contracts to be delegates of a Bouncer #1005

Closed
1 task done
shrugs opened this issue Jun 13, 2018 · 18 comments
Closed
1 task done

Allow contracts to be delegates of a Bouncer #1005

shrugs opened this issue Jun 13, 2018 · 18 comments

Comments

@shrugs
Copy link
Contributor

shrugs commented Jun 13, 2018

🎉 Description

continuing the discussion from #973

I want to explore the idea proposed by @PhABC where a contract can be the authority, but delegate authorizations to other arbitrary logic. This is a pattern that 0x v2 is also adopting for allowing the main exchange contract to delegate order acceptance logic to a contract.

  • 📈 This is a feature request.
@frangio
Copy link
Contributor

frangio commented Jun 13, 2018

Regarding the snippet from #973 (comment), particularly this part:

    const signer = signerOf(_sig)
    if (signer.isContract()) {
      // ERC165 detect and call on isValidSignature
      return signer.isValidSignature(hash, _sig)

How is the signerOf function implemented? It can't be ecrecover because contracts don't have private keys.

@frangio
Copy link
Contributor

frangio commented Jun 13, 2018

Hm, I suppose _sig becomes some other structure instead of an actual signature. Interesting... I need to think about this. As I mentioned in #950 though, I would really like to hear the scenarios where this setup would be used!

@shrugs
Copy link
Contributor Author

shrugs commented Jun 13, 2018

@frangio oh yeah, the signer would be the account as normal. and I the msg.sender is the contract itself. So it should be

    if (msg.sender.isContract()) {
      // ERC165 detect and call on isValidSignature
      return msg.sender.isValidSignature(hash, _sig);
    } else {
      return hasRole(signerOf(_sig), ROLE_BOUNCER);
    }

@PhABC
Copy link
Contributor

PhABC commented Jun 15, 2018

@frangio One example would be to have a "smart account" be an owner. A smart account is a contract that would hold all the assets of a user to facilitate a bunch of stuff and where different private key can perform different action. Some PV key could transfer assets and set permissions while others could simply play games or interact on social media on the behalf of this smart account. Smart accounts increase security and can add a bunch of critical functionalities to regular accounts in order to improve UX.

Now, if this smart account, which would actually be my main account, is owner of a contract, or has a given role is some bouncer protected contract, it will need to have this kind of method.

I have a strong feeling that most users will not use their regular account in the future, which offers very low security and poor UX and will most likely use a smart contract (that is, until account abstraction).

For more info about the future of accounts and the concept of smart accounts, check out Alex van de Sande's 15 mins presentation and then mine at the EDCON Ux unconference.

@shrugs
Copy link
Contributor Author

shrugs commented Jun 15, 2018

yeah, very much like ethereum/EIPs#725

@shrugs shrugs changed the title Allow contracts to be Invitees of a Bouncer and clean up signature construction Allow contracts to be Invitees of a Bouncer Jun 16, 2018
@frangio
Copy link
Contributor

frangio commented Jun 18, 2018

A quick thought. Isn't "delegating authorization to other arbitrary logic" already covered by being able to assign a role to any contract?

@shrugs
Copy link
Contributor Author

shrugs commented Jun 19, 2018

@frangio I suppose so, yeah; you can easily just compose signature bouncers together if you want to delegate logic like that. But I think the intention here is to remove the coupling between the signer and the "is this signer allowed to sign for this sender", to allow for identity contracts that are proxies with their own access control (like EIP725).

The only way for a bouncer contract to know if the signer is one of the approved addresses is to call back to this identity contract an ask. Perhaps it's possible to use the getKeysByPurpose method of 725 to get all of the action keys of an identity and see if it includes the signer of the voucher that was submitted to the bouncer? that way we avoid a custom method implementation. what do you think, @PhABC ?

@PhABC
Copy link
Contributor

PhABC commented Jun 21, 2018

A quick thought. Isn't "delegating authorization to other arbitrary logic" already covered by being able to assign a role to any contract?

Yes, but with the SignatureBouncer.sol, a bouncer need to use an ECDSA signature, however allowing for contract could support arbitrary signature schemes and more.

Perhaps it's possible to use the getKeysByPurpose method of 725 to get all of the action keys of an identity and see if it includes the signer of the voucher that was submitted to the bouncer?

I don't think this will be only useful for identities per say. It could be a contract that has multiple owners for example, a contract that has a lottery signer, where a new signer is randomly picked every X blocks without on-chain transactions, a validators subset in a proof of stake scheme or a PoW requirement provided with a signature. I'm giving random examples, but the general idea is a contract can have custom control mechanics that are not restricted to ECDSA signatures.

@frangio
Copy link
Contributor

frangio commented Jul 19, 2018

Let's try to think this through a bit better. I think we can converge at a better design if we first try to define the problem clearly. (Apologies if I repeat stuff expressed elsewhere or if I'm too verbose. 😅 It was quite hard for me to get the necessary context so I hope that this can make it easier for others interested in the discussion.)

SignatureBouncer currently allows a contract to validate that an ECDSA signature was created by one of a set of signers, and it allows to use valid signatures as authorization for function calls. This allows the authorization to happen off-chain, whereas something like RBAC requires first registering the prospective caller on-chain.

The feature request in this issue touches on two mostly orthogonal dimensions where SignatureBouncer can be generalized: 1) allowing signatures other than ECDSA, and 2) allowing external contracts to be validators. I say "mostly" because the second allows to trivially implement the first, but note that the 0x implementation deals with each separately: there are different types of signatures that can be validated internally, and additionally there are Wallets and Validators that can be invoked externally.

There is another concept lingering around here which is that of smart accounts or identity contracts. In fact 0x's Wallet seems to be an implementation of the same concept. The basic idea is that a user can interact with blockchain applications through a smart contract instead of an account backed by a key, and this allows interesting new possibilities such as having the fees paid by someone else (a service provider of sorts). I actually had not understood the full implications of this idea until i watched @PhABC's talk linked earlier in this thread. Very interesting stuff!

Whereas classical accounts are driven by ECDSA signed transactions, smart accounts are driven by the arbitrary logic of a smart contract. This is where it relates to the generalization of SignatureBouncer. SignatureBouncer validates ECDSA signatures, but smart accounts are not backed by a key and can never produce such signatures. Thus, a smart account can never be a bouncer, unless we generalize to other authentication schemes.

@shrugs @PhABC Hopefully by this point we're all in the same page! Now for some of my own thoughts on what to do with all this.

After thinking about it for a while, I'm completely sold on the idea of smart accounts and I want OpenZeppelin to have facilities for this type of authentication. However, I'm not sure if SignatureBouncer is the first place where we should include support for it, because its raison d'etre is definining a finite set of signers (the actual bouncers, also called delegates in #1024) that can authorize other accounts to execute functions on the contract. The signers are assumed to be under the control of the contract "owner". Smart accounts are orthogonal to this.

tl;dr: IMO, what needs to be generalized for smart accounts is ECRecovery (or something like it), which is used by SignatureBouncer, but not SignatureBouncer itself. I would close this issue and open a new one for that purpose. Only after figuring that out I would look into using those changes in SignatureBouncer.

I think the reason this was conflated is that SignatureBouncer was the first contract to establish a "protocol" on how to grab an arbitrary transaction's msg.data and validate that it was signed by an account. We may want to explore the design space in that direction. I have some concrete ideas on how to improve that protocol which we can discuss

@shrugs
Copy link
Contributor Author

shrugs commented Jul 19, 2018

Ah! The point about abstracting ECRecovery to support this is really smart!

An update on the language I'm using for the Bouncer:

  • bouncer is the contract itself that does the sig verification
  • the EOA or contract signer is a delegate
  • the signature itself is a ticket

So it seems like the code changes that need to take place are

  1. a new issue/PR for adding isValidSignature stuff to ECRecovery
  • 0x includes the delegate contract in the signature itself that's submitted aka it's not just v + r + s, it's something like delegateAddress + v + r + s. Current pattern in the feat: Bouncer refactor, test streamlining, and vocabulary updates #1024 is making the delegateAddress an additional argument in the function, but I think concatenating it might be a good move. Then calling it a ticket would make sense, since it's a new concept, not just a vrs signature.
  1. update feat: Bouncer refactor, test streamlining, and vocabulary updates #1024 to use this new ECRecovery code.

does that sound right?

thanks for your in-depth comment, @frangio !

@PhABC
Copy link
Contributor

PhABC commented Jul 20, 2018

Great summary @frangio! Not too verbose at all :). One point I would like to both of your thoughts on is regarding

The signers are assumed to be under the control of the contract "owner". Smart accounts are orthogonal to this.

Personally, I don't think we can assume the signers are under the control of the owner. Imagine if we are part of a DAO and the owner is a vote based contract where we all vote on (e.g. multisig). My main account is an EOA, @shrugs's main account is a smart account (i.e smart contract) and @frangio's main account is another type of identity based smart contract. For some reason, @shrugs and @frangio are set as ROLE_SUPER_BOUNCER and i'm set as ROLE_LAME_BOUNCER. Matt will want the "signer" to be his smart account, since it makes it easier for him to manage all his devices + private keys, same goes for you and I just use my EOA account. Hence, 2 bouncers out of 3 are smart contracts.

If in 5 years everyone is using smart accounts as their main account, then this will be even more frequent. The smart account allows you to manage all your private keys as well, one for your phone, one for your laptop, one for your ledger, etc. Actually, a smart account would also have something like RBAC controlling EOA accounts priviledges, but that's a tangent.

I personally believe the change is fairly simple and does not deviate from the initial goal of the Bouncer contract logic, it's simply allowing signers to be contracts.

@shrugs
Copy link
Contributor Author

shrugs commented Jul 20, 2018

delegates may or may not be related to the owner of the bouncer contract, yeah; I don't think that's the case in the current design either. for example, we could make a digital club where members are able to invite 5 other people via these signatures, so anyone with a signature from an existing member can join the club.

verification goes like "owner says identity contract is a delegate, so let's ask the identity contract if the signature is from one of the managed EOA accounts with the correct permissions and if so let them in."

lmk if I interpreted that correctly?

@PhABC
Copy link
Contributor

PhABC commented Jul 20, 2018

Sounds about right! Doesn't need to be identity, could be a multisig or whatever contract that has some access control method.

@frangio
Copy link
Contributor

frangio commented Jul 22, 2018

Ah, you're right, the delegates are not necessarily in control of the owner. The digital club is an interesting example.

I agree with the steps you proposed @shrugs! I opened #1104.

Is the concept of "Invitees" in the title of this issue still valid/relevant?

@shrugs
Copy link
Contributor Author

shrugs commented Jul 22, 2018

@frangio ah, no, "invitees" have been replaced by "delegate"

@shrugs
Copy link
Contributor Author

shrugs commented Jul 22, 2018

I'll close this issue for the benefit of organization, but will most likely use these same commits in a PR for #1104 (or one that is rebased off of it)

@shrugs shrugs closed this as completed Jul 22, 2018
@shrugs
Copy link
Contributor Author

shrugs commented Jul 22, 2018

wait scratch that, this is an issue that we should keep up :D

@shrugs shrugs reopened this Jul 22, 2018
@shrugs shrugs changed the title Allow contracts to be Invitees of a Bouncer Allow contracts to be delegates of a Bouncer Jul 22, 2018
@shrugs
Copy link
Contributor Author

shrugs commented Sep 4, 2018

closing in favor of tracking via #1272

@shrugs shrugs closed this as completed Sep 4, 2018
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

3 participants