diff --git a/.changeset/refactor-upgradeable-functions.md b/.changeset/refactor-upgradeable-functions.md new file mode 100644 index 000000000..97fb0a377 --- /dev/null +++ b/.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` diff --git a/packages/core/solidity/src/governor.test.ts b/packages/core/solidity/src/governor.test.ts index 8b84b468c..3d7535024 100644 --- a/packages/core/solidity/src/governor.test.ts +++ b/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); }); diff --git a/packages/core/solidity/src/governor.test.ts.md b/packages/core/solidity/src/governor.test.ts.md index d3f9d9ffa..cfb1351a9 100644 --- a/packages/core/solidity/src/governor.test.ts.md +++ b/packages/core/solidity/src/governor.test.ts.md @@ -1614,17 +1614,16 @@ 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");␊ @@ -1632,7 +1631,6 @@ Generated by [AVA](https://avajs.dev). __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.␊ diff --git a/packages/core/solidity/src/governor.test.ts.snap b/packages/core/solidity/src/governor.test.ts.snap index 1e58b118d..cf418a4b8 100644 Binary files a/packages/core/solidity/src/governor.test.ts.snap and b/packages/core/solidity/src/governor.test.ts.snap differ diff --git a/packages/core/solidity/src/governor.ts b/packages/core/solidity/src/governor.ts index a8d64f010..0fec389dd 100644 --- a/packages/core/solidity/src/governor.ts +++ b/packages/core/solidity/src/governor.ts @@ -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): boolean { - return opts.upgradeable === 'uups'; +export function isAccessControlRequired(_: Partial): boolean { + return false; } function withDefaults(opts: GovernorOptions): Required { @@ -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; diff --git a/packages/core/solidity/src/set-upgradeable.ts b/packages/core/solidity/src/set-upgradeable.ts index ff6502805..bdd72198d 100644 --- a/packages/core/solidity/src/set-upgradeable.ts +++ b/packages/core/solidity/src/set-upgradeable.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' }],