`Authorizer` access control issues: roles can be granted and revoked directly

[hats-bug-reporter[bot]]

Github username: @0xfuje Twitter username: 0xfuje Submission hash (on-chain): 0x94ec3c2f8f0e3eaafd4880abb7394d6d51bb135f038f2e5695eb6b7e651162b3 Severity: medium

Description:

Impact

Unauthorized execution of role restricted functions can lead to numerous issues

Description

The Authorizer module is based on openzeppelin's AccessControl contract (AccessControlEnumerableUpgradeable inherits AccessControlUpgradeable). The problem is that any role owner can grant the same role to infinite addresses and also revoke their own address via directly calling grantRole() and revokeRole(). This is an issue because it breaks the trust assumptions of the authorizer module.

lib/openzeppelin-contracts-upgradeable/contracts/access/AccessControlUpgradeable.sol

    function grantRole(bytes32 role, address account) public virtual onlyRole(getRoleAdmin(role)) {
        _grantRole(role, account);
    }

    function revokeRole(bytes32 role, address account) public virtual onlyRole(getRoleAdmin(role)) {
        _revokeRole(role, account);
    }

I will demonstrate a few problematic examples:

1. PAYMENT_PUSHER_ROLE starts to push malicious payments, so the owner will try to renounce their role, however he already duplicated the role 1000 times. To bypass the revokes, he will front-run revokes with more grantRole() calls.
2. There are multiple BOUNTY_ISSUER_ROLEs issuing bounties, however to stay ahead of the competition one of them decides to revoke every other issuer via calling revokeRole() directly.
3. There are multiple owners of an Orchestrator via AUT_ROLES_v1, however one of them decides to revoke each one, including himself, in the process permanently denying every owner restricted function.

---

Note that to give and revoke permissions of specific module roles, the malicious actors should generate the roles via generateRoleId(module, role)

Recommendation

Consider to override and revert grantRole() and revokeRole() functions in AUT_Roles_v1.sol:

    function grantRole(bytes32 role, address account) public virtual override onlyRole(getRoleAdmin(role)) {
        revert();
    }

    function revokeRole(bytes32 role, address account) public virtual override onlyRole(getRoleAdmin(role)) {
        revert();
    }

[FHieser]

This is an issue because it breaks the trust assumptions of the authorizer module. -> where does it break the trust assumption of the Authorizer? This should be invalid, because we assume that the roles are trusted and uncorrupted in the first place Or am I misreading something here?

[0xmahdirostami]

thanks @FHieser , the issue is invalid