Redesigning Access Control for the OpenZeppelin Contracts

One of our goals outlined in our January-March roadmap for OpenZeppelin Contracts is to reduce library complexity. A key idea here is a revamp of our solution for Access Control, something that we've been discussing for some time.

This post will describe what we want to achieve in this re-design and go over possible implementations, with the goal of gathering feedback and choosing the version OpenZeppelin Contracts v3.0 will ship with.

The Goal

We want to have an Access Control library that is easy to use, hard to misuse, and we're confident is appropriate for the security needs of most if not all projects.

Ultimately, we want for usage of this library to be suggested in our security audits.

Scope

Our library will be based around the concept of roles: we want for users to restrict actions so that only accounts that posses a certain role can perform them.

Additionally, we will also handle role management: how they are are granted and revoked.

Assumptions & Threat Model

Because this is a generic library, we can only make broad assumptions about its usage, which will inform design decisions:

  1. Roles should be transferable. For different reasons, people may need to switch accounts (such as from a hot to a cold wallet): this behavior should be allowed.
  2. Roles can be granted by other accounts with the role. This is deduced from (1): it is possible to transfer the role to a smart contract that can only be used by a set of accounts, effectively granting the role to all of them.
  3. Roles should be removable. In the event of a compromised account (e.g. lost or stolen laptop), it should be possible to revoke the role from that account. This is only useful before the compromised account is used by an attacker: at that point, the damage made is system-dependent, potentially catastrophic (minting of tokens, withdrawal of funds, etc.).

Proposals

edit (15-Feb-2020): For the latest proposal, head to this post.

We came up with a number of variants of increasing degrees of sophistication. The final version will probably not be one of these copied verbatim, but rather a mix of the ideas they present.

All of these proposals make use of the Roles library that is already part of the OpenZeppelin Contracts: I suggest giving it a quick review before moving on.

Sidenote: Role Identifiers

All of the proposals below use bytes32 computed as the result of a hash to identify the different roles.

Expand this to learn why this is considered the best approach
  • They don't have the issues associated with strings, unless computed off-chain (web3.utils.soliditySha3("ROLE_NAME"))
  • They don't have the issues associated with integers, namely, choosing how to associate a role name to an integer
  • Unlike bytes32 = "string", all 256 bits of the identifier are random-ish, increasing the Hamming distance between them
  • With a consistent convention, clashes are not possible (e.g. by always doing bytes32 constant X_ROLE = keccak256("X"))
  • They don't take up storage by being constant
  • They are reasonably cheap: calling a pure function on a contract that returns such a hash costs ~250 gas (this figure includes the overhead of decoding calldata, jumping based on the function selector, etc.)
  • They can be expanded to be immutable once it is released

1. Barebones

This solution is simple and straightforward: any account with a role can grant it and revoke it.

AccessControl.sol
pragma solidity ^0.6.0;

import "./Roles.sol";

contract AccessControl {
    using Roles for Roles.Role;

    mapping (bytes32 => Roles.Role) _roles;

    function hasRole(bytes32 roleId, address account)
        public view returns (bool)
    {
        return _roles[roleId].has(account);
    }

    function transferRole(bytes32 roleId, address account) external {
        require(
            hasRole(roleId, msg.sender),
            "AccessControl: sender must have the role to transfer"
        );

        _revokeRole(roleId, msg.sender);
        _grantRole(roleId, account);
    }

    function grantRole(bytes32 roleId, address account) external {
        require(
            hasRole(roleId, msg.sender),
            "AccessControl: sender must have the role to grant"
        );

        _grantRole(roleId, account);
    }

    function revokeRole(bytes32 roleId, address account) external {
        require(
            hasRole(roleId, msg.sender),
            "AccessControl: sender must have the role to revoke"
        );

        _revokeRole(roleId, account);
    }

    function _grantRole(bytes32 roleId, address account) internal {
        _roles[roleId].add(account);
    }

    function _revokeRole(bytes32 roleId, address account) internal {
        _roles[roleId].remove(account);
    }
}
Usage
pragma solidity ^0.6.0;

import "../access/AccessControl.sol";

contract AccessControlMock is AccessControl {
    bytes32 constant SOME_ROLE = keccak256("SOME_ROLE");

    constructor() public {
        _grantRole(SOME_ROLE, msg.sender);
    }

    function foo() public view returns (uint256) {
        return address(this).balance;
    }

    function bar() public view returns (uint256) {
        require(hasRole(SOME_ROLE, msg.sender));

        return address(this).balance;
    }
}

If we removed the external revokeRole, it'd be up to the user to define their own way to revoke roles. This could be achieved e.g. by creating a second role that has this special power, which might be error-prone.

At the same time, letting any role-bearer revoke their peers might be too powerful and undesirable for what is expected to be an emergency system.

2. Super Admin

This builds on top of the barebones proposal by adding a predefined, all-powerful role that can grant and revoke any role.

AccesControl.sol
pragma solidity ^0.6.0;

import "./Roles.sol";

contract AccessControl {
    using Roles for Roles.Role;

    mapping (bytes32 => Roles.Role) _roles;

    bytes32 constant SUPER_ADMIN_ROLE = keccak256("SUPER_ADMIN_ROLE");

    function hasRole(bytes32 roleId, address account)
        public view returns (bool)
    {
        return _roles[roleId].has(account);
    }

    function transferRole(bytes32 roleId, address account) external {
        require(
            hasRole(roleId, msg.sender),
            "AccessControl: sender must have the role to transfer"
        );

        _revokeRole(roleId, msg.sender);
        _grantRole(roleId, account);
    }

    function grantRole(bytes32 roleId, address account) external {
        require(
            hasRole(roleId, msg.sender) || hasRole(SUPER_ADMIN_ROLE, msg.sender),
            "AccessControl: sender must have the role to grant"
        );

        _grantRole(roleId, account);
    }

    function revokeRole(bytes32 roleId, address account) external {
        require(
            hasRole(SUPER_ADMIN_ROLE, msg.sender),
            "AccessControl: sender must be an admin to revoke"
        );

        _revokeRole(roleId, account);
    }

    function _grantRole(bytes32 roleId, address account) internal {
        _roles[roleId].add(account);
    }

    function _revokeRole(bytes32 roleId, address account) internal {
        _roles[roleId].remove(account);
    }
}

The addition of a baked-in super admin role makes management much easier: super admins are in charge of emergency response (revoking roles), while regular accounts are used for regular operations.

Note that the semantics behind hasRole have not changed: an admin does not have all roles, that is, it cannot perform actions that other roles can (though it can of course grant itself any role and then perform the action).

A disadvantage of this approach is that it might be too rigid, and creating a more-flexible system (e.g. with multiple levels of administrative power) could require too much custom code.

3. Granular Permissions

This makes a radical change on the Roles library: instead of each role being binary (an account has it or it doesn't), different permissions are instead associated with each one. These would be:

  • Permission to use the role
  • Permission to transfer and grant the role
  • Permission to revoke the role

(those changes are not included in the sample below, which is only illustrative - it is not complete)

AccessControl.sol
pragma solidity ^0.6.0;

import "./Roles.sol";

contract AccessControl {
    using Roles for Roles.Role;

    mapping (bytes32 => Roles.Role) _roles;

    function hasRole(bytes32 roleId, address account)
        public view returns (bool)
    {
        return _roles[roleId].has(account);
    }

    function canGrantRole(bytes32 roleId, address account)
        public view returns (bool)
    {
        return _roles[roleId].canGrant(account);
    }

    function canRevokeRole(bytes32 roleId, address account)
        public view returns (bool)
    {
        return _roles[roleId].canRevoke(account);
    }

    function grantRole(bytes32 roleId, address account) external {
        require(
            canGrantRole(roleId, msg.sender)
            "AccessControl: sender cannot grant the role"
        );

        _grantRole(roleId, account);
    }

    function revokeRole(bytes32 roleId, address account) external {
        require(
            canRevokeRole(SUPER_ADMIN_ROLE, msg.sender),
            "AccessControl: sender cannot revoke the role"
        );

        _revokeRole(roleId, account);
    }

    function _grantRole(bytes32 roleId, address account) internal {
        _roles[roleId].add(account);
    }

    function _revokeRole(bytes32 roleId, address account) internal {
        _roles[roleId].remove(account);
    }
}

This approach is much more flexible, letting users decide which level of granularity they want with their permissions without the library being opinionated about it.

It might also be too flexible. Because we wouldn't want to include enums in the public API functions, we would need to add many external methods, such as allowToGrantRole, allowToRevokeRole, disallowToGrantRoles and disallowToRevokeRoles (and their internal variants).

Additionally, constructs like the Super Admin are not easy to make under this system: to give these powers to an account we'd need to call allowToGrantRole and allowToRevokeRole for every single role defined in the system.

Conclusion

Each proposal has different trade-offs: I'd like to gather feedback on them to inform the decision of what AccessControl will look like. It'd be specially useful to learn from real-life scenarios, such as strategies projects have already adopted, or situations where keys were lost and emergency measures had to be taken.

Thanks!

4 Likes

Great writeup @nventuro!

I wouldn't set this as a goal. The access control library we should strive for should be simple and easy to use, but I wouldn't try to build something that tries to cater for everyone's needs. If there is a project with certain needs, I'm fine if this access control library doesn't cut it for them. What should happen is that the library is not intertwined with other contracts, making it impossible to use a custom access control solution with the ERC implementations we provide.


From the writeup, I see the following decisions or attention points:

  • Are we ok using keccak(STRING) as role identifiers?
  • Do we want to support transferRole? How about just supporting grantRole? I'm worried about situations where the role is transferred to an incorrect account, and the user is locked out. This is similar to the difference between the vanilla Ownable and Claimable, where in the latter the recipient of ownership needs to accept it to prevent this.
  • In the granular permissions scenario, does it make sense to differentiate usage, grant, and revoke? Or could use&grant and revoke be enough?
1 Like

I would like to see an additional design goal of:

  • It is possible to iterate over all users in each role

I don’t mind having to iterate each role separately, though it would be nice if I could easily iterate over all privileged users of the system and get or query the role of each, but as long as there is some way to iterate over all users I’m content.

This is to make it possible to audit all privileged users of the system once logs start getting pruned (something that has been on various Ethereum client roadmaps for a while). If you are relying solely on logs for knowing the current state of the system, then once logs are pruned it can become impossible to know the state of the system.

3 Likes

It would be more convenient if one could just have the contract return an array of all users, rather than having to iterate on-chain. However, that has potential to limit the usefulness to applications who have a constrained set of users. An application where anyone can have a role (unbounded number of users) would make the ability to just return an array an attack vector.

I suspect if you want to try to target the broadest audience, you’ll have to sacrifice the usability benefits of “return an array” and instead opt to “support iteration”.

2 Likes

Thanks for the write up.

I think that roles should only have role functionality (though they could have the ability to renounce themselves).

My preference is for admin’s per role which can grant and revoke a specific role, so closer to option 3. With a super admin, I assume it would be all or nothing, without the ability to give up ability to grant/revoke a specific role.

An admin role should be renounceable so that this functionality can be removed, as projects where possible would evolve to remove roles to become more decentralized.

A minor point, if a role can renounce themselves, then the renounce function should require a parameter, ideally the address to renounce. This is to protect from accidental renouncing. I had the experience of accidentally removing the minter role using a generated dapp interface and renounce was the default function. (oops :eyes:).

1 Like

Let me propose a bit of a departure from Roles.sol. It might help.

A disadvantage of this approach is that it might be too rigid, and creating a more-flexible system (e.g. with multiple levels of administrative power) could require too much custom code.

I've used this in several projects, and is about a hundred lines of code:

pragma solidity ^0.5.10;


/**
 * @title RBAC
 * @author Alberto Cuesta Canada
 * @notice Implements runtime configurable Role Based Access Control.
 */
contract RBAC {
    event RoleCreated(bytes32 roleId);
    event RoleRemoved(bytes32 roleId);
    event MemberAdded(address member, bytes32 roleId);
    event MemberRemoved(address member, bytes32 roleId);

    bytes32 public constant ROOT_ROLE = "ROOT";

    /**
     * @notice A role, which will be used to group users.
     * @dev The role id is its position in the roles array.
     * @param admin The only role that can add or remove members from this role. To have the role
     * members to be also the role admins you should pass roles.length as the admin role.
     * @param members Addresses belonging to this role.
     */
    struct Role {
        bool exists;
        bytes32 adminRoleId;
        mapping (address => bool) members;
    }

    mapping (bytes32 => Role) internal roles;

    /**
     * @notice The contract initializer. It adds NO_ROLE as with role id 0x0, and ROOT_ROLE with role id 'ROOT'.
     */
    constructor(address _root) public {
        roles[ROOT_ROLE] = Role({ exists: true, adminRoleId: ROOT_ROLE });

        emit RoleCreated(ROOT_ROLE);
        roles[ROOT_ROLE].members[_root] = true;
        emit MemberAdded(_root, ROOT_ROLE);
    }

    /**
     * @notice A method to verify if a role exists.
     * @param _roleId The id of the role being verified.
     * @return True or false.
     * @dev roleExists of NO_ROLE returns false.
     */
    function roleExists(bytes32 _roleId)
        public
        view
        returns(bool)
    {
        return (roles[_roleId].exists);
    }

    /**
     * @notice A method to verify whether an member is a member of a role
     * @param _member The member to verify.
     * @param _roleId The role to look into.
     * @return Whether the member is a member of the role.
     */
    function hasRole(address _member, bytes32 _roleId)
        public
        view
        returns(bool)
    {
        require(roleExists(_roleId), "Role doesn't exist.");
        return roles[_roleId].members[_member];
    }

    /**
     * @notice A method to create a new role.
     * @param _roleId The id for role that is being created
     * @param _adminRoleId The role that is allowed to add and remove members from
     * the role being created.
     */
    function addRole(bytes32 _roleId, bytes32 _adminRoleId)
        public
    {
        // require(_roleId != NO_ROLE, "Reserved role id.");
        require(!roleExists(_roleId), "Role already exists.");
        require(roleExists(_adminRoleId), "Admin role doesn't exist.");
        require(hasRole(msg.sender, _adminRoleId), "Not admin of role.");

        roles[_roleId] = Role({ exists: true, adminRoleId: _adminRoleId });
        emit RoleCreated(_roleId);
    }

    /**
     * @notice A method to add a member to a role
     * @param _member The member to add as a member.
     * @param _roleId The role to add the member to.
     */
    function addMember(address _member, bytes32 _roleId)
        public
    {
        require(roleExists(_roleId), "Role doesn't exist.");
        require(
            hasRole(msg.sender, roles[_roleId].adminRoleId),
            "User can't add members."
        );
        require(
            !hasRole(_member, _roleId),
            "Address is member of role."
        );

        roles[_roleId].members[_member] = true;
        emit MemberAdded(_member, _roleId);
    }

    /**
     * @notice A method to remove a member from a role
     * @param _member The member to remove as a member.
     * @param _roleId The role to remove the member from.
     */
    function removeMember(address _member, bytes32 _roleId)
        public
    {
        require(roleExists(_roleId), "Role doesn't exist.");
        require(
            hasRole(msg.sender, roles[_roleId].adminRoleId),
            "User can't remove members."
        );
        require(
            hasRole(_member, _roleId),
            "Address is not member of role."
        );

        delete roles[_roleId].members[_member];
        emit MemberRemoved(_member, _roleId);
    }
}

I created it initially as a hierarchical role based access control system. The basic idea is that roles have an identifier, a list of members, and the identifier of another role which acts as an admin role.

If you belong to a role you can create other roles that you administer, and you can add and remove members to your roles.

The easiest way to use this contract to manage access control in another contract is by having something like this:

import "./RBAC.sol";


contract ManagedContract {
    bytes32 constant ROLE1 = "ROLE1";
    bytes32 constant ROLE2 = "ROLE2";
    bytes32 constant ROLE3 = "ROLE3";
    RBAC private _rbac;

    constructor (address rbac) {
        _rbac = RBAC(rbac);
        _rbac.addRole(ROLE1, _rbac.ROOT_ROLE);
        _rbac.addRole(ROLE2, _rbac.ROOT_ROLE);
        _rbac.addRole(ROLE3, _rbac.ROOT_ROLE);
    }

    modifier onlyRole(bytes32 role) {
        require(_rbac.hasRole(msg.sender, role), "Unauthorized.");
    }

    function needsRole1() public onlyRole(ROLE1) {
        // ...
    }

    function needsRole2() public onlyRole(ROLE2) {
        // ...
    }

    function needsRole3() public onlyRole(ROLE3) {
        // ...
    }

    function addMember(address member, bytes32 role) public {
        _rbac.addMember(member, role);
    }

    // ...
}

Upon instantiation of RBAC the deployer account always gets added to the root role. In this example that same account runs afterwards the constructor for ManagedContract, and then it creates three roles that it manages (it can do so because it belongs to the root role). That is enough to create some access control modifiers.

In this contract only the account that deployed the contract can add or remove members to the roles, but a multilayered role structure can be built on the constructor. It can also be built after the deployment with a bit more complexity.

    constructor (address rbac) {
        _rbac = RBAC(rbac);
        _rbac.addRole(ROLE1, _rbac.ROOT_ROLE);
        _rbac.addRole(ROLE2, ROLE1);
        _rbac.addRole(ROLE3, ROLE2);
    }

I think this approach could be used to implement role based access control more or less following the second option, but with a lot more flexibility with regards to how many roles a user needs, in which structure, and what for.

2 Likes

Yes, my original phrasing was a bit off. I don't intend for this the be the ultimate Access Control solution, but I'd like for it to be general and sensible enough that adding it to a project shouldn't require careful deliberation.

I like them for a variety of reasons:

  • They don't have the issues associated with strings
  • They don't have the issues associated with integers, namely, choosing how to associate a role name to an integer
  • Unlike bytes32 = "string", all 256 bits of the identifier are random-ish, increasing the Hamming distance between them
  • With a consistent convention, clashes are not possible (e.g. by always doing bytes32 constant X_ROLE = keccak256("X"))
  • They don't take up storage
  • They are reasonably cheap: calling a pure function on a contract that returns such a hash costs ~250 gas (this figure includes the overhead of decoding calldata, jumping based on the function selector, etc.)
  • They are easy to compute off-chain: web3.utils.soliditySha3("ROLE_NAME");

I'll add this to the original post.

I've considered alternatives for this, but wanted to focus the discussion on the other points discussed here first.

One option would be, as you suggest, removing transfer and having users do grant followed by renounce (after they've verified the grant was correct). An alternative would be to make grant and transfer follow Claimable's pattern of the recipient having to accept the role being granted.

We might get away with merging usage and grant, yes. I split them following the idea behind the Super Admin example, where those accounts cannot use the role so that we don't muddle the definition of hasRole.

I think iteration is important: you bring a valid point about log pruning, and there are situations where being able to list the roles is definitely worthwhile, such as being able to prove no accounts have a certain role, or that all holders of a role are DAOs/multisigs/etc.

Do you think it's necessary for the system to also list all roles in use? I'd argue roles are meaningless without inspecting the source code of the contract, and that analysis should yield the list of valid roles. You'd then be able to query all users that have each role.

Interesting, thanks! I don't think we've used the 'confirm before deletion' pattern in the past, but this might be a good place to add it.

Thanks for you suggestion! This an interesting evolution from the Super Admin scheme, where each role has its own admin (though many roles can have the same admin role). If we allowed for a role to be its own admin, we'd be also be able to implement the first pattern using this.

1 Like

This an interesting evolution from the Super Admin scheme, where each role has its own admin (though many roles can have the same admin role). If we allowed for a role to be its own admin, we’d be also be able to implement the first pattern using this.

I'm glad you like it. As it is, the root role is its own admin. All other roles follow the hierarchical structure.

So you can use this RBAC.sol pretty much the same way as you use Roles.sol, see an example of reimplementing ERC20Mintable.

pragma solidity ^0.5.0;

import "../../GSN/Context.sol";
import "../RBAC.sol";

contract MinterRBAC is Context, RBAC {
    event MinterAdded(address indexed account);
    event MinterRemoved(address indexed account);

    constructor () internal RBAC(_msgSender()) {
    }

    modifier onlyMinter() {
        require(isMinter(_msgSender()), "MinterRole: caller does not have the Minter role");
        _;
    }

    function isMinter(address account) public view returns (bool) {
        return hasRole(account, ROOT_ROLE);
    }

    function addMinter(address account) public onlyMinter {
        _addMinter(account);
    }

    function renounceMinter() public {
        _removeMinter(_msgSender());
    }

    function _addMinter(address account) internal {
        addMember(account, ROOT_ROLE);
        emit MinterAdded(account);
    }

    function _removeMinter(address account) internal {
        removeMember(account, ROOT_ROLE);
        emit MinterRemoved(account);
    }
}

Maybe all methods would have to be made internal in RBAC.sol, but that's about it.

1 Like

I think if the role list is hard coded, there is no need to be able to iterate over it. If the role list is something dynamic (e.g., some role has the ability to create new roles) then I think it is necessary to be able to iterate over the roles themselves. Since I'm guessing the library will give contracts the ability to have dynamic role lists, then I think this means that the library needs to make role lists enumerable.

3 Likes

I'm wary of allowing members of a role to add other members to the role.

Consider, for example, a project that has a "voters" role which allows its members to vote. And suppose that project makes decisions based on the outcome of a plurality vote. Then any member of the "voters" role could simply add sybil members to the "voters" role and use them to dominate the vote.

Another example: a project is using a price oracle that reports the median reported price of a set of trusted third-party oracles. They may implement an "oracles" role and allow members of that role to report the price of some asset. Later, the contract takes the median of those reported prices and uses that to make some decision. In this case, any member of the "oracles" role could add sybil members and then unilaterally control the final consumed (median) price.

Roles can be granted by other accounts with the role. This is deduced from (1): it is possible to transfer the role to a smart contract that can only be used by a set of accounts, effectively granting the role to all of them.

I think what this misses is that power is often measured by how many (or what percentage of) members of a given role agree on something. The ability to add sybil users is the ability to inflate one's own power in this kind of setting. Transferring my power to another address is one thing. Multiplying my total power by increasing the total number of members that I control is another.

So I like the idea of keeping the ability to add members to a role separate from the role itself.

I also like Micah's idea of being able to iterate over all members of a role, as it could aid in implementing the kind of "power measurements" mention above.

3 Likes

This is an extremely good insight, thank you very much @Austin-Williams! I think we never considered this because roles have not been used for those applications in the OpenZeppelin Contracts.

I am now convinced that transfer and grant are two different things, and while role bearers might be allowed to transfer their role, they should definitely not be able to grant it. This means we can discard outright the first proposal: we need a way for other accounts to grant roles (a Super Admin, the role's admin, or something else).

3 Likes

Allowing role bearers to transfer their roles is not something that every application will accept. In both examples from @Austin-Williams the same sybil attack can be done with transfer unless the application tracks the transfer operations, which already presumes a thorough understanding of the access control contract.

In another example, DeFi applications that aim to operate within the real economy need to comply with KYC and AML laws. Under those rules only properly identified individuals can use the offered financial services, and allowing transfer of permissions to unidentified accounts would certainly be forbidden. My best assumption is that DeFi applications would prefer to have a process to add additional accounts to an individual (grant) than to allow transfer.

As an analogy, you can give your credit card to someone and that someone could use it without problems, but the bank will tell you that you can’t do it, you could be punished if it is discovered, and certainly the bank will follow KYC processes if you ask for your card to be given to someone.

I think that an access control contract to be used widely would have to be flexible enough to allow all these possible implementations, as well as many we won’t think about.

Without going as far as the ACL implementation from Aragon (function hasPermission(address who, address where, bytes32 what, bytes how) public view returns (bool);), I think that the access control contract should contain a number of access control functions, but to be marked internal so that more specific implementations can be built out of that. Then I would focus in providing the right support and examples so that extending the access control contract is done right.

2 Likes

Hi all, chipping in here with some additions for the Roles library that may have affect Access Control design decisions. It has happened to me, and I foresee many others, would want to know how many accounts have a particular Role at a given moment. This could be accomplished with a public counter inside the _add and _remove functions of the library.

Moreover, with the advent of increased transparency and the Ethereum Name Service (i.e. clean account names instead of hexadecimal), applications may want to display all or some of the current bearers of a particular Role. An address[] public would do this. However, as a role grows in popularity it can get expensive to store all its addresses on-chain.

Thoughts?

2 Likes

I don’t think our roles solution should necessarily be built to satisfy those use cases, but this raises a very interesting point. I think so far we’ve been thinking about access control in a way that only includes authorization and not identification, so situations where it only matters that someone can do something and not who they are.

In that sense, voting and oracles are not purely about access control, because the notion of identity becomes relevant given that you need to differentiate who is voting or reporting a price.

Our reasoning that “everyone can grant” is a natural consequence of “everyone can transfer” only made sense in the context of pure authorization. However, since users of roles will have access to msg.sender anyway, we may want to design with identification in mind too. In that case, “everyone can grant” feels wrong.

On the other hand, if identification is important then transfer is also a potential issue. Voters or oracles could also transfer their role to a different account and that would likely result in a sybil attack as well. It’s only possible to prevent this if we use for identification an identifier other than the account address, that is preserved on a role transfer. Users still have ready access to msg.sender anyway so it is likely that they will use all of this incorrectly.

I think we have to reconsider the design with all of this in mind, but my conclusion is that roles should not be used for situations like voters or oracles in the way @Austin-Williams described. They’re different concepts that should be implemented on their own.

5 Likes

Not to confuse the conversation here, but for consideration:

Many applications separate the concepts of permissions and roles. This has the advantage to potentially simplify code maintenance while giving flexibility and improving ease-of-use for administrators.

Functions in this case check for permissions (essentially hard-coded), while roles are an arbitrary collection of permissions created and labeled by administrators. Then administrators assign roles to accounts (thereby assigning a collection of permissions to each account).

For example in a health care application, there might be separate functions with permissions to: (1) submit test results, (2) prescribe category 1 medicines, (3) prescribe category 2 medicines, (4) request an MRI, (5) approve an MRI, and (6) approve surgery.

In one health care facility they might have the following roles:
nurse: can do (1)(2)(4)
doctor: can do (1)(2)(3)(4)(5)

But in another health care facility they might have different roles:
junior nurse: can do (1)(2)(4)
senior nurse: can do (1)(2)(3)(4)
doctor: can do (1)(2)(3)(4)(5)

And in another they have even different roles and labels such as:
attending: can do (1)(2)(4)
resident: can do (1)(2)(3)(4)
surgeon: can do (1)(2)(3)(4)(5)

Another use-case mentioned in the comments is voting. Is an admin account allowed also to vote? Must an admin have a separate account for voting? In the permissions and roles model, there might be one permissions for “vote administration” and another for “voting”, then I can have a role for “admin” which has both permissions, and another role for “voter” which only has the voting permission.

Separating permissions and roles provides this flexibility. The implementation would need to include ability to create and define roles. Checking for a permission on an account would involve a lookup of the role for the account and then a lookup whether the role contains that permission.

In applications that have very limited number of roles and permissions (for example just super-admin, admin and general-user) then this flexibility and extra layer or implementation is not required. But for applications that will have increased functionality (more permissions) and more roles, especially those that might extend use to multiple groups that have different definition of roles (like example above), then this flexibility and extra layer becomes critical.

5 Likes

I think that is a great insight, @jonathan.

Following that model is easy to code a base RBAC contract that just records the Roles, and then segregate in different contracts the RBAC rules for a particular business and the Permissions for business functions.

Sample code

BaseRBAC:

pragma solidity ^0.5.10;

import "../GSN/Context.sol";


/**
 * @title BaseRoles
 * @author Alberto Cuesta Canada
 * @notice Implements runtime configurable Role Based Access Control.
 */
contract BaseRoles is Context {
    event RoleAdded(bytes32 roleId);
    event RoleRemoved(bytes32 roleId);
    event BearerAdded(address bearer, bytes32 roleId);
    event BearerRemoved(address bearer, bytes32 roleId);

    /**
     * @notice A role, which will be used to group users.
     * @dev The role id is its position in the roles array.
     * @param admin The only role that can add or remove bearers from this role. To have the role
     * bearers to be also the role admins you should pass roles.length as the admin role.
     * @param bearers Addresses belonging to this role.
     */
    struct Role {
        bool exists;
        mapping (address => bool) bearers;
    }

    mapping (bytes32 => Role) internal roles;

    /**
     * @notice A method to verify if a role exists.
     * @param roleId The id of the role being verified.
     * @return True or false.
     */
    function roleExists(bytes32 roleId)
        public
        view
        returns(bool)
    {
        return (roles[roleId].exists);
    }

    /**
     * @notice A method to verify whether an bearer is a bearer of a role
     * @param bearer The bearer to verify.
     * @param roleId The role to look into.
     * @return Whether the bearer is a bearer of the role.
     */
    function hasRole(address bearer, bytes32 roleId)
        public
        view
        returns(bool)
    {
        require(roleExists(roleId), "Role doesn't exist.");
        return roles[roleId].bearers[bearer];
    }

    /**
     * @notice A method to verify whether msg.sender is a bearer of a role
     * @param roleId The role to look into.
     * @return Whether the bearer is a bearer of the role.
     */
    function hasRole(bytes32 roleId)
        public
        view
        returns(bool)
    {
        return hasRole(_msgSender(), roleId);
    }


    /**
     * @notice A method to create a new role.
     * @param roleId The id for role that is being created
     * the role being created.
     */
    function _addRole(bytes32 roleId)
        internal
    {
        require(!roleExists(roleId), "Role already exists.");

        roles[roleId] = Role({ exists: true });
        emit RoleAdded(roleId);
    }

    /**
     * @notice A method to create a new role.
     * @param roleId The id for role that is being created
     * the role being created.
     */
    function _removeRole(bytes32 roleId)
        internal
    {
        require(roleExists(roleId), "Role doesn't exist.");

        delete roles[roleId];
        emit RoleRemoved(roleId);
    }


    /**
     * @notice A method to add a bearer to a role
     * @param bearer The address to add as a bearer.
     * @param roleId The role to add the bearer to.
     */
    function _addBearer(address bearer, bytes32 roleId)
        internal
    {
        require(
            !hasRole(bearer, roleId),
            "Address is bearer of role."
        );

        roles[roleId].bearers[bearer] = true;
        emit BearerAdded(bearer, roleId);
    }

    /**
     * @notice A method to remove a bearer from a role
     * @param bearer The address to remove as a bearer.
     * @param roleId The role to remove the bearer from.
     */
    function _removeBearer(address bearer, bytes32 roleId)
        internal
    {
        require(
            hasRole(bearer, roleId),
            "Address is not bearer of role."
        );

        delete roles[roleId].bearers[bearer];
        emit BearerRemoved(bearer, roleId);
    }
}

Then the specific business decides the RBAC rules, whether then need transfer, grant, superadmins, even dynamic roles can be created. For your scenarios something like this would do.

HealthRBAC:

pragma solidity ^0.5.10;

import "./BaseRoles.sol";


/**
 * @title HealthRBAC
 * @author Alberto Cuesta Canada
 * @notice Implements RBAC rules for an example Health business.
 */
contract HealthRBAC is BaseRoles {
    bytes32 constant ADMIN = "ADMIN";
    bytes32 constant NURSE = "NURSE";
    bytes32 constant DOCTOR = "DOCTOR";

    // Define initial roles and permissions
    constructor(address admin) public {
        _addRole(ADMIN);
        _addRole(NURSE);
        _addRole(DOCTOR);
        _addBearer(admin, ADMIN);
    }

    // Define RBAC rules
    function addBearer(address bearer, bytes32 roleId)
        public
    {
        require(hasRole(ADMIN), "Unauthorized");
        _addBearer(bearer, roleId);
    }

    function removeBearer(address bearer, bytes32 roleId)
        public
    {
        require(hasRole(ADMIN), "Unauthorized");
        _removeBearer(bearer, roleId);
    }
}

And then finally on that base we can code the actual business rules.

HealthBusiness:

pragma solidity ^0.5.10;

import "./HealthRBAC.sol";


/**
 * @title HealthBusiness
 * @author Alberto Cuesta Canada
 * @notice Implements a business using HealthRBAC.
 */
contract HealthBusiness is HealthRBAC {
    constructor(address admin) public HealthRBAC(admin) {}

    // Define business
    function submitTestResults()
        public
    {
        require(hasRole(NURSE) || hasRole(DOCTOR), "Unauthorized");
        // ...
    }

    function prescribeCat1()
        public
    {
        require(hasRole(NURSE) || hasRole(DOCTOR), "Unauthorized");
        // ...
    }

    function prescribeCat2()
        public
    {
        require(hasRole(DOCTOR), "Unauthorized");
        // ...
    }

    function requestMRI()
        public
    {
        require(hasRole(NURSE) || hasRole(DOCTOR), "Unauthorized");
        // ...
    }

    function approveSurgery()
        public
    {
        require(hasRole(DOCTOR), "Unauthorized");
        // ...
    }
}

This pattern is very flexible, and I don’t think it is too complicated to use successfully.

1 Like

Let’s focus the discussion on the design constraints for now. We can worry about the implementation details later. I think it’s also valuable to discuss potential APIs but with some minimal examples to keep things focused.

1 Like

That’s a good point.
To expand on this:

The ability to transfer your role implies the ability to trustlessly sell your role (or trustlessly rent out your role’s privileges).

Perhaps the ability to transfer roles should not be allowed by default. Perhaps, by default, if you want to transfer your role then the admin would need to perform the transfer.

@nventuro, I think I’m struggling to reason about Roles without context.

Can you give an example of when someone might want to use our Roles library? What kinds of use cases are we designing for?

I’m thinking back through recent audits and struggling to think of any examples where there is more than one role (other than the use of Ownable, where there is only a single _owner).

1 Like

These are roles we've had in Contracts:

  • minter can arbitrarily mint new tokens
  • pauser can pause and unpause a contract (which functions are paused is up to the user)
  • a capper was able set a cap on how many tokens an account could purchase in a crowdsale
  • similarly, a whitelistee was able to participate in a crowdsale, and whitelisters granted the whitelistee role (this a departure from the previous examples, and is sort of tied to identity)

As you can see, most are associated with being able to perform administrative tasks. We decided to have multiple roles instead of a single owner to allow for more granular designs, you may for example only let a multisig pause the system, while a different contract might have minting permissions. There's some relevant discussion here.

I was originally designing with use cases similar to these in mind (ability to pause, upgrade, selfdestruct, change system settings, etc.), and had not really considered voting or oracles. Like @frangio, my intuition is that these two contexts might be different enough that a single design won't be able to cover both, but I'm not entirely sure we can't make it work yet.

2 Likes

Thank you. Those examples help.

Additionally, we will also handle role management: how they are are granted and revoked.

Perhaps this goal goes to far.

It seems, from our discussion above, that granting and revoking conditions are heavily situation-dependent. If our Library is too opinionated about how this is done, then it may not be flexible enough to accomplish the first goal:

appropriate for the security needs of most if not all projects.

Perhaps we could not have our Roles library decide this for the users, but instead provide examples of how to use the library for a few of the most common access/revoke patterns.

2 Likes