diff --git a/.changeset/legal-cameras-sin.md b/.changeset/legal-cameras-sin.md new file mode 100644 index 00000000000..850aea2bc68 --- /dev/null +++ b/.changeset/legal-cameras-sin.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`AccessManager`: treat `setAuthority` differently in `canCall` to prevent bypassing the `updateAuthority` security using an `execute`. diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 12a734d4075..e52a62a481e 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -148,6 +148,11 @@ contract AccessManager is Context, Multicall, IAccessManager { // Caller is AccessManager, this means the call was sent through {execute} and it already checked // permissions. We verify that the call "identifier", which is set during {execute}, is correct. return (_isExecuting(target, selector), 0); + } else if (selector == IAccessManaged.setAuthority.selector) { + (bool isAdmin, uint32 executionDelay) = hasRole(ADMIN_ROLE, caller); + uint32 adminDelay = getTargetAdminDelay(target); + uint32 setAuthorityDelay = uint32(Math.max(executionDelay, adminDelay)); + return isAdmin ? (setAuthorityDelay == 0, setAuthorityDelay) : (false, 0); } else { uint64 roleId = getTargetFunctionRole(target, selector); (bool isMember, uint32 currentDelay) = hasRole(roleId, caller); @@ -388,6 +393,11 @@ contract AccessManager is Context, Multicall, IAccessManager { * Emits a {TargetFunctionRoleUpdated} event. */ function _setTargetFunctionRole(address target, bytes4 selector, uint64 roleId) internal virtual { + if (selector == IAccessManaged.setAuthority.selector) { + // Prevent updating authority using an execute call, instead only allow it through updateAuthority to + // ensure the proper delay and admin restrictions are applied. + revert AccessManagerLockedFunction(selector); + } _targets[target].allowedRoles[selector] = roleId; emit TargetFunctionRoleUpdated(target, selector, roleId); } diff --git a/contracts/access/manager/IAccessManager.sol b/contracts/access/manager/IAccessManager.sol index 056a2f49f43..0eca267fe86 100644 --- a/contracts/access/manager/IAccessManager.sol +++ b/contracts/access/manager/IAccessManager.sol @@ -80,6 +80,7 @@ interface IAccessManager { error AccessManagerNotReady(bytes32 operationId); error AccessManagerExpired(bytes32 operationId); error AccessManagerLockedRole(uint64 roleId); + error AccessManagerLockedFunction(bytes4 selector); error AccessManagerBadConfirmation(); error AccessManagerUnauthorizedAccount(address msgsender, uint64 roleId); error AccessManagerUnauthorizedCall(address caller, address target, bytes4 selector); diff --git a/test/access/manager/AccessManager.behavior.js b/test/access/manager/AccessManager.behavior.js index 830700e3762..319a8e2c05a 100644 --- a/test/access/manager/AccessManager.behavior.js +++ b/test/access/manager/AccessManager.behavior.js @@ -57,20 +57,15 @@ function shouldBehaveLikeDelayedAdminOperation() { */ function shouldBehaveLikeNotDelayedAdminOperation() { const getAccessPath = LIKE_COMMON_GET_ACCESS; - - function testScheduleOperation(mineDelay) { - return function self() { - self.mineDelay = mineDelay; - beforeEach('set execution delay', async function () { - this.scheduleIn = this.executionDelay; // For testAsSchedulableOperation - }); - testAsSchedulableOperation(LIKE_COMMON_SCHEDULABLE); - }; - } - + testAsDelayedOperation.mineDelay = true; getAccessPath.requiredRoleIsGranted.roleGrantingIsDelayed.callerHasAnExecutionDelay.afterGrantDelay = - testScheduleOperation(true); - getAccessPath.requiredRoleIsGranted.roleGrantingIsNotDelayed.callerHasAnExecutionDelay = testScheduleOperation(false); + testAsDelayedOperation; + getAccessPath.requiredRoleIsGranted.roleGrantingIsNotDelayed.callerHasAnExecutionDelay = function () { + beforeEach('set execution delay', async function () { + this.scheduleIn = this.executionDelay; // For testAsSchedulableOperation + }); + testAsSchedulableOperation(LIKE_COMMON_SCHEDULABLE); + }; beforeEach('set target as manager', function () { this.target = this.manager; diff --git a/test/access/manager/AccessManager.predicate.js b/test/access/manager/AccessManager.predicate.js index 8b4c5f4b642..2c9daf9e4f6 100644 --- a/test/access/manager/AccessManager.predicate.js +++ b/test/access/manager/AccessManager.predicate.js @@ -275,7 +275,7 @@ function testAsDelayedOperation() { describe('when operation delay is greater than execution delay', function () { beforeEach('set operation delay', async function () { this.operationDelay = this.executionDelay + time.duration.hours(1); - await this.manager.$_setTargetAdminDelay(this.target, this.operationDelay); + await this.manager.$_setTargetAdminDelay(this.operationDelayTarget ?? this.target, this.operationDelay); this.scheduleIn = this.operationDelay; // For testAsSchedulableOperation }); @@ -285,7 +285,7 @@ function testAsDelayedOperation() { describe('when operation delay is shorter than execution delay', function () { beforeEach('set operation delay', async function () { this.operationDelay = this.executionDelay - time.duration.hours(1); - await this.manager.$_setTargetAdminDelay(this.target, this.operationDelay); + await this.manager.$_setTargetAdminDelay(this.operationDelayTarget ?? this.target, this.operationDelay); this.scheduleIn = this.executionDelay; // For testAsSchedulableOperation }); @@ -296,7 +296,7 @@ function testAsDelayedOperation() { describe('without operation delay', function () { beforeEach('set operation delay', async function () { this.operationDelay = 0n; - await this.manager.$_setTargetAdminDelay(this.target, this.operationDelay); + await this.manager.$_setTargetAdminDelay(this.operationDelayTarget ?? this.target, this.operationDelay); this.scheduleIn = this.executionDelay; // For testAsSchedulableOperation }); diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index 7726831b268..79536d69a2a 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -1063,6 +1063,7 @@ describe('AccessManager', function () { describe('restrictions', function () { beforeEach('set method and args', function () { + this.operationDelayTarget = this.newManagedTarget; this.calldata = this.manager.interface.encodeFunctionData('updateAuthority(address,address)', [ this.newManagedTarget.target, this.newAuthority.target, @@ -1086,6 +1087,7 @@ describe('AccessManager', function () { describe('#setTargetClosed', function () { describe('restrictions', function () { beforeEach('set method and args', function () { + this.operationDelayTarget = this.other; const args = [this.other.address, true]; const method = this.manager.interface.getFunction('setTargetClosed(address,bool)'); this.calldata = this.manager.interface.encodeFunctionData(method, args); @@ -1124,6 +1126,7 @@ describe('AccessManager', function () { describe('#setTargetFunctionRole', function () { describe('restrictions', function () { beforeEach('set method and args', function () { + this.operationDelayTarget = this.other; const args = [this.other.address, ['0x12345678'], 443342]; const method = this.manager.interface.getFunction('setTargetFunctionRole(address,bytes4[],uint64)'); this.calldata = this.manager.interface.encodeFunctionData(method, args); @@ -1162,6 +1165,15 @@ describe('AccessManager', function () { ); } }); + + it('reverts setting a function role for the setAuthority selector', async function () { + const { selector } = this.target.interface.getFunction('setAuthority'); + await expect( + this.manager.connect(this.admin).$_setTargetFunctionRole(this.target, selector, this.roles.ADMIN.id), + ) + .to.be.revertedWithCustomError(this.manager, 'AccessManagerLockedFunction') + .withArgs(selector); + }); }); describe('role admin operations', function () { @@ -2240,6 +2252,41 @@ describe('AccessManager', function () { .to.be.revertedWithCustomError(this.manager, 'AccessManagerNotScheduled') .withArgs(operationId); }); + + it('can execute a setAuthority call when no target admin delay is set', async function () { + const newAuthority = await ethers.deployContract('$AccessManager', [this.admin]); + + await expect(this.manager.getTargetAdminDelay(this.target)).to.eventually.equal(0n); + + await expect( + this.manager + .connect(this.admin) + .execute(this.target, this.target.interface.encodeFunctionData('setAuthority', [newAuthority.target])), + ) + .to.emit(this.target, 'AuthorityUpdated') + .withArgs(newAuthority); + }); + + it('cannot execute a setAuthority call when a target admin delay is set', async function () { + const newAuthority = await ethers.deployContract('$AccessManager', [this.admin]); + + await this.manager.$_setTargetAdminDelay(this.target, 10n); + await time.increaseBy.timestamp(5n * 86400n, true); // minSetBack is 5 days + + await expect(this.manager.getTargetAdminDelay(this.target)).to.eventually.equal(10n); + + // cannot update directly - there is a delay + await expect( + this.manager.connect(this.admin).updateAuthority(this.target, newAuthority), + ).to.be.revertedWithCustomError(this.manager, 'AccessManagerNotScheduled'); + + // cannot bypass via execute either - targetAdminDelay is enforced for setAuthority + await expect( + this.manager + .connect(this.admin) + .execute(this.target, this.target.interface.encodeFunctionData('setAuthority', [newAuthority.target])), + ).to.be.revertedWithCustomError(this.manager, 'AccessManagerNotScheduled'); + }); }); describe('#consumeScheduledOp', function () {