AccessControl extensions don't have instructions for overriding functions defined more than once

[SvenMeyer]

Using AccessControlDefaultAdminRules or AccessControlEnumerable on its own looks straight forward.
However if I derive from both, I get quite some error messages.

`contract SecureContract is AccessControlDefaultAdminRules, AccessControlEnumerable, Pausable, ReentrancyGuard { ... }

Derived contract must override function "_grantRole". Two or more base classes define function with same name and parameter types.solidity(6480)
Derived contract must override function "_revokeRole". Two or more base classes define function with same name and parameter types.solidity(6480)
Derived contract must override function "_setRoleAdmin". Two or more base classes define function with same name and parameter types.solidity(6480)
Derived contract must override function "grantRole". Two or more base classes define function with same name and parameter types.solidity(6480)
Derived contract must override function "renounceRole". Two or more base classes define function with same name and parameter types.solidity(6480)
Derived contract must override function "revokeRole". Two or more base classes define function with same name and parameter types.solidity(6480)
Derived contract must override function "supportsInterface". Two or more base classes define function with same name and parameter types.solidity(6480)

Even if it is intented, I would be helpful the have example code in the documentation, however ideally just deriving from both should work and implement some reasonable default behaviour (if possible)

[ernestognw]

This is a good point and I agree it undermines the developer experience. However, I think the proposal has some nuances because neither Solidity nor us can define a default behavior for this because in order to disambiguate, one must assess the linearization order first.

https://docs.soliditylang.org/en/v0.8.20/contracts.html#multiple-inheritance-and-linearization

Basically, the order of execution of the clashing functions is determined by the order in which you put the inherited contracts, (i.e is not the same is A, B to is B, A).

In this particular case, I think the linearization order doesn't seem to cause issues so the only thing you should do is adding an override (AccessControlDefaultAdminRules, AccessControlEnumerable) to the clashing functions.

Curious to know, how would you think it'd be best to orient users in this regard?

[SvenMeyer]

Disclaimer : I am not (yet) an expert in multiple inheritance ...

1) First idea just for this specific issue.
Is there anything wrong with making AccessControl behave like (replace it with) AccessControlEnumerable by default ?

• There is a additional cost, but how often do roles really change ?
• Yes, it would be easier for everybody (not only the admin) to get an overview who has what rule, could that be a security risk? However, any decent attacker would be able to grab that information from a public chain anyway.

2) I understand that functions from child contracts overwrite each other, which is why order is important. However, in this case, function should be merged, like one function called first from AccessControlDefaultAdminRules (check rights) and then the function from AccessControlEnumerable (add to enumerable list).

I thought that I could make it work by creating a linear inheritance, leveraging the super... code lines, that i.e. TestERC20.grantrole calls TestERC20_ACDAR super.grantRole which does the checks and then execute its code to add a new entry to the list ...
That looks like the easiest solution to me ... however, I get the same errors as before :-(

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;

// OZ contracts v4.9.2
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/access/AccessControlDefaultAdminRules.sol";
import "@openzeppelin/contracts/access/AccessControlEnumerable.sol";

contract TestERC20_ACDAR is ERC20, AccessControlDefaultAdminRules {
    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

    constructor() ERC20("TestERC20", "TestERC20") AccessControlDefaultAdminRules (10 days, msg.sender) {
        // _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
        _grantRole(MINTER_ROLE, msg.sender);
    }

    function mint(address to, uint256 amount) public onlyRole(MINTER_ROLE) {
        _mint(to, amount);
    }
}

contract TestERC20 is TestERC20_ACDAR, AccessControlEnumerable {
    // nothing to see here
}

3) I think this is what you suggested? At least it compiles without error, but I had to make quite some changes, added code that I would not trust it. Honestly, I would not want to mess around with an OZ code on access control which has been carefully drafted and audited 😬 ... misses the whole point of developing a secure contract by using solid code as a base.

Maybe a OZ provide contract "AccessControlEnumerable + AccessControlDefaultAdminRules", but if going that route so many combinations may be needed that it becomes hard to maintain.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;

// OZ contracts v4.9.2
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/access/AccessControlDefaultAdminRules.sol";
import "@openzeppelin/contracts/access/AccessControlEnumerable.sol";

contract TestERC20 is ERC20, AccessControlEnumerable, AccessControlDefaultAdminRules {
    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

    constructor() ERC20("TestERC20", "TestERC20") AccessControlDefaultAdminRules(10 days, msg.sender) {
        // _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
        _grantRole(MINTER_ROLE, msg.sender);
    }

    function mint(address to, uint256 amount) public onlyRole(MINTER_ROLE) {
        _mint(to, amount);
    }

    function supportsInterface(
        bytes4 interfaceId
    ) public view override(AccessControlEnumerable, AccessControlDefaultAdminRules) returns (bool) {
        return
            interfaceId == type(IAccessControlEnumerable).interfaceId ||
            interfaceId == type(IAccessControlDefaultAdminRules).interfaceId ||
            super.supportsInterface(interfaceId); // Do I need this ???
    }

    function _grantRole(
        bytes32 role,
        address account
    ) internal override(AccessControlEnumerable, AccessControlDefaultAdminRules) {
        AccessControlDefaultAdminRules._grantRole(role, account);
        AccessControlEnumerable._grantRole(role, account);
    }

    function _revokeRole(
        bytes32 role,
        address account
    ) internal override(AccessControlEnumerable, AccessControlDefaultAdminRules) {
        AccessControlDefaultAdminRules._revokeRole(role, account);
        AccessControlEnumerable._revokeRole(role, account);
    }

    function _setRoleAdmin(
        bytes32 role,
        bytes32 adminRole
    ) internal override(AccessControl, AccessControlDefaultAdminRules) {
        AccessControlDefaultAdminRules._setRoleAdmin(role, adminRole);
        AccessControl._setRoleAdmin(role, adminRole);
    }

    function grantRole(
        bytes32 role,
        address account
    ) public override(AccessControlDefaultAdminRules, AccessControl, IAccessControl) {
        AccessControlDefaultAdminRules.grantRole(role, account);
    }

    function renounceRole(
        bytes32 role,
        address account
    ) public override(AccessControlDefaultAdminRules, AccessControl, IAccessControl) {
        AccessControlDefaultAdminRules.renounceRole(role, account);
    }

    function revokeRole(
        bytes32 role,
        address account
    ) public override(AccessControlDefaultAdminRules, AccessControl, IAccessControl) {
        AccessControlDefaultAdminRules.revokeRole(role, account);
    }
}

I do not even want to think about adding AccessControlCrossChain also to the mix ... 😬

[SvenMeyer]

4) Finally, I could copy-paste the whole code of AccessControlEnumerable. It works, but of course adds problems in terms of code bloat, ungradeability to new OZ contract versions, and renders the whole idea of multiple inheritance (largely) useless .. but I would trust it more than solution 3) as I did not have to change/invent any code.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;

// OZ contracts v4.9.2
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/access/AccessControlDefaultAdminRules.sol";
import "@openzeppelin/contracts/access/IAccessControlEnumerable.sol";
import "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";


contract TestERC20 is ERC20, AccessControlDefaultAdminRules {
    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

    constructor() ERC20("TestERC20", "TestERC20") AccessControlDefaultAdminRules (10 days, msg.sender) {
        // _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
        _grantRole(MINTER_ROLE, msg.sender);
    }

    // import "@openzeppelin/contracts/access/AccessControlEnumerable.sol"; - copy-paste whole code
    using EnumerableSet for EnumerableSet.AddressSet;

    mapping(bytes32 => EnumerableSet.AddressSet) private _roleMembers;

    /**
     * @dev See {IERC165-supportsInterface}.
     */
    function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) {
        return interfaceId == type(IAccessControlEnumerable).interfaceId || super.supportsInterface(interfaceId);
    }

    /**
     * @dev Returns one of the accounts that have `role`. `index` must be a
     * value between 0 and {getRoleMemberCount}, non-inclusive.
     *
     * Role bearers are not sorted in any particular way, and their ordering may
     * change at any point.
     *
     * WARNING: When using {getRoleMember} and {getRoleMemberCount}, make sure
     * you perform all queries on the same block. See the following
     * https://forum.openzeppelin.com/t/iterating-over-elements-on-enumerableset-in-openzeppelin-contracts/2296[forum post]
     * for more information.
     */
    function getRoleMember(bytes32 role, uint256 index) public view virtual returns (address) {
        return _roleMembers[role].at(index);
    }

    /**
     * @dev Returns the number of accounts that have `role`. Can be used
     * together with {getRoleMember} to enumerate all bearers of a role.
     */
    function getRoleMemberCount(bytes32 role) public view virtual returns (uint256) {
        return _roleMembers[role].length();
    }

    /**
     * @dev Overload {_grantRole} to track enumerable memberships
     */
    function _grantRole(bytes32 role, address account) internal virtual override {
        super._grantRole(role, account);
        _roleMembers[role].add(account);
    }

    /**
     * @dev Overload {_revokeRole} to track enumerable memberships
     */
    function _revokeRole(bytes32 role, address account) internal virtual override {
        super._revokeRole(role, account);
        _roleMembers[role].remove(account);
    }

}

I am actually surprised that 2) does not work ... or is just my syntax wrong ?

[ernestognw]

Hey @SvenMeyer, I edited your replies and added 10 days to the AccessControlDefaultAdminRules examples because I think it'd be dangerous if anyone comes here and picks the initial example with 0 days.

That said, I'm answering your questions:

1) First idea just for this specific issue.

Is there anything wrong with making AccessControl behave like (replace it with) AccessControlEnumerable by default ?

• There is a additional cost, but how often do roles really change ?
• Yes, it would be easier for everybody (not only the admin) to get an overview who has what rule, could that be a security risk? However, any decent attacker would be able to grab that information from a public chain anyway.

We think enumerability should usually be solved by offchain indexers and not something to be tracked onchain because of the code size and runtime costs. You can see more information about why we made this change here.

Also, yes, the access control information is always public and not supposed to be hidden. The security model doesn't rely on the privileged addresses to be unknown.

2) I understand that functions from child contracts overwrite each other ...

which is why order is important. However, in this case, function should be merged, like one function called first from AccessControlDefaultAdminRules (check rights) and then the function from AccessControlEnumerable (add to enumerable list).

You're sort of right. What changes the inheritance order is the order in which a contract is A, B, C. It doesn't matter if the clashing function uses override(A, B, C) or override(B, C, A), you just need to add the override modifier.

In the example you're sharing, the inheritance graph for TestERC20 is the same as if you were doing:

contract TestERC20 is ERC20, AccessControlDefaultAdminRules, AccessControlEnumerable { ... }

Therefore, you still need to use the override.

3) I think this is what you suggested?

At least it compiles without error, but I had to make quite some changes, added code that I would not trust it. Honestly, I would not want to mess around with an OZ code on access control which has been carefully drafted and audited 😬 ... misses the whole point of developing a secure contract by using solid code as a base.

Yes, it looks similar to what I suggested. I'd say there's no problem with making these overrides because this is only added behavior and it's not changing the code from OpenZeppelin Contracts

super.supportsInterface(interfaceId); // Do I need this ???

Yes, otherwise you'll lose the already supported interfaces from the inherited contracts. Particularly ERC165 in this case. Also both ERC721Enumerable and AccessControlDefaultAdminRules are already supported by using super afaik (may require testing though).

Maybe a OZ provide contract "AccessControlEnumerable + AccessControlDefaultAdminRules", but if going that route so many combinations may be needed that it becomes hard to maintain.

We're not fans of multiple versions of the same contract, also as I previously mentioned, the inheritance order is something we usually can't decide because it depends on the way users are composing contracts. In this particular case, it seems viable but the contract in our library will be exactly (or very similar) to the one you shared in point 3.

4) Finally, I could copy-paste the whole code of AccessControlEnumerabl

It works, but of course adds problems in terms of code bloat, ungradeability to new OZ contract versions, and renders the whole idea of multiple inheritance (largely) useless .. but I would trust it more than solution 3) as I did not have to change/invent any code.

⚠️ This is not advised as copypasting can break the security guarantees provided along with the contract.

The contract variables are private by design so the business logic remains untouched. The correct way of handling multiple inheritance is by following the linearization order.

I still agree with you that the linearization order could be a problem for newcomers, so perhaps we can suggest/think of better alternatives to document this behavior. Currently, this is our official inheritance recommendation from the docs.

I'll close this thread because it's becoming more support-related, which is provided in the forum, feel free to post your questions there and also feel free to open another issue if there's a concrete suggestion on how to improve the documentation for multiple inheritance.

[SvenMeyer]

@ernestognw Thanks a lot for your extensive reply explaining the options to approach this and your design decisions ! 👍