From ccd5264bc5676e8dfd258438f7d8b33ca76e3d43 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 3 Mar 2026 17:41:05 +0100 Subject: [PATCH 1/5] poc: execute can be used to bypass target admin delay --- test/access/manager/AccessManager.test.js | 24 +++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index 7726831b268..78e333c06c5 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -1677,6 +1677,30 @@ describe('AccessManager', function () { }); }); }); + + it('BUG: execute can bypass target admin delay to update authority', async function () { + const newAuthority = await ethers.deployContract('$AccessManager', [this.admin]); + + // set a delay on the target - and wait for it to be active + 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'); + + // bypass the check by just doing execute + await expect( + this.manager + .connect(this.admin) + .execute(this.target, this.target.interface.encodeFunctionData('setAuthority', [newAuthority.target])), + ) + .to.emit(this.target, 'AuthorityUpdated') + .withArgs(newAuthority); + }); }); describe('access managed self operations', function () { From 983b24704c2ebd8299ee8f7c11fe0e47e2baebc4 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 3 Mar 2026 18:09:18 +0100 Subject: [PATCH 2/5] proposed fix --- contracts/access/manager/AccessManager.sol | 10 +++ contracts/access/manager/IAccessManager.sol | 1 + test/access/manager/AccessManager.test.js | 69 ++++++++++++++------- 3 files changed, 58 insertions(+), 22 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 12a734d4075..a189a4374e8 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 adminDelay) = hasRole(ADMIN_ROLE, caller); + uint32 targetAdminDelay = getTargetAdminDelay(target); + uint32 setAuthorityDelay = uint32(Math.max(adminDelay, targetAdminDelay)); + 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.test.js b/test/access/manager/AccessManager.test.js index 78e333c06c5..1a902cfe292 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -1678,28 +1678,53 @@ describe('AccessManager', function () { }); }); - it('BUG: execute can bypass target admin delay to update authority', async function () { - const newAuthority = await ethers.deployContract('$AccessManager', [this.admin]); - - // set a delay on the target - and wait for it to be active - 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'); - - // bypass the check by just doing execute - await expect( - this.manager - .connect(this.admin) - .execute(this.target, this.target.interface.encodeFunctionData('setAuthority', [newAuthority.target])), - ) - .to.emit(this.target, 'AuthorityUpdated') - .withArgs(newAuthority); + describe.only('target admin delay bypass issue', function () { + it('can execute a setAuthority, if there is no target admin delay', async function () { + const newAuthority = await ethers.deployContract('$AccessManager', [this.admin]); + + // double check there is no delay + await expect(this.manager.getTargetAdminDelay(this.target)).to.eventually.equal(0n); + + // execute the setAuthority through the manager's execute function + 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, if there is an target admin delay', async function () { + const newAuthority = await ethers.deployContract('$AccessManager', [this.admin]); + + // set a delay on the target - and wait for it to be active + 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'); + + // bypass the check by just doing execute -- covered + await expect( + this.manager + .connect(this.admin) + .execute(this.target, this.target.interface.encodeFunctionData('setAuthority', [newAuthority.target])), + ).to.be.revertedWithCustomError(this.manager, 'AccessManagerNotScheduled'); + }); + + it('cannot set a function role for setAuthority', 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); + }); }); }); From 021cae1b100fe5a1cb7e01703485885d07e23242 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 3 Mar 2026 18:13:49 +0100 Subject: [PATCH 3/5] add changeset --- .changeset/legal-cameras-sin.md | 5 +++++ test/access/manager/AccessManager.test.js | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 .changeset/legal-cameras-sin.md 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/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index 1a902cfe292..cc332bc5655 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -1678,7 +1678,7 @@ describe('AccessManager', function () { }); }); - describe.only('target admin delay bypass issue', function () { + describe('target admin delay bypass issue', function () { it('can execute a setAuthority, if there is no target admin delay', async function () { const newAuthority = await ethers.deployContract('$AccessManager', [this.admin]); From 34b62675d84ecae075555c8104e9da37429c7e9c Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 3 Mar 2026 11:29:00 -0600 Subject: [PATCH 4/5] up --- contracts/access/manager/AccessManager.sol | 6 +++--- test/access/manager/AccessManager.behavior.js | 21 +++++++------------ .../access/manager/AccessManager.predicate.js | 6 +++--- test/access/manager/AccessManager.test.js | 3 +++ 4 files changed, 17 insertions(+), 19 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index a189a4374e8..e52a62a481e 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -149,9 +149,9 @@ contract AccessManager is Context, Multicall, IAccessManager { // 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 adminDelay) = hasRole(ADMIN_ROLE, caller); - uint32 targetAdminDelay = getTargetAdminDelay(target); - uint32 setAuthorityDelay = uint32(Math.max(adminDelay, targetAdminDelay)); + (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); 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 cc332bc5655..f342c6894d2 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); From dc7e2dec91dacd54291ab11b98b71f368ee419ec Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 3 Mar 2026 11:35:44 -0600 Subject: [PATCH 5/5] up --- test/access/manager/AccessManager.test.js | 93 +++++++++++------------ 1 file changed, 44 insertions(+), 49 deletions(-) diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index f342c6894d2..79536d69a2a 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -1165,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 () { @@ -1680,55 +1689,6 @@ describe('AccessManager', function () { }); }); }); - - describe('target admin delay bypass issue', function () { - it('can execute a setAuthority, if there is no target admin delay', async function () { - const newAuthority = await ethers.deployContract('$AccessManager', [this.admin]); - - // double check there is no delay - await expect(this.manager.getTargetAdminDelay(this.target)).to.eventually.equal(0n); - - // execute the setAuthority through the manager's execute function - 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, if there is an target admin delay', async function () { - const newAuthority = await ethers.deployContract('$AccessManager', [this.admin]); - - // set a delay on the target - and wait for it to be active - 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'); - - // bypass the check by just doing execute -- covered - await expect( - this.manager - .connect(this.admin) - .execute(this.target, this.target.interface.encodeFunctionData('setAuthority', [newAuthority.target])), - ).to.be.revertedWithCustomError(this.manager, 'AccessManagerNotScheduled'); - }); - - it('cannot set a function role for setAuthority', 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('access managed self operations', function () { @@ -2292,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 () {