Use onlyGovernance to restrict upgrades for Governor with UUPS (#544)

Co-authored-by: Eric Lau <ericglau@outlook.com>

Author: elisafalk

.changeset/refactor-upgradeable-functions.md

@@ -0,0 +1,7 @@
+---
+'@openzeppelin/wizard': patch
+---
+
+Use `onlyGovernance` to restrict upgrades for Governor with UUPS
+- **Potentially breaking changes**:
+  - Governor with UUPS: `_authorizeUpgrade` function is restricted by `onlyGovernance` instead of `onlyOwner`

packages/core/solidity/src/governor.test.ts

@@ -162,6 +162,6 @@ test('API assert defaults', async t => {
 });
 
 test('API isAccessControlRequired', async t => {
-  t.is(governor.isAccessControlRequired({ upgradeable: 'uups' }), true);
+  t.is(governor.isAccessControlRequired({ upgradeable: 'uups' }), false);
   t.is(governor.isAccessControlRequired({}), false);
 });

packages/core/solidity/src/governor.test.ts.md

@@ -1614,25 +1614,23 @@ Generated by [AVA](https://avajs.dev).
     import {GovernorVotesQuorumFractionUpgradeable} from "@openzeppelin/contracts-upgradeable/governance/extensions/GovernorVotesQuorumFractionUpgradeable.sol";␊
     import {IVotes} from "@openzeppelin/contracts/governance/utils/IVotes.sol";␊
     import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";␊
-    import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";␊
     import {TimelockControllerUpgradeable} from "@openzeppelin/contracts-upgradeable/governance/TimelockControllerUpgradeable.sol";␊
     import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";␊
     ␊
-    contract MyGovernor is Initializable, GovernorUpgradeable, GovernorCountingSimpleUpgradeable, GovernorVotesUpgradeable, GovernorVotesQuorumFractionUpgradeable, GovernorTimelockControlUpgradeable, OwnableUpgradeable, UUPSUpgradeable {␊
+    contract MyGovernor is Initializable, GovernorUpgradeable, GovernorCountingSimpleUpgradeable, GovernorVotesUpgradeable, GovernorVotesQuorumFractionUpgradeable, GovernorTimelockControlUpgradeable, UUPSUpgradeable {␊
         /// @custom:oz-upgrades-unsafe-allow constructor␊
         constructor() {␊
             _disableInitializers();␊
         }␊
     ␊
-        function initialize(IVotes _token, TimelockControllerUpgradeable _timelock, address initialOwner)␊
+        function initialize(IVotes _token, TimelockControllerUpgradeable _timelock)␊
             public initializer␊
         {␊
             __Governor_init("MyGovernor");␊
             __GovernorCountingSimple_init();␊
             __GovernorVotes_init(_token);␊
             __GovernorVotesQuorumFraction_init(4);␊
             __GovernorTimelockControl_init(_timelock);␊
-            __Ownable_init(initialOwner);␊
             __UUPSUpgradeable_init();␊
         }␊
     ␊
@@ -1647,7 +1645,7 @@ Generated by [AVA](https://avajs.dev).
         function _authorizeUpgrade(address newImplementation)␊
             internal␊
             override␊
-            onlyOwner␊
+            onlyGovernance␊
         {}␊
     ␊
         // The following functions are overrides required by Solidity.␊

packages/core/solidity/src/governor.test.ts.snap

@@ -7,7 +7,7 @@ import { OptionsError } from './error';
 import { setAccessControl } from './set-access-control';
 import { printContract } from './print';
 import { setInfo } from './set-info';
-import { setUpgradeable } from './set-upgradeable';
+import { setUpgradeableGovernor } from './set-upgradeable';
 import { defineFunctions } from './utils/define-functions';
 import { durationToBlocks, durationToTimestamp } from './utils/duration';
 import { clockModeDefault, type ClockMode } from './set-clock-mode';
@@ -61,8 +61,8 @@ export interface GovernorOptions extends CommonOptions {
   settings?: boolean;
 }
 
-export function isAccessControlRequired(opts: Partial<GovernorOptions>): boolean {
-  return opts.upgradeable === 'uups';
+export function isAccessControlRequired(_: Partial<GovernorOptions>): boolean {
+  return false;
 }
 
 function withDefaults(opts: GovernorOptions): Required<GovernorOptions> {
@@ -99,7 +99,7 @@ export function buildGovernor(opts: GovernorOptions): Contract {
   addTimelock(c, allOpts);
 
   setAccessControl(c, allOpts.access);
-  setUpgradeable(c, allOpts.upgradeable, allOpts.access);
+  setUpgradeableGovernor(c, allOpts.upgradeable);
   setInfo(c, allOpts.info);
 
   return c;

packages/core/solidity/src/governor.ts

@@ -7,7 +7,11 @@ export const upgradeableOptions = [false, 'transparent', 'uups'] as const;
 
 export type Upgradeable = (typeof upgradeableOptions)[number];
 
-export function setUpgradeable(c: ContractBuilder, upgradeable: Upgradeable, access: Access) {
+function setUpgradeableBase(
+  c: ContractBuilder,
+  upgradeable: Upgradeable,
+  restrictAuthorizeUpgradeWhenUUPS: () => void,
+) {
   if (upgradeable === false) {
     return;
   }
@@ -24,7 +28,7 @@ export function setUpgradeable(c: ContractBuilder, upgradeable: Upgradeable, acc
       break;
 
     case 'uups': {
-      requireAccessControl(c, functions._authorizeUpgrade, access, 'UPGRADER', 'upgrader');
+      restrictAuthorizeUpgradeWhenUUPS();
       const UUPSUpgradeable = {
         name: 'UUPSUpgradeable',
         path: '@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol',
@@ -42,6 +46,18 @@ export function setUpgradeable(c: ContractBuilder, upgradeable: Upgradeable, acc
   }
 }
 
+export function setUpgradeable(c: ContractBuilder, upgradeable: Upgradeable, access: Access) {
+  setUpgradeableBase(c, upgradeable, () => {
+    requireAccessControl(c, functions._authorizeUpgrade, access, 'UPGRADER', 'upgrader');
+  });
+}
+
+export function setUpgradeableGovernor(c: ContractBuilder, upgradeable: Upgradeable) {
+  setUpgradeableBase(c, upgradeable, () => {
+    c.addModifier('onlyGovernance', functions._authorizeUpgrade);
+  });
+}
+
 const functions = defineFunctions({
   _authorizeUpgrade: {
     args: [{ name: 'newImplementation', type: 'address' }],