Improve RBAC role type

[nventuro]

As of v1.11.0, we're using strings as roles in RBAC. This can be problematic, because strings are UTF-8 encoded in Solidity. Therefore, the following is valid Solidity code, and looks correct at a first glance:

contract Campaign is RBAC {
  function () external payable {
    require(msg.value > 1 ether);
    addRole(msg.sender, "backer");
  }

  function doBackerStuff() public onlyRole("backer​") {
    ...
  }
}

doBackerStuff's modifier, however, does not contain the original string: an invisible ZERO WIDTH SPACE (U+200B) character has been added at the end, causing seemingly correct code to contain an (intentional?) bug.

A suggested alternative is to use integers as role identifiers, which removes this issue entirely. Assigning new identifiers to a role becomes a highly arbitrary task however, since numbers don't convey any role information, as opposed to strings like 'admin', 'curator', 'minter', etc. This could be sidestepped by using the hash of the role string as the integer identifier, e.g., uint256 public constant adminRole = 0x01ffc9a7...; // 0x01ffc9a... == keccak256('admin'), at the cost of placing the extra burden of having to use this pattern on the library users.

[shrugs]

Personally a fan of the bytes4(keccak) approach, since it gives you both globally(ish) unique identifiers (no collisions) and also visually distinct identifiers. Doing the bytes4(keccak) process is annoying, though, agreed.

[nventuro]

I wonder how hard it'd be to make a collision in 32 bits (i.e. bytes4(keccak('role1234')) matching bytes4(keccak('admin'))); we might want to increase it to bytes8 (or higher?).

[shrugs]

ah true, let's just future-proof it with bytes8

with bytes4, there are about 65,000 unique ids (sqrt(2^(8 * 4)))
with bytes8, there are about 4,294,967,296 unique ids (sqrt(2^(8 * 8)))

[maraoz]

amazing @nventuro !

[nventuro]

(see #1146 for some relevant discussion first, if you haven't)

So, I've tried out a couple of approaches, and am not entirely sure on which one we should pick moving forward. I've split this into multiple posts to make reading it easier.

[nventuro]

@shrugs proposed keeping the current method, but replacing the string with a hash:

contract MintableToken {
  bytes32 public constant ROLE_MINTER = 0xf1b18b0abd8edbdba00557edd963301d3be0cba30d024e258df9c747cbbf5e42;

  modifier hasMintPermission() {
    checkRole(msg.sender, ROLE_MINTER);
    _;
  }

  function addMinter(address _minter) public onlyAdmin {
    addRole(_minter, ROLE_MINTER);
  }

  function removeMinter(address _minter) public onlyAdmin {
    removeRole(_minter, ROLE_MINTER);
  }
}

The main benefits of this approach stem from the ability to refer to the role with an identifier, allowing to not declare the hasMintPermission modifier, since we can just use hasRole(ROLE_MINTER).

It also makes role assignment easier, e.g. with an admin role:

function addGenericRole(address _account, bytes32 _role) public onlyAdmin {
  addRole(_account, _role); // role can be ROLE_MINTER, ROLE_BURNER, etc
}

Finally, role assignment during construction would also be somewhat straightforward:

constructor(bytes32[] _roles, address[] _accounts) public {
  require(_roles.length == _accounts.length);
      
  for (uint256 i = 0; i < _roles.length; ++i) {
    addRole(_roles[i], _accounts[i])
  }
}

And once the experimental ABI encoder hits,

struct Designator {
  bytes32 roleId;
  address[] accounts;
}

constructor(Designator[] _designators) public {
  for (uint256 i = 0; i < _designators.length; ++i) {
    for (uint256 j = 0; j < _designators[i].accounts.length; ++j) {
      addRole(_designators[i].roleId, _designators[i].accounts[j]);
    }
  }
}

A couple issues, though:

• The ids, though constant, are not part of the contract type (i.e. you can't do MintableToken.MINTER_ROLE): you need to (afaik) either a) have a deployed contract and call the getter on it, or b) manually copy the id.
• It is possible to assign addresses to roles that do nothing. In other words, every single possible role exists and is valid.
• Since everything is generic, adding validation rules for certain roles (e.g. only allow one admin, admin must be a deployed contract, etc.) becomes a bit cumbersome.

Note that all three of these issues are also present in the RBAC contract.

[nventuro]

@frangio, on the other hand, suggested dropping RBAC and using the Roles library directly:

contract MintableToken {
  using Roles for Roles.Role;

  Roles.Role private minters;

  modifier hasMintPermission() {
    minters.check(msg.sender);
    _;
  }

  function addMinter(address _minter) internal {
    minters.add(_minter);
  }

  function removeMinter(address _minter) internal {
    minters.remove(_minter);
  }
}

Because there's no way to refer to each role (except by their storage slot - more on that later), we'd need to provide specific functions for each role, e.g. addMinter, addBurner, addAdmin, etc., leading to repeated non-reusable code. Role initialization for contracts will multiple roles wouldn't be that bad though, save for the fact that we'd need to provide a distinct array argument for each role:

library Roles {
  function addMany(Role storage _role, address[] _accounts) {
    for (uint256 i = 0; i < _accounts.length; ++i) {
      add(_role, _accounts[i]);
    }
  }
  ...
}

contract MintableToken {
  using Roles for Roles.Role;

  Roles.Role private minters;

  constructor(address[] _minters) {
    minters.addMany(_minters);
  }
}

// Same for BurnableToken

contract Derived is MintableToken, BurnableToken {
  constructor(address[] _minters, address[] _burners) 
    public
    MintableToken(_minters) 
    BurnableToken(_burners) 
  { }
}

This pattern also allows each base class to easily add custom validation rules in their constructor (e.g. only allow a certain number of address).

One of the issues with this is the need to have multiple arguments in the constructor, which stems from the fact that public and external functions cannot have arguments of storage location. However, if (once) Solidity exposes the storage layout, I believe it would be possible to pass the storage slot to a function, and have it cast it into a storage reference. While hacky, I like the underlying principle: the storage slot would in effect act as the id of the role, and would be both provisioned automatically and guarantee no collisions. They'd be contract specific, but I don't think this is an issue: I don't see a case where the roles have any meaning outside of a single contract.

[nventuro]

I'm currently leaning towards @frangio's proposal, because it is able to achieve the same goals as RBAC while doing away with the whole id -> role mapping, and provides easy to implement custom validations. I don't think our contracts will have so many roles for the associated issues to become a problem, and if they eventually do, we could at that point take another stab at this.

[frangio]

Great write up @nventuro! I really liked the second proposal. (I wouldn't say it's mine, though.)

Would it be possible to run into a problem with having too many arrays in the constructor? There are some limits imposed by the Solidity compiler, right?

[nventuro]

Yes - though in Remix I managed to compile a constructor receiving 15 arguments of type address[], which I think leaves us with plenty of headroom for now.

[frangio]

Interesting. It doesn't depend on the length of the arrays, does it?

[nventuro]

No, it depends on the types of the arguments.