How to test upgradeability for proxies

Support Upgrades
1

My contract previously looked like this;

contract MyContract is UUPSUpgradeable, ERC20Upgradeable {

   function initialize(...) initializer {
      // ..... some code
   }
}

and my tests were fine. I was deploying MyContract with .deploy() function in my hardhat ethers and that was it. Now, I decided to bring extra safety in all my contracts, so most of them are upgradeable, which means in 99% of them, I had to add the following:

constructor() {
   _disableInitializers();
}

Now, Due to this, every test that I had was failing because, they can't call initialize anymore since constructor disables it. NOTE that I was deploying the contracts directly.

Due to this change, I got 1 option now:

  • Update every test in a way that I don't use deploy, but I use deployProxy from OZ (This will ensure that in the end proxy will be deployed and initializers won't be disabled on the proxy, but on the implementation(which I don't care).

Problem with this is that tests pretty much slow down 2x times.(It now needs to deploy 2 contracts instead of 1). Are there any other ways you can think of which doesn't slow down tests, but also doesn't require too much work ?

2

If an implementation was written to only be used with a proxy, then it seems reasonable to always use deployProxy to test with a proxy rather than testing the implementation directly. That would help ensure the tested behaviour is as close as possible to what it would be on a real network. I don't see a way to avoid deploying proxies in this case.

3

Thanks for the answer, but implementation was not written only to be used with proxy, but the problem somehow is disableInitializers being in the constructor which then causes initialize to not be called !

4

disableInitializers being in the constructor which then causes initialize to not be called is the intended effect. Having this in the constructor means the implementation is not meant to be usable directly, and therefore only usable with a proxy.

1 Like
5

Does this imply having _disableInitializers(); in the constructor is a new standard way or at least recommended way of using UUPSUpgradeable compliant logic contract?

6

@ericglau I also am curious how it would actually work.

The OZ already fixed the problem by adding onlyProxy modifier to upgrade functions. This means nobody can call upgradeToAndCall on the implementation. What other risks do you see if you leave implementation uninitialized?

7

Does this imply having _disableInitializers(); in the constructor is a new standard way or at least recommended way of using UUPSUpgradeable compliant logic contract?

Yes, this is recommended as described in https://docs.openzeppelin.com/contracts/4.x/api/proxy#Initializable

The OZ already fixed the problem by adding onlyProxy modifier to upgrade functions. This means nobody can call upgradeToAndCall on the implementation. What other risks do you see if you leave implementation uninitialized?

You are correct that the previous UUPS vulnerability was fixed by restricting the upgrade functions to onlyProxy, but disabling initializers is still recommended as a best practice to provide an extra layer of protection against these types of attacks.

2 Likes