Upgradable Authenticator #351

bh2smith · 2021-01-18T08:29:50Z

Closes #167

This PR introduces minor changes to the AllowListAuthenticator making it upgradable and includes tests verifying upgradability. In particular:

removing customInitiallyOwnable as it is no longer used: Not yet sure how this will affect the deployments directory which still includes it.
Updating authenticator deployment script to deploy as proxy.
rename authenticator owner to manager because of name collision with proxy owner
introduce proxy.ts with helper methods to fetch implementation and owner address.
Include two unit tests: one verifying that upgrade is possible and the other to ensure preservation of storage.

Note that the unit tests involving Authenticator's non-upgrade related functionality do not use the proxy deployment as specified in the migration scripts, so that manager must be set immediately after deployment.

We had originally planned/hoped to use the hardhat-upgrades plugin and opened the following two PRs to openzeppelin-upgrades, but this plugin turned out to be unnecessary.

Custom Deployments: Custom Deployments OpenZeppelin/openzeppelin-upgrades#273
Manual Safety Override: Add a manual override for every check OpenZeppelin/openzeppelin-upgrades#280

Test Plan

Old + new unit and e2e tests.

nlordell

Looks very good! Requested changes:

Remove @openzeppelin/contracts-upgradeable since we shouldn't be mixing and matching upgradeable contract libraries
Add unit tests to make sure setManager cannot be called to "take control" of the contract after deployment.

package.json

src/contracts/GPv2AllowListAuthentication.sol

src/ts/deploy.ts

src/ts/proxy.ts

test/GPv2AllowListAuthenticator.test.ts

src/contracts/GPv2AllowListAuthentication.sol

test/e2e/upgradeAuthenticator.test.ts

fedgiac

Nice work.
How does changing the owner and the manager work? To my understanding:

the owner can probably be changed with a custom proxy function,
the manager cannot be changed once is set, and is the initial owner.

I think 2 can be fixed by hardcoding the manager to be the owner with some assembly reading the right storage slot, then the behavior would be as expected (owner = manager after changing the owner).
In any case, I would test both cases to clarify what is expected of the proxy.

test/e2e/upgradeAuthenticator.test.ts

test/GPv2AllowListAuthenticator.test.ts

test/e2e/deployment.test.ts

test/e2e/upgradeAuthenticator.test.ts

Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>

…cy assurance, rename setManager to initializeManager

Co-authored-by: Federico Giacon <[email protected]>

nlordell

Looks great!

src/ts/proxy.ts

test/GPv2AllowListAuthenticator.test.ts

nlordell

Just one change I noticed last minute

nlordell · 2021-02-02T14:26:27Z

yarn.lock

@@ -629,72 +629,72 @@
    path-browserify "^1.0.0"
    url "^0.11.0"

-"@sentry/[email protected]":


Can we revert the yarn.lock changes? I don't think they should be included in this PR.

This wasn't intentional.. I will revert them but I am just using whatever yarn install gives me.

This is probably because the package.json changed and then the change was reverted. If you revert the lock file, then yarn install this should be the same.

I am not able to make this happen without forcing the lock file from main. Could it be my node version that is doing this?

Super weird....

Hmm, maybe its something specific to your setup. If I run yarn install on your branch, it respects the lock file and does not overwrite it:

$ git clone -b 167/upgradable_authenticator [email protected]:gnosis/gp-v2-contracts.git TEMP ... $ cd TEMP; yarn ... $ git status On branch 167/upgradable_authenticator Your branch is up to date with 'origin/167/upgradable_authenticator'. nothing to commit, working tree clean

frangio · 2021-02-02T14:32:04Z

As I was discussing with @bh2smith over Slack, I think it's necessary to have an automated mechanism to check that the storage layout of future upgrades remains compatible, to avoid accidentally corrupting the storage of live contracts. This check is built into the OpenZeppelin Upgrades plugin but I understand you weren't able to use it because you needed deterministic deployments.

Even if you don't use it for production deployment, my recommendation would be to have a set of tests that use our plugin to deploy and upgrade from one version to the next. Having this as part of your test suite should provide some assurance that future upgrades are correct.

One issue we have currently is that you need to skip one of the checks that we do (disallowing delegatecall), but this is something we can quickly add if you need it.

fedgiac

A point worth discussing before merging: do we really want that the manager is set once for all and can never change until we change the authenticator contract code with a function that does so? The owner of the proxy may change, but not the manager.

Otherwise, the PR and the feature look very nice to me!

To frangio: I agree that with the current changes we can't update the contract without testing that each "base" entry in storage is preserved. We should discuss this in the team, but I believe that upgrades will be sufficiently few and the used storage sufficiently limited that we can resort to manual testing for each update. Depending on the complexity, we may as well do as you say and use openzeppelin-upgrades to test the upgrade process.

src/ts/proxy.ts

bh2smith · 2021-02-02T17:56:54Z

@fedgiac note that we can always introduce a function in the version 2 contract that allows us to update the manager. Is this good enough for our purposes or should we make another onlyManager method right now that allows the manager to reassign someone else?

fedgiac · 2021-02-03T09:25:32Z

I'd make it so that the proxy owner can change the manager at will. Upgrading the proxy is a relatively expensive operations (as pointed out by frangio), and changing the manager is something that we might need to do from the start, for example because we want to assign this power to some other staking contract, or a DAO. Note that we use the same owner address on mainnet and Rinkeby for now: the first transaction I expect after releasing is changing the owner of the mainnet contract to some other address. Then also the manager should be changing.

Co-authored-by: Federico Giacon <[email protected]>

…2-contracts into 167/upgradable_authenticator

nlordell · 2021-02-03T12:44:23Z

src/contracts/GPv2AllowListAuthentication.sol

+    }
+
+    function updateManager(address newManager) external onlyProxyAdmin {
+        manager = newManager;


If the manager gets set to address(0), then anyone can call initializeManager and take ownership of the contract now.

Maybe instead of adding the ability of changing the manager in this PR, we do it as a separate PR. What do you think @fedgiac ?

My understanding is that the current changes on the owner are work in progress.
I don't mind if it's done here or done in another PR, but I think that making provision for changing the manager is necessary before releasing.

I think that making provision for changing the manager is necessary before releasing.

Selbstverständlich

Reverted the onlyProxyAdmin

nlordell

I think there are some security wholes with the additional changes. I propose merging the PR as it was before and adding the ability to change the manager and proxy owner as a separate PR.

nlordell · 2021-02-03T12:48:19Z

src/contracts/GPv2AllowListAuthentication.sol

+        // Slot taken from https://eips.ethereum.org/EIPS/eip-1967#specification
+        // obtained as bytes32(uint256(keccak256('eip1967.proxy.admin')) - 1)
+        bytes32 slot =
+            0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103;


This should probably be a constant.

fedgiac · 2021-02-03T12:57:48Z

For the records, I'm ok with merging the PR as it was before the recent changes and discuss the work on the settable manager in another PR.

fleupold · 2021-02-03T16:29:03Z

If we allow to change the manager later, isn't that equivalent to inheriting open zeppelin's Ownable?

nlordell · 2021-02-03T17:03:35Z

isn't that equivalent to inheriting open zeppelin's Ownable?

Yes, I think we just need to inherit both Initializable and Ownable and then we get everything out of the box.

bh2smith changed the title ~~attempt at upgradable Authenticator with hardhat-upgrades~~ Upgradable Authenticator Jan 18, 2021

fedgiac mentioned this pull request Jan 27, 2021

[playground] Upgradeable authenticator: split proxy ownership and authenticator ownership #379

Merged

making Authenticator upgradable

4fe914f

bh2smith force-pushed the 167/upgradable_authenticator branch from b15486b to 4fe914f Compare February 1, 2021 21:16

bh2smith added 3 commits February 2, 2021 10:51

remove customInitiallyOwnable

8d96c84

remove unnecessary deployment options

6a0ba03

remove commented import

137f6a1

bh2smith marked this pull request as ready for review February 2, 2021 10:01

bh2smith requested a review from a team February 2, 2021 10:01

nlordell suggested changes Feb 2, 2021

View reviewed changes

fedgiac reviewed Feb 2, 2021

View reviewed changes

test/e2e/upgradeAuthenticator.test.ts Outdated Show resolved Hide resolved

test/GPv2AllowListAuthenticator.test.ts Show resolved Hide resolved

test/e2e/deployment.test.ts Show resolved Hide resolved

test/e2e/upgradeAuthenticator.test.ts Show resolved Hide resolved

bh2smith and others added 4 commits February 2, 2021 12:31

Update src/ts/proxy.ts

dd520d2

Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>

remove dependency on oz/contracts-upgradable, implement own idempoten…

ee3cd11

…cy assurance, rename setManager to initializeManager

Update test/e2e/upgradeAuthenticator.test.ts

3920986

Co-authored-by: Federico Giacon <[email protected]>

include note about how from address is owner/manager on upgrade

7f3e8c0

bh2smith requested a review from nlordell February 2, 2021 12:19

fetch solver from wallets:

39e5c51

nlordell approved these changes Feb 2, 2021

View reviewed changes

src/ts/proxy.ts Show resolved Hide resolved

test/GPv2AllowListAuthenticator.test.ts Show resolved Hide resolved

nlordell reviewed Feb 2, 2021

View reviewed changes

fedgiac reviewed Feb 2, 2021

View reviewed changes

src/ts/proxy.ts Outdated Show resolved Hide resolved

src/ts/proxy.ts Show resolved Hide resolved

nlordell mentioned this pull request Feb 3, 2021

Add test verifying deterministic deployment of proxy #398

Merged

bh2smith and others added 6 commits February 3, 2021 11:53

Update src/ts/proxy.ts

cee5b69

Co-authored-by: Federico Giacon <[email protected]>

Merge branch 'main' into 167/upgradable_authenticator

9526057

yarn lock

ecd7315

copy paste lock file from main

26e8335

check idempotency with from owner

106c2c0

add additionaly idempotency check

1a1f5ad

bh2smith and others added 4 commits February 3, 2021 12:09

Merge branch 'main' into 167/upgradable_authenticator

b4e7d5c

Merge branch 'main' into 167/upgradable_authenticator

6c5ad0f

include onlyProxyAdmin modifier for updateManager

504228d

Merge branch '167/upgradable_authenticator' of github.com:gnosis/gp-v…

888c203

…2-contracts into 167/upgradable_authenticator

nlordell reviewed Feb 3, 2021

View reviewed changes

nlordell suggested changes Feb 3, 2021

View reviewed changes

revert onlyProxyAdmin for another PR

9414ed8

nlordell approved these changes Feb 3, 2021

View reviewed changes

nlordell mentioned this pull request Feb 3, 2021

Make allow list manager settable. #405

Closed

bh2smith added the merge when green Merge when green! label Feb 3, 2021

mergify bot merged commit 9b5796c into main Feb 3, 2021

mergify bot deleted the 167/upgradable_authenticator branch February 3, 2021 14:30

fedgiac mentioned this pull request Feb 4, 2021

Allow changing manager #407

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgradable Authenticator #351

Upgradable Authenticator #351

bh2smith commented Jan 18, 2021 •

edited

Loading

nlordell left a comment

fedgiac left a comment

nlordell left a comment

nlordell left a comment

nlordell Feb 2, 2021

bh2smith Feb 2, 2021

nlordell Feb 2, 2021 •

edited

Loading

bh2smith Feb 3, 2021

nlordell Feb 3, 2021

nlordell Feb 3, 2021

frangio commented Feb 2, 2021

fedgiac left a comment

bh2smith commented Feb 2, 2021

fedgiac commented Feb 3, 2021

nlordell Feb 3, 2021

fedgiac Feb 3, 2021

nlordell Feb 3, 2021

bh2smith Feb 3, 2021

nlordell left a comment

nlordell Feb 3, 2021

fedgiac commented Feb 3, 2021

fleupold commented Feb 3, 2021

nlordell commented Feb 3, 2021

Upgradable Authenticator #351

Upgradable Authenticator #351

Conversation

bh2smith commented Jan 18, 2021 • edited Loading

Test Plan

nlordell left a comment

Choose a reason for hiding this comment

fedgiac left a comment

Choose a reason for hiding this comment

nlordell left a comment

Choose a reason for hiding this comment

nlordell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlordell Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frangio commented Feb 2, 2021

fedgiac left a comment

Choose a reason for hiding this comment

bh2smith commented Feb 2, 2021

fedgiac commented Feb 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlordell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedgiac commented Feb 3, 2021

fleupold commented Feb 3, 2021

nlordell commented Feb 3, 2021

bh2smith commented Jan 18, 2021 •

edited

Loading

nlordell Feb 2, 2021 •

edited

Loading