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

Add OpenZeppelin's upgrades validation for proxies #162

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

outdoteth
Copy link

@outdoteth outdoteth commented Jul 23, 2021

Fixes: #141

Also, this PR builds on #142 so that needs to be merged first.

This integrates open zeppelin's upgrades plugin: https://github.com/OpenZeppelin/openzeppelin-upgrades
and uses their upgrade verification logic to ensure that a contract is a valid implementation.

Some examples of the cool new errors generated 😄 :

Screenshot 2021-07-23 at 16 47 09

Screenshot 2021-07-23 at 16 48 27

This is what happens with this plugin:

  1. Add a subtask to solidity compile and format solc output to match open zeppelin's required format. Save this output into cache/validations.json
  2. User runs a proxy deploy task. E.g.:
    await deploy("Foo", {
        from: deployer,
        contract: "ContractA",
        proxy: {
            proxyContract: "TransparentUpgradeableProxy",
            viaAdminContract: {
                name: "ProxyAdmin",
                args: [],
            },
        },
    });
  1. Fetch "ContractA" from cache/validations.json and put it through openzeppelin_assertIsValidImplementation to ensure the contract doesn't contain any constructors etc. and is valid.
  2. Save "ContractA" to ".openzeppelin" manifest

Then on upgrade, there is an additional step.

  1. User runs deploy task again on "Foo" but upgrades implementation to "ContractB":
    await deploy("Foo", {
        from: deployer,
        contract: "ContractB", // Change to ContractB from ContractA
        proxy: {
            upgradeIndex: 1, // we are upgrading the contract here
            proxyContract: "TransparentUpgradeableProxy",
            viaAdminContract: {
                name: "ProxyAdmin",
                args: [],
            },
        },
    });
  1. Repeat steps 2-4 but with "ContractB" instead
  2. Then pass the proxy contract and the new implementation into openzeppelin_assertIsValidUpgrade. This will fetch the old implementation and compare it against the new implementation to make sure it is valid.
  3. Save "ContractB" to ".openzeppelin" manifest

@outdoteth outdoteth changed the title Add proxy validation Add OpenZeppelin's proxy upgrades validation Jul 23, 2021
@outdoteth outdoteth changed the title Add OpenZeppelin's proxy upgrades validation Add OpenZeppelin's upgrades validation for proxies Jul 23, 2021
@outdoteth
Copy link
Author

outdoteth commented Jul 23, 2021

Possible TODOs:

  • Make the validation have a toggle so the user can opt out if needed.
  • Allow the user to pass validation config to openzeppelin verifier.

But I think this serves as a good starting point for now and is sufficient to be merged.

@gregegan
Copy link

@outdoteth @wighawag - This looks extremely helpful, we're actually trying to solve for figuring out storage slot comparisons during proxy upgrades. What is the status of this PR? Would love to get this merged in if it's 👍

@outdoteth
Copy link
Author

@gregegan no update from me. Waiting for @wighawag to review.

@wighawag
Copy link
Owner

wighawag commented Dec 4, 2021

Hey all, sorry for the lack of update, need to look deeper but currently not available.
Once I have more time, I ll look into it

@nishuzumi
Copy link
Contributor

This is a very important feature that will take a lot of work out of it. Hope to merge soon

@wighawag
Copy link
Owner

I just had a quick look and saw that there is a dependency on hardhat-upgrades

Any way to remove the dependency on hardhat-upgrades which is itself a plugin ?

having plugin import other plugin is not great

@outdoteth
Copy link
Author

@wighawag Yes it should be possible. There are only 3 small utility functions which are used from that package. I'll take a look tomorrow.

@nishuzumi
Copy link
Contributor

@wighawag Yes it should be possible. There are only 3 small utility functions which are used from that package. I'll take a look tomorrow.

Hi, what about now

// Appends the implementation to the implementation history of the proxy.
// Used for comparison against future implementations to ensure they are
// compatible with eachother in `openzeppelin_assertIsValidUpgrade()`.
export const openzeppelin_saveDeploymentManifest = async (
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be better if we did not use openzeppelin file but instead include the data in the deployment file where it should belong

@fr0zn
Copy link

fr0zn commented Feb 21, 2022

Any updates on this one?

@naderhen
Copy link

If this PR isn't progressing, is anyone aware of a good way to validate upgrades that are deployed via hardhat-deploy? I recently moved my project over to hardhat-deploy (for so many reasons, this project is amazing!), but would definitely miss the confidence that OZ's validation gives me when releasing upgrades.

I'm not super familiar with the architecture here, but I may be able to help keep things moving with a point in the right direction.

@fullkomnun
Copy link
Contributor

fullkomnun commented Aug 2, 2022

I submitted an updated PR with support for openzeppelin's upgradability validations #358
@outdoteth @wighawag

@pnghai
Copy link

pnghai commented Oct 24, 2022

@outdoteth I doubt that pending conflicts are blocking this fork from merging with main branch?

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

Successfully merging this pull request may close these issues.

Proxy contracts NEED to have some kind of validation
8 participants