Pattern to make an UUPS not upgradeable

Smart Contracts Review Wanted
1

Hi,

I'm using UUPS proxies in some of my projects and I was thinking about a pattern to make a proxy not upgradeable anymore. Because of the _upgradeToAndCallSecure function with the secure pattern I think a good way is to make it upgradeable and have the ability to stop the upgradeability afterwards. I dont know if there is any other official pattern, I propose those two and ask for a review. Any improvement is appreciated.

First proposal:

// SPDX-License-Identifier: Unlicensed
pragma solidity 0.8.9;

import "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol";
import "../utils/AccessControlProxyPausable.sol";

contract UUPSNotUpgradeable is AccessControlProxyPausable, UUPSUpgradeable {

    bytes32 public constant UPGRADER_ROLE = keccak256("UPGRADER_ROLE");

    function endUpgradeability() public onlyRole(UPGRADER_ROLE) {
        StorageSlot.getBooleanSlot(bytes32(uint256(keccak256("eip1967.proxy.upgradeabilityEnded")) - 1)).value = true;
    }

    function upgradeabilityEnded() public view returns(bool) {
        return StorageSlot.getBooleanSlot(bytes32(uint256(keccak256("eip1967.proxy.upgradeabilityEnded")) - 1)).value;
    }

    function _authorizeUpgrade(address newImplementation)
        internal
        onlyRole(UPGRADER_ROLE)
        override
    {
        require(!upgradeabilityEnded(), "UUPSNotUpgradeable: not upgradeable anymore");
    }
}

Second proposal:

// SPDX-License-Identifier: Unlicensed
pragma solidity 0.8.9;

import "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol";
import "../utils/AccessControlProxyPausable.sol";

contract UUPSNotUpgradeable is AccessControlProxyPausable, UUPSUpgradeable {

    bytes32 public constant UPGRADER_ROLE = keccak256("UPGRADER_ROLE");

    function _authorizeUpgrade(address newImplementation)
        internal
        onlyRole(UPGRADER_ROLE)
        override
    {
        require(StorageSlot.getBooleanSlot(bytes32(uint256(keccak256("eip1967.proxy.rollback")) - 1)).value, "UUPSNotUpgradeable: not upgradeable anymore");
    }
}
2

Hello @gperezalba

IMO, both proposals are valid, and both would achieve the expected goal.

More than a question of "security", I believe both options have a philosophical difference:

  • Option 1 requires you to take explicit action to stop upgradeability.
  • Option 2 automatically breaks upgradeability in a way that is smart, but also insidious. Some people will see that as elegant, other people will see that as an attack.
3

Great, thanks @Amxx for your review :wink: so if there isn't any other "official" or standar I'll use option 2, I see it elegant haha

4

This is something we have imagined internally, but as far as I know you would be the first one to actually do it. We did come up with the same 2 options, but during very different discussion:

  • option 1 came up when discussing how to remove upgradeability "purposefully"
  • we discussed option 2 when thinking about "what can actually break the upgradeability that our system wouldn't catch"