From 3aa0189c0877572e10683fc7ba6fc55d850645dd Mon Sep 17 00:00:00 2001 From: Amie Corso Date: Thu, 28 May 2026 07:44:32 -0700 Subject: [PATCH 1/2] fix(roles): reject revokeRole on sole admin (BOP-196) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without this guard, revokeRole(DEFAULT_ADMIN_ROLE, soleAdmin) succeeds silently: the admin role is cleared, adminCount drops to 0, and every future admin operation reverts unauthorized. No LastAdminRenounced event fires, so off-chain consumers see no signal that the token has reached its terminal admin-less state. Adds the same last-admin guard already present in renounceRole: when role == DEFAULT_ADMIN_ROLE, the target holds it, and adminCount == 1, revert LastAdminCannotRenounce. The renounceLastAdmin() path remains the only legitimate way to remove the final admin, and it emits the explicit terminal signal. Per Slack consensus from Conner: error when admin count is 1. Test added: test_revokeRole_revert_lastAdmin demonstrates the bug + verifies the fix; complemented by test_revokeRole_success_revokesNonSoleAdmin which pins the multi-admin-allowed case. Existing _success_ tests update their natspec to reference the new guard instead of the (previously-wrong) reasoning. Mock suite: 501/0/0. Note: the Rust precompile has the same bug — the new test currently fails in fork mode and will flip to passing once base/base ships the mirror fix (BOP-197, assigned to Andreas). --- test/lib/mocks/MockB20.sol | 11 ++++++ test/unit/B20/roles/revokeRole.t.sol | 50 ++++++++++++++++++++++++++-- 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/test/lib/mocks/MockB20.sol b/test/lib/mocks/MockB20.sol index 14c78e51..8a278bc7 100644 --- a/test/lib/mocks/MockB20.sol +++ b/test/lib/mocks/MockB20.sol @@ -274,6 +274,17 @@ contract MockB20 is IB20 { } function revokeRole(bytes32 role, address account) external onlyRoleAdmin(role) { + // Last-admin guard: prevent silently bricking the token by removing the sole + // DEFAULT_ADMIN_ROLE holder via revokeRole. The dedicated `renounceLastAdmin()` + // path remains the only legitimate way to clear the final admin (and emits the + // explicit `LastAdminRenounced` signal). Mirrors the same guard already present + // in `renounceRole`. The check fires only when the target ACTUALLY holds the + // role, preserving idempotent-no-op semantics for revoke calls against accounts + // that don't hold DEFAULT_ADMIN_ROLE. + MockB20Storage.Layout storage $ = MockB20Storage.layout(); + if (role == DEFAULT_ADMIN_ROLE && $.roles[DEFAULT_ADMIN_ROLE][account] && $.adminCount == 1) { + revert LastAdminCannotRenounce(); + } _revokeRole(role, account); } diff --git a/test/unit/B20/roles/revokeRole.t.sol b/test/unit/B20/roles/revokeRole.t.sol index 62317774..1955d764 100644 --- a/test/unit/B20/roles/revokeRole.t.sol +++ b/test/unit/B20/roles/revokeRole.t.sol @@ -23,10 +23,28 @@ contract B20RevokeRoleTest is B20Test { token.revokeRole(role, account); } + /// @notice Verifies revokeRole reverts when the call would remove the sole DEFAULT_ADMIN_ROLE holder + /// @dev BOP-196 regression. Without this guard, `revokeRole(DEFAULT_ADMIN_ROLE, soleAdmin)` would + /// succeed silently, bricking the token: admin operations become unreachable, and no + /// `LastAdminRenounced` event is emitted to signal the terminal state. The dedicated + /// `renounceLastAdmin()` path remains the only legitimate way to remove the final admin. + /// Reverts with `LastAdminCannotRenounce` (reused from the renounceRole guard — same + /// invariant: the last admin can only be removed via the explicit + /// `renounceLastAdmin` path). + function test_revokeRole_revert_lastAdmin() public { + assertEq(token.hasRole(B20Constants.DEFAULT_ADMIN_ROLE, admin), true, "precondition: admin is the sole admin"); + + vm.prank(admin); + vm.expectRevert(IB20.LastAdminCannotRenounce.selector); + token.revokeRole(B20Constants.DEFAULT_ADMIN_ROLE, admin); + } + /// @notice Verifies revokeRole sets hasRole(role, account) to false /// @dev Read-after-write; canonical hasRole readback test lives in hasRole.t.sol. /// Skips revoking DEFAULT_ADMIN_ROLE from the sole bootstrap admin - /// (would require renounceLastAdmin instead, covered in that file). + /// (would revert with LastAdminCannotRenounce per the BOP-196 guard; + /// see test_revokeRole_revert_lastAdmin and renounceLastAdmin.t.sol + /// for the terminal-admin removal path). /// Paired slot assertion: the `roles[role][account]` slot /// reads back as zero after the revoke. function test_revokeRole_success_clearsRole(bytes32 role, address account) public { @@ -43,8 +61,36 @@ contract B20RevokeRoleTest is B20Test { ); } + /// @notice Verifies revokeRole successfully removes a non-sole admin + /// @dev With multiple admins, revoking one is permitted because adminCount stays >= 1 + /// and the token remains operable. Complements test_revokeRole_revert_lastAdmin + /// which proves the guard only fires when the count would drop to zero. + function test_revokeRole_success_revokesNonSoleAdmin(address otherAdmin) public { + _assumeValidActor(otherAdmin); + vm.assume(otherAdmin != admin); + _grantRole(B20Constants.DEFAULT_ADMIN_ROLE, otherAdmin); + assertEq( + uint256(vm.load(address(token), MockB20Storage.adminCountSlot())), + 2, + "precondition: two admins exist" + ); + + vm.prank(admin); + token.revokeRole(B20Constants.DEFAULT_ADMIN_ROLE, otherAdmin); + + assertFalse(token.hasRole(B20Constants.DEFAULT_ADMIN_ROLE, otherAdmin), "otherAdmin must lose admin role"); + assertTrue(token.hasRole(B20Constants.DEFAULT_ADMIN_ROLE, admin), "original admin retains role"); + assertEq( + uint256(vm.load(address(token), MockB20Storage.adminCountSlot())), + 1, + "adminCount must drop from 2 to 1" + ); + } + /// @notice Verifies revokeRole is idempotent when the account does not hold the role - /// @dev No-op for not-held accounts; no revert, no duplicate event. + /// @dev No-op for not-held accounts; no revert (including for the + /// DEFAULT_ADMIN_ROLE-on-non-holder case, since the last-admin guard only + /// fires when the target ACTUALLY holds the role), no duplicate event. function test_revokeRole_success_idempotent(bytes32 role, address account) public { vm.assume(!(role == B20Constants.DEFAULT_ADMIN_ROLE && account == admin)); assertFalse(token.hasRole(role, account), "precondition: role not held"); From 64f08154758842bce7ccf17795f96d223c39da95 Mon Sep 17 00:00:00 2001 From: Amie Corso Date: Thu, 28 May 2026 07:52:03 -0700 Subject: [PATCH 2/2] style: forge fmt Collapse short assertEq calls onto single lines per forge fmt. Fixes the CI format check on PR #91. Test behavior is unchanged. --- test/unit/B20/roles/revokeRole.t.sol | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/test/unit/B20/roles/revokeRole.t.sol b/test/unit/B20/roles/revokeRole.t.sol index 1955d764..086b6439 100644 --- a/test/unit/B20/roles/revokeRole.t.sol +++ b/test/unit/B20/roles/revokeRole.t.sol @@ -69,11 +69,7 @@ contract B20RevokeRoleTest is B20Test { _assumeValidActor(otherAdmin); vm.assume(otherAdmin != admin); _grantRole(B20Constants.DEFAULT_ADMIN_ROLE, otherAdmin); - assertEq( - uint256(vm.load(address(token), MockB20Storage.adminCountSlot())), - 2, - "precondition: two admins exist" - ); + assertEq(uint256(vm.load(address(token), MockB20Storage.adminCountSlot())), 2, "precondition: two admins exist"); vm.prank(admin); token.revokeRole(B20Constants.DEFAULT_ADMIN_ROLE, otherAdmin); @@ -81,9 +77,7 @@ contract B20RevokeRoleTest is B20Test { assertFalse(token.hasRole(B20Constants.DEFAULT_ADMIN_ROLE, otherAdmin), "otherAdmin must lose admin role"); assertTrue(token.hasRole(B20Constants.DEFAULT_ADMIN_ROLE, admin), "original admin retains role"); assertEq( - uint256(vm.load(address(token), MockB20Storage.adminCountSlot())), - 1, - "adminCount must drop from 2 to 1" + uint256(vm.load(address(token), MockB20Storage.adminCountSlot())), 1, "adminCount must drop from 2 to 1" ); }