Override Proxy Contract?

Archive SDK
1

Is it possible to override methods in proxy contract (without ruining all the upgrade magic the cli does for me)?

I’m trying to make it such that upgradeTo can only be called once.

1 Like
2

Hi @timigod,

Instead of overriding the Proxy contract:
To prevent an upgradeable contract from being upgraded, you could just call changeProxyAdmin to set the newAdmin to 0x1 address (0x0 is prevented).

Does that work for your use case?

3

Good idea but it doesn’t exactly work for this use case.
I want it to be known that it’ll only be upgraded one time and there’s no better way to enforce that than with code. If I did things the way you suggested, users will have to trust the admin.

1 Like
4

Hi @timigod,

As far as I know, overriding the proxy would then mean that you couldn’t use the CLI.

You could look at having any restrictions being part of the upgrades governance instead:
See the example of using a multi-sig for upgrades governance: https://docs.openzeppelin.com/sdk/2.5/upgrades-governance

Also instead of adding restrictions to the proxy contract, look at adding restrictions on the admin (once deployed).

1 Like
5

I thought as much @abcoathup. Thanks.

I’m currently considering forking the CLI code & changing the Proxy (template) from there. So I’d be able to use my modified CLI to deploy & upgrade.

Do you have any immediate tips/thoughts/warnings?

1 Like
6

Hi @timigod,

My suggestion would be to look at adding restrictions to an overriden ProxyAdmin contract.
This would give you the benefit of using the OpenZeppelin CLI for development and deployment. As soon as you have deployed you could change the admin of the proxy to a proxy admin that only allows one upgrade, prior to announcing that your contract is live to your users.

That way you get the functionality you want, the benefit of the CLI and without having to fork the CLI.

7

Ah @abcoathup , I just looked into this and it makes perfect sense. I didn’t initially realize that upgradeTo & upgradeToAndCall were external and as such can only be called by another contract (in this case, only the ProxyAdmin contract).

Thanks!

1 Like
8

@abcoathup I’m having issues with this. These are the steps I followed:

  1. Deployed (with flag --minimal) my overridden ProxyAdmin contract (see code below). Called initialize(address _owner, uint 256 _allowedUpgrades) and set the owner to the default (0 index) account in my local ganache setup.
  2. Deployed the main contract.
  3. Used set-admin to change the proxy admin to the address of the contract deployed in step 1. I also made sure to double check that the transaction is being sent from the same default (0 index) account.
  4. Attempt to upgrade contract with the upgrade command.

I keep getting this error message “No contract instances that match contract ContractName are owned by this project”

I assumed that by sending the upgrade transaction from the same address as the owner of the ProxyAdmin contract, it’ll all work. Apparently not. How do I make it such that the project still owns the contract?

Here’s my custom ProxyAdmin code:

import "@openzeppelin/upgrades/contracts/upgradeability/AdminUpgradeabilityProxy.sol";
import "@openzeppelin/upgrades/contracts/ownership/Ownable.sol";

contract ProxyAdmin is OpenZeppelinUpgradesOwnable {
    uint256 public allowedUpgrades = 0;
    uint256 public upgradeCount = 0;
    bool internal initialized;

    function initialize(address _owner, uint256 _allowedUpgrades) public {
        require(!initialized);

        _transferOwnership(_owner);
        allowedUpgrades = _allowedUpgrades;
        initialized = true;
    }
    
    function getProxyImplementation(AdminUpgradeabilityProxy proxy) public view returns (address) {
        // We need to manually run the static call since the getter cannot be flagged as view
        // bytes4(keccak256("implementation()")) == 0x5c60da1b
        (bool success, bytes memory returndata) = address(proxy).staticcall(hex"5c60da1b");
        require(success);
        return abi.decode(returndata, (address));
    }
    
    function getProxyAdmin(AdminUpgradeabilityProxy proxy) public view returns (address) {
        // We need to manually run the static call since the getter cannot be flagged as view
        // bytes4(keccak256("admin()")) == 0xf851a440
        (bool success, bytes memory returndata) = address(proxy).staticcall(hex"f851a440");
        require(success);
        return abi.decode(returndata, (address));
    }
    
    function upgrade(AdminUpgradeabilityProxy proxy, address implementation) public onlyOwner {
        require(upgradeCount < allowedUpgrades);
        proxy.upgradeTo(implementation);
        upgradeCount++;
    }
    
    function upgradeAndCall(AdminUpgradeabilityProxy proxy, address implementation, bytes memory data) payable public onlyOwner {
        require(upgradeCount < allowedUpgrades);
        proxy.upgradeToAndCall.value(msg.value)(implementation, data);
        upgradeCount++;
    }
}
1 Like
9

Hi @timigod,

I repeated steps 1-4 and got the same result (as I would expect).

In my <network>.json I changed the address of the proxyAdmin to the address of MyProxyAdmin contract and repeated step 4 and was able to upgrade via the CLI using upgrade.

I assume you should call your ProxyAdmin contract by a different name e.g. something like LimitedUpgradesProxyAdmin.

Could you inherit from ProxyAdmin instead of recreating?

The current implementation is a count of upgrades for proxies that the ProxyAdmin is the admin for. The upgrade count may need to be per proxy depending on your requirements.

You need to check whether set-admin could be used to change to an unrestricted ProxyAdmin?

Please note that minimal proxy support is still experimental as per the warning:

Minimal proxy support is still experimental.

In upgradeable contracts we can’t set initial values in a field declaration (see the warning below) e.g. uint256 public upgradeCount = 0;

- Contract MyProxyAdmin or one of its ancestors sets an initial value in a field declaration. Consider moving all field initializations to an initializer function. See https://docs.openzeppelin.com/sdk/2.5/writing_contracts.html#avoid-initial-values-in-fields-declarations.

See the documentation for details: https://docs.openzeppelin.com/sdk/2.5/writing-contracts.html#avoid-initial-values-in-fields-declarations

If you create your own ProxyAdmin then this would need appropriate testing and auditing to ensure operation as expected.

My thoughts would still be that rather than create your own ProxyAdmin, it would be to do the restriction via governance e.g. a multi-sig, or explain to your users that once the project was at a certain stage then upgradeability would be removed.

10

Thanks a lot @abcoathup, I tried that and it worked. As for the other things you said:

  1. Yes, it makes sense to change the name.
  2. It also makes sense to use a mapping so that it can be an upgrade count per proxy.
  3. Extending ProxyAdmin would certainly look neater. But I want to remove the ability to change the admin again (like I did above)
  4. We do plan to get an audit soon (with OpenZeppelin of course)
  5. While I know I can tell my users that upgradeability will be removed, I (and I’m sure they will) value the ability to express this via smart contracts & the blockchain. There’s no way that’s more trustworthy than that. I want it such that changing my mind about the upgrade count would be impossible.
1 Like
11

Hi @timigod,

Good to hear that you are planning an audit.

I recommend reading the following checklist for before an audit:

I found reading past audits very useful: https://blog.openzeppelin.com/security-audits/

For details on OpenZeppelin audits, please see the website: https://openzeppelin.com/security-audits/

1 Like