diff --git a/.changeset/light-news-listen.md b/.changeset/light-news-listen.md new file mode 100644 index 00000000000..1572d908139 --- /dev/null +++ b/.changeset/light-news-listen.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`AccessManager`: Allow the `onlyAuthorized` modifier to restrict functions added to the manager. diff --git a/CODE_OF_CONDUCT.md b/CODE_OF_CONDUCT.md index cd7770dac27..f7cdce98e08 100644 --- a/CODE_OF_CONDUCT.md +++ b/CODE_OF_CONDUCT.md @@ -6,7 +6,7 @@ In the interest of fostering an open and welcoming environment, we as contributors and maintainers pledge to making participation in our project and our community a harassment-free experience for everyone, regardless of age, body size, disability, ethnicity, sex characteristics, gender identity and expression, -level of experience, education, socio-economic status, nationality, personal +level of experience, education, socioeconomic status, nationality, personal appearance, race, religion, or sexual identity and orientation. ## Our Standards diff --git a/certora/diff/access_manager_AccessManager.sol.patch b/certora/diff/access_manager_AccessManager.sol.patch index 29ff9234677..cfb6cdf2f06 100644 --- a/certora/diff/access_manager_AccessManager.sol.patch +++ b/certora/diff/access_manager_AccessManager.sol.patch @@ -64,8 +64,8 @@ */ function _getAdminRestrictions( bytes calldata data -- ) private view returns (bool restricted, uint64 roleAdminId, uint32 executionDelay) { -+ ) internal view returns (bool restricted, uint64 roleAdminId, uint32 executionDelay) { // private → internal for FV +- ) private view returns (bool adminRestricted, uint64 roleAdminId, uint32 executionDelay) { ++ ) internal view returns (bool adminRestricted, uint64 roleAdminId, uint32 executionDelay) { // private → internal for FV if (data.length < 4) { return (false, 0, 0); } diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index e59c50e665b..76abea41c9a 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -412,9 +412,6 @@ contract AccessManager is Context, Multicall, IAccessManager { * Emits a {TargetClosed} event. */ function _setTargetClosed(address target, bool closed) internal virtual { - if (target == address(this)) { - revert AccessManagerLockedAccount(target); - } _targets[target].closed = closed; emit TargetClosed(target, closed); } @@ -586,7 +583,9 @@ contract AccessManager is Context, Multicall, IAccessManager { // ================================================= ADMIN LOGIC ================================================== /** - * @dev Check if the current call is authorized according to admin logic. + * @dev Check if the current call is authorized according to admin and roles logic. + * + * WARNING: Carefully review the considerations of {AccessManaged-restricted} since they apply to this modifier. */ function _checkAuthorized() private { address caller = _msgSender(); @@ -611,7 +610,7 @@ contract AccessManager is Context, Multicall, IAccessManager { */ function _getAdminRestrictions( bytes calldata data - ) private view returns (bool restricted, uint64 roleAdminId, uint32 executionDelay) { + ) private view returns (bool adminRestricted, uint64 roleAdminId, uint32 executionDelay) { if (data.length < 4) { return (false, 0, 0); } @@ -648,7 +647,7 @@ contract AccessManager is Context, Multicall, IAccessManager { return (true, getRoleAdmin(roleId), 0); } - return (false, 0, 0); + return (false, getTargetFunctionRole(address(this), selector), 0); } // =================================================== HELPERS ==================================================== @@ -673,7 +672,7 @@ contract AccessManager is Context, Multicall, IAccessManager { } /** - * @dev A version of {canCall} that checks for admin restrictions in this contract. + * @dev A version of {canCall} that checks for restrictions in this contract. */ function _canCallSelf(address caller, bytes calldata data) private view returns (bool immediate, uint32 delay) { if (data.length < 4) { @@ -686,8 +685,10 @@ contract AccessManager is Context, Multicall, IAccessManager { return (_isExecuting(address(this), _checkSelector(data)), 0); } - (bool enabled, uint64 roleId, uint32 operationDelay) = _getAdminRestrictions(data); - if (!enabled) { + (bool adminRestricted, uint64 roleId, uint32 operationDelay) = _getAdminRestrictions(data); + + // isTragetClosed apply to non-admin-restricted function + if (!adminRestricted && isTargetClosed(address(this))) { return (false, 0); } diff --git a/contracts/access/manager/IAccessManager.sol b/contracts/access/manager/IAccessManager.sol index 697301d1c4a..50637e90dbf 100644 --- a/contracts/access/manager/IAccessManager.sol +++ b/contracts/access/manager/IAccessManager.sol @@ -82,7 +82,6 @@ interface IAccessManager { error AccessManagerNotScheduled(bytes32 operationId); error AccessManagerNotReady(bytes32 operationId); error AccessManagerExpired(bytes32 operationId); - error AccessManagerLockedAccount(address account); error AccessManagerLockedRole(uint64 roleId); error AccessManagerBadConfirmation(); error AccessManagerUnauthorizedAccount(address msgsender, uint64 roleId); @@ -108,7 +107,7 @@ interface IAccessManager { * is backward compatible. Some contracts may thus ignore the second return argument. In that case they will fail * to identify the indirect workflow, and will consider calls that require a delay to be forbidden. * - * NOTE: This function does not report the permissions of this manager itself. These are defined by the + * NOTE: This function does not report the permissions of the admin functions in the manager itself. These are defined by the * {AccessManager} documentation. */ function canCall( @@ -134,6 +133,8 @@ interface IAccessManager { /** * @dev Get whether the contract is closed disabling any access. Otherwise role permissions are applied. + * + * NOTE: When the manager itself is closed, admin functions are still accessible to avoid locking the contract. */ function isTargetClosed(address target) external view returns (bool); @@ -308,6 +309,8 @@ interface IAccessManager { /** * @dev Set the closed flag for a contract. * + * Closing the manager itself won't disable access to admin methods to avoid locking the contract. + * * Requirements: * * - the caller must be a global admin diff --git a/contracts/mocks/AccessManagerMock.sol b/contracts/mocks/AccessManagerMock.sol new file mode 100644 index 00000000000..952c761199d --- /dev/null +++ b/contracts/mocks/AccessManagerMock.sol @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.20; + +import {AccessManager} from "../access/manager/AccessManager.sol"; +import {StorageSlot} from "../utils/StorageSlot.sol"; + +contract AccessManagerMock is AccessManager { + event CalledRestricted(address caller); + event CalledUnrestricted(address caller); + + constructor(address initialAdmin) AccessManager(initialAdmin) {} + + function fnRestricted() public onlyAuthorized { + emit CalledRestricted(msg.sender); + } + + function fnUnrestricted() public { + emit CalledUnrestricted(msg.sender); + } +} diff --git a/test/access/manager/AccessManager.behavior.js b/test/access/manager/AccessManager.behavior.js index c9e236eb0f8..385da578c0b 100644 --- a/test/access/manager/AccessManager.behavior.js +++ b/test/access/manager/AccessManager.behavior.js @@ -193,9 +193,65 @@ function shouldBehaveLikeAManagedRestrictedOperation() { }); } +/** + * @requires this.{target,manager,roles,calldata,role} + */ +function shouldBehaveLikeASelfRestrictedOperation() { + function revertUnauthorized() { + it('reverts as AccessManagerUnauthorizedAccount', async function () { + await expect(this.caller.sendTransaction({ to: this.target, data: this.calldata })) + .to.be.revertedWithCustomError(this.manager, 'AccessManagerUnauthorizedAccount') + .withArgs(this.caller, this.role?.id ?? 0n); + }); + } + + const getAccessPath = LIKE_COMMON_GET_ACCESS; + + function testScheduleOperation(mineDelay) { + return function self() { + self.mineDelay = mineDelay; + beforeEach('sets execution delay', async function () { + this.scheduleIn = this.executionDelay; // For testAsSchedulableOperation + }); + testAsSchedulableOperation(LIKE_COMMON_SCHEDULABLE); + }; + } + + getAccessPath.requiredRoleIsGranted.roleGrantingIsDelayed.callerHasAnExecutionDelay.afterGrantDelay = + testScheduleOperation(true); + getAccessPath.requiredRoleIsGranted.roleGrantingIsNotDelayed.callerHasAnExecutionDelay = testScheduleOperation(false); + + beforeEach('set target as manager', function () { + this.target = this.manager; + }); + + const isExecutingPath = LIKE_COMMON_IS_EXECUTING; + isExecutingPath.notExecuting = revertUnauthorized; + + testAsCanCall({ + closed: revertUnauthorized, + open: { + callerIsTheManager: isExecutingPath, + callerIsNotTheManager: { + publicRoleIsRequired() { + it('succeeds called directly', async function () { + await this.caller.sendTransaction({ to: this.target, data: this.calldata }); + }); + + it('succeeds via execute', async function () { + await this.manager.connect(this.caller).execute(this.target, this.calldata); + }); + }, + specificRoleIsRequired: getAccessPath, + }, + }, + }); +} + module.exports = { shouldBehaveLikeDelayedAdminOperation, shouldBehaveLikeNotDelayedAdminOperation, shouldBehaveLikeRoleAdminOperation, shouldBehaveLikeAManagedRestrictedOperation, + shouldBehaveLikeASelfRestrictedOperation, }; diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index 07d7b5a19c5..7726831b268 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -23,6 +23,7 @@ const { shouldBehaveLikeNotDelayedAdminOperation, shouldBehaveLikeRoleAdminOperation, shouldBehaveLikeAManagedRestrictedOperation, + shouldBehaveLikeASelfRestrictedOperation, } = require('./AccessManager.behavior'); const { @@ -48,7 +49,7 @@ async function fixture() { roles.SOME.members = [member]; roles.PUBLIC.members = [admin, roleAdmin, roleGuardian, member, user, other]; - const manager = await ethers.deployContract('$AccessManager', [admin]); + const manager = await ethers.deployContract('$AccessManagerMock', [admin]); const target = await ethers.deployContract('$AccessManagedTarget', [manager]); for (const { id: roleId, admin, guardian, members } of Object.values(roles)) { @@ -1105,10 +1106,18 @@ describe('AccessManager', function () { expect(await this.manager.isTargetClosed(this.target)).to.be.false; }); - it('reverts if closing the manager', async function () { - await expect(this.manager.connect(this.admin).setTargetClosed(this.manager, true)) - .to.be.revertedWithCustomError(this.manager, 'AccessManagerLockedAccount') - .withArgs(this.manager); + describe('when the target is the manager', async function () { + it('closes and opens the manager', async function () { + await expect(this.manager.connect(this.admin).setTargetClosed(this.manager, true)) + .to.emit(this.manager, 'TargetClosed') + .withArgs(this.manager, true); + expect(await this.manager.isTargetClosed(this.manager)).to.be.true; + + await expect(this.manager.connect(this.admin).setTargetClosed(this.manager, false)) + .to.emit(this.manager, 'TargetClosed') + .withArgs(this.manager, false); + expect(await this.manager.isTargetClosed(this.manager)).to.be.false; + }); }); }); @@ -1670,18 +1679,74 @@ describe('AccessManager', function () { }); }); + describe('access managed self operations', function () { + describe('when calling a restricted target function', function () { + const method = 'fnRestricted()'; + + beforeEach('set required role', async function () { + this.role = { id: 785913n }; + await this.manager.$_setTargetFunctionRole( + this.manager, + this.manager[method].getFragment().selector, + this.role.id, + ); + }); + + describe('restrictions', function () { + beforeEach('set method and args', function () { + this.caller = this.user; + this.calldata = this.manager.interface.encodeFunctionData(method, []); + }); + + shouldBehaveLikeASelfRestrictedOperation(); + }); + + it('succeeds called by a role member', async function () { + await this.manager.$_grantRole(this.role.id, this.user, 0, 0); + + await expect(this.manager.connect(this.user)[method]()) + .to.emit(this.manager, 'CalledRestricted') + .withArgs(this.user); + }); + }); + + describe('when calling a non-restricted target function', function () { + const method = 'fnUnrestricted()'; + + beforeEach('set required role', async function () { + this.role = { id: 879435n }; + await this.manager.$_setTargetFunctionRole( + this.manager, + this.manager[method].getFragment().selector, + this.role.id, + ); + }); + + it('succeeds called by anyone', async function () { + await expect(this.manager.connect(this.user)[method]()) + .to.emit(this.manager, 'CalledUnrestricted') + .withArgs(this.user); + }); + }); + }); + describe('access managed target operations', function () { describe('when calling a restricted target function', function () { - beforeEach('set required role', function () { - this.method = this.target.fnRestricted.getFragment(); + const method = 'fnRestricted()'; + + beforeEach('set required role', async function () { this.role = { id: 3597243n }; - this.manager.$_setTargetFunctionRole(this.target, this.method.selector, this.role.id); + await this.manager.$_setTargetFunctionRole( + this.target, + this.target[method].getFragment().selector, + this.role.id, + ); }); describe('restrictions', function () { beforeEach('set method and args', function () { - this.calldata = this.target.interface.encodeFunctionData(this.method, []); this.caller = this.user; + this.calldata = this.target.interface.encodeFunctionData(method, []); }); shouldBehaveLikeAManagedRestrictedOperation(); @@ -1690,11 +1755,7 @@ describe('AccessManager', function () { it('succeeds called by a role member', async function () { await this.manager.$_grantRole(this.role.id, this.user, 0, 0); - await expect( - this.target.connect(this.user)[this.method.selector]({ - data: this.calldata, - }), - ) + await expect(this.target.connect(this.user)[method]()) .to.emit(this.target, 'CalledRestricted') .withArgs(this.user); }); @@ -1713,11 +1774,7 @@ describe('AccessManager', function () { }); it('succeeds called by anyone', async function () { - await expect( - this.target.connect(this.user)[method]({ - data: this.calldata, - }), - ) + await expect(this.target.connect(this.user)[method]()) .to.emit(this.target, 'CalledUnrestricted') .withArgs(this.user); });