search

ScopeLift / umbra-protocol

🌕🌑 Privacy Preserving Shielded Payments On The Ethereum Blockchain
https://app.umbra.cash
MIT License
360 stars 93 forks source link

Separate Withdrawal Contract From Announcements Contract & Support Other Meta TX Systems #36

Closed apbendi closed 3 years ago

apbendi commented 4 years ago

Problem

Currently, the Umbra contract serves several functions simultaneously:

Because it's serving all three purposes, the system is less flexible overall. In particular, it's unable to accommodate alternate meta tx services, for example any.sender or others.

Proposal

We can separate the withdraw functionality from the other functions of the contract, and allow multiple implementations of Withdrawal contracts to exist. Different Withdrawal contracts could implement different meta tx schemes, and/or other behavioral differences (for example, see #37).

Implementation Options

Arbitrary Withdrawal Contracts

In this conception, any contract could serve as a withdraw contract, and anyone would be free to deploy a contract that could serve for withdrawals. A 'withdraw' method on the Umbra contract would exist that could take the withdrawal request, validate it in some way it came from the receiving stealth address, then take some action based on the data passed to the main contract. Theoretically, the withdraw message could even be called directly by the stealth address itself.

One of the issues here is that, because the Withdrawal contract will be the caller, the msg.sender of the Umbra contract method will be the Withdrawal contract. We'd therefore be unable to do a naive check of msg.sender == stealthAddress to ensure the funds were being claimed by the stealth receiver.

In fact, meta tx systems themselves have this issue, and overcome it by recovering the true "sender" from a signature on the metatx, and forwarding it along with the call. In the current Umbra contract, following GSN's implementation, we use a helper method _msgSender() which is recovered from a signature on the metatx.

We could do something similar within the Umbra withdraw method. For example, instead of using typical Solidity arguments for parameters, we could use packed abi encoded bytes that must be signed by the stealth address' private key. We could then use ecrecover to get the address of the withdrawer.

One slightly bizarre thing here is we'd be stacking signature validated meta-ness between the meta tx system and our withdraw method. For a client interacting with a Withdrawal contract that implements GSN, for example, the client would have to sign the bytes for our arguments, then include those arguments in a meta-tx, which it would also have to sign. I don't think this is really a problem, but it does feel a bit odd. Is there a smell here suggesting a simpler way?

Trusted Withdrawal Contracts

One option to workaround the issues described above is to have an array of trusted withdrawal contracts which can be updated by the Admin key. We could then rely on the contracts to do the verification of the stealth addresses ownership according to whatever system is appropriate for the meta-tx system being implemented, and avoid verifying this in the base Umbra contract at all.

This has a maaaajor downside of adding huge centralizing and security risk. The owner of the admin key would be able to deploy a contract that didn't verify control of the stealth address, and subsequently drained user funds. There might be workarounds for this (time delays, etc...) but they add as much complexity as that which we'd be trying to avoid.

I think we should eschew this approach unless we're unable to make arbitrary withdrawal work, or there's a simpler way to make a trusted list work.

Static Withdrawal Contracts

We could still use a list of different withdrawal contracts, implementing several meta-tx systems, but we could deploy a static list of these with the protocol deployment that could not be updated. This removes the trust assumption. The obvious downside is the lack of flexibility. We'd be locked into only the options we implement before deployment.

Other Considerations Or Downsides

apbendi commented 3 years ago

This issue is essentially closed by #72, which implements the pattern enabling arbitrary withdrawal contracts. As documented in #81, our current configuration does lead to double signatures when used with OpenGSN, however this can be negated by implementing a custom forwarder that validates only the method being called, and skips the signature check which will be done by Umbra itself.