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

Research how to best handle changing proxy addresses #38

Closed
rmeissner opened this issue Mar 26, 2020 · 1 comment · Fixed by #130
Closed

Research how to best handle changing proxy addresses #38

rmeissner opened this issue Mar 26, 2020 · 1 comment · Fixed by #130
Assignees
Labels
documentation Improvements or additions to documentation

Comments

@rmeissner
Copy link
Member

rmeissner commented Mar 26, 2020

It would be good to have an outline what our strategy is to handle new mastercopies.

As new master copies will change the predicted proxy address we should probably add a lib that can check based on the different mastercopies (and other params) if a proxy exists.

Questions

  • How to check if a proxy already exists
    • Should we calcualte potential proxy addresses based on all known/ supported mastercopies?
  • How to handle features that are only supported by new mastercopies
    • Should the proxy be auto updated?
@rmeissner rmeissner added the documentation Improvements or additions to documentation label Mar 26, 2020
@cag cag mentioned this issue May 1, 2020
3 tasks
@cag cag changed the title Add documentation on potential support for new mastercopies Research how to best handle changing proxy addresses Jun 9, 2020
@cag
Copy link
Contributor

cag commented Aug 4, 2020

So a big question that has come up is the direction of this project. Should we try to resolve the issue of changing proxy addresses once and for all, or would it be not a big deal, or even a feature, if the proxy that is used is tied to the configuration of the CPK?

I can think of some pros and cons:

Pros:

  • Versioning will become easier. New major versions would just be based on breaking changes in the JS/TS interface, and won't be dependent on contract changes.
  • There's no config to manage, other than possibly accounting for legacy instances.
  • Less overall gas consumption. Sharing a single proxy means multiple dapps don't need to reapprove tokens for the proxy.
  • Users only have to manage one new Safe if using the Safe app with the proxy.

Cons:

  • Security of the proxy. Because it is shared across all users of the CPK, a single malicious dapp could misconfigure the proxy for all dapps relying on it, especially without Allow salt nonce to be configured #14.
  • First transaction would consume more gas due to the inclusion of another transaction to set the Safe version used by the proxy.
  • Would need to design future-proof factory, since factory would not be able to change.

The relayer service will have to change as well, especially to accommodate #67, so the factory would have to be able to take into account the "true" requester when setting up proxies. I think this might mean that any final proxy factory would essentially be proprietary. Should we support GSN in this case? (#76 #79)


Anyway, auto-updating would be easier with finalizing the proxy address determination. It might not be easily doable otherwise though, since there would have to be some way of figuring out how to move from version A to version G, or B to G, C to G, etc, and then from A to H, B to H, etc., after an update. Then again, if sticking within Safe versions, we probably can specify an interface updating relies on in order to make auto-updating more feasible, which means declaring something like this to be part of the interface: https://github.com/gnosis/safe-contracts/blob/94f9b9083790495f67b661bfa93b06dcba2d3949/contracts/common/MasterCopy.sol#L18

Currently, the function that updates the implementation of the proxy isn't implemented on the proxy, but rather the GnosisSafe implementation. However, the accessor that reports the implementation address is implemented on the proxy. So while the storage slot (0) that reports the implementation is determined by the accessor implemented on the proxy, the proxy can be sealed by updating to an implementation that does not allow further mutations of that slot's value.

@cag cag mentioned this issue Sep 10, 2020
8 tasks
@germartinez germartinez self-assigned this Oct 22, 2020
@germartinez germartinez linked a pull request Nov 3, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants