fix(roles): reject revokeRole on sole admin (BOP-196)

[amiecorso]

Summary

Closes BOP-196.

revokeRole(DEFAULT_ADMIN_ROLE, soleAdmin) succeeded silently before this PR.

The dedicated renounceLastAdmin() path was intended to be the only legitimate way to remove the final admin — renounceRole already has the equivalent guard. revokeRole was missing it.

The fix

Mirror the existing renounceRole guard inside revokeRole:

function revokeRole(bytes32 role, address account) external onlyRoleAdmin(role) {
    MockB20Storage.Layout storage $ = MockB20Storage.layout();
    if (role == DEFAULT_ADMIN_ROLE && $.roles[DEFAULT_ADMIN_ROLE][account] && $.adminCount == 1) {
        revert LastAdminCannotRenounce();
    }
    _revokeRole(role, account);
}

Tests

• test_revokeRole_revert_lastAdmin (NEW): regression coverage for the bug. The exact test that should have existed from day one — it would have caught this immediately. Fails without the fix, passes with it.
• test_revokeRole_success_revokesNonSoleAdmin (NEW): pins the multi-admin-allowed case so the guard isn't over-restrictive.
• Existing _success_ tests retain their vm.assume(!(role == DEFAULT_ADMIN_ROLE && account == admin)) exclusion (the case is now covered by the dedicated revert test), with natspec updated to reference the new guard instead of the old (incorrect) "would require renounceLastAdmin instead" reasoning.

How was this missed before?

Same pattern as the audit findings in PR #89: every existing test_revokeRole_success_* had vm.assume(!(role == DEFAULT_ADMIN_ROLE && account == admin)) — an exclusion based on the test author's incorrect mental model of the code. The comment even spelled out the wrong assumption: "would require renounceLastAdmin instead." The implementation enforced no such thing, and the vm.assume precisely excluded the bug-housing input from fuzz.

[linear]

BOP-196

[ilikesymmetry]

Thanks! Can we confirm we also have this logic built in for renounceRole? renouceLastAdmin should be the only path to remove last admin

[refcell]

Thanks! Can we confirm we also have this logic built in for renounceRole? renouceLastAdmin should be the only path to remove last admin

renounceRole also reverts if the caller is the last admin. See here