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

Proxy contracts NEED to have some kind of validation #141

Closed
outdoteth opened this issue Jun 24, 2021 · 3 comments · May be fixed by #162
Closed

Proxy contracts NEED to have some kind of validation #141

outdoteth opened this issue Jun 24, 2021 · 3 comments · May be fixed by #162

Comments

@outdoteth
Copy link

outdoteth commented Jun 24, 2021

At the moment if you try to deploy a proxy contract with an implementation that has invalid structure it will still succeed.
For example this contract will deploy fine:

contract Foo {
  uint public b;
  uint public c = 5;

  constructor(a uint) { 
    b = a; 
  }
}

But this contract is invalid when deployed from a proxy. Because any proxy relaying to this implementation won't have the c or b variables set. I think it should throw an error here if the user tries to deploy this or at least some kind of warning.

@outdoteth
Copy link
Author

outdoteth commented Jun 24, 2021

Also related, is in upgrading proxies - Storage collisions cause non-trivial errors. Obviously, it's harder to verify this because an upgrade could happen at a distant point in the future.

Old implementation:

contract Foo { 
  uint public b = 5;
};

New implementation:

// Incorrect
contract Bar { 
  uint public c; // Collision here! - value is actually set to 5 not 0
}

// Correct
contract Bar {
  uint public b; // value is 5 as expected
  uint public c; // value is 0
}

It would be useful to have a function that checks that the new contract doesn't conflict with the old contracts storage and is a valid proxy upgrade.

// Checks if "Foo" can be upgraded to "Bar"
const isValid = deployments.isValidProxyUpgrade("Foo", "Bar");

OpenZeppelin has a similar check but it is in-built into their deploy function so is of no use here in hardhat-deploy although their verification code can probably be mostly reused (?)

@wighawag
Copy link
Owner

Definitely wanted :), this was also mentioned here : #65

@wighawag
Copy link
Owner

wighawag commented Oct 3, 2021

closing as duplicate of #65

@wighawag wighawag closed this as completed Oct 3, 2021
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 a pull request may close this issue.

2 participants