0x52 - Freezing roles in ERC721NFTProduct and ERC1155NFTProduct is moot

[sherlock-admin]

0x52

high

Freezing roles in ERC721NFTProduct and ERC1155NFTProduct is moot

Summary

In ERC721NFTProduct and ERC1155NFTProduct roles can be frozen which is supposed to lock role to current addresses and not allow any changes. The problem is that admin can still use AccessControlUpgradable#grantRole and revokeRole to grant and remove roles to addresses because hasRole allows "ADMIN_ROLE" to bypass all role restrictions even "DEFAULT_ADMIN_ROLE".

Vulnerability Detail

function hasRole(bytes32 role, address account)
    public
    view
    virtual
    override
    returns (bool)
{
    return
        super.hasRole(ADMIN_ROLE, account) || super.hasRole(role, account);
}

In GranularRoles.sol and AccessControlUpgradable.sol, developers are careful to never grant the "DEFAULT_ADMIN_ROLE" to any user. Additionally they never set the admin role of any role so that it's admin will remain "DEFAULT_ADMIN_ROLE". In theory this should make so that there is no way to grant or revoke roles outside of GranularRoles#_initRoles and updateRoles. The issue is that the override by GranularRoles#hasRole allows "ADMIN_ROLE" to bypass any role restriction including "DEFAULT_ADMIN_ROLE". This allows "ADMIN_ROLE" to directly call AccessControlUpgradable#grantRole and revokeRole, which makes the entire freezing system useless as it doesn't actually stop any role modification.

Impact

Freezing roles doesn't actually prevent "ADMIN_ROLE" from modifying roles as intended. Submitting as high due to gross over-extension of admin authority clearly violating intended guardrails.

Code Snippet

GranularRoles.sol#L87-L96

Tool used

Manual Review

Recommendation

Override AccessControlUpgradable#grantRole and revokeRole in GranularRoles.sol to revert when called:

 GranularRoles.sol

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

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

[Evert0x]

Admin role having more power than intended is not a med/high issue for the protocol team.

[hyperspacebunny]

@Evert0x This actually is valid and pretty high priority for us since it's a workaround for some pretty explicit rules in our permissions system. Can you reopen it?

[hyperspacebunny]

Fixed in https://github.com/nftport/evm-minting-sherlock-fixes/pull/15

[rayn731]

The fix will disable DEFAULT_ADMIN_ROLE to grant/revoke roles, but _owner still has the ability to grant/revoke roles even if all roles are frozen?

[hyperspacebunny]

Yup, our current intent is that the owner should always have control over the roles if they want to self-manage, freezing is just to remove the delegation to ADMIN_ROLE