Role library now requires non-zero addresses. (#1303)

* The role library now requires non-zero addresses.

* Fixed SignatureBouncer checks with null address.

* change ternary operator for or operator

* adapt to new variable name convention

* Update Roles.sol

contracts/access/Roles.sol

@@ -13,26 +13,29 @@ library Roles {
   /**
    * @dev give an account access to this role
    */
-  function add(Role storage _role, address _account) internal {
-    _role.bearer[_account] = true;
+  function add(Role storage role, address account) internal {
+    require(account != address(0));
+    role.bearer[account] = true;
   }
 
   /**
    * @dev remove an account's access to this role
    */
-  function remove(Role storage _role, address _account) internal {
-    _role.bearer[_account] = false;
+  function remove(Role storage role, address account) internal {
+    require(account != address(0));
+    role.bearer[account] = false;
   }
 
   /**
    * @dev check if an account has this role
    * @return bool
    */
-  function has(Role storage _role, address _account)
+  function has(Role storage role, address account)
     internal
     view
     returns (bool)
   {
-    return _role.bearer[_account];
+    require(account != address(0));
+    return role.bearer[account];
   }
 }

contracts/drafts/SignatureBouncer.sol

@@ -132,6 +132,7 @@ contract SignatureBouncer is SignerRole {
     address signer = hash
       .toEthSignedMessageHash()
       .recover(signature);
-    return isSigner(signer);
+
+    return signer != address(0) && isSigner(signer);
   }
 }

test/access/Roles.test.js

+const { assertRevert } = require('../helpers/assertRevert');
+
 const RolesMock = artifacts.require('RolesMock');
 
 require('chai')
@@ -10,6 +12,10 @@ contract('Roles', function ([_, authorized, otherAuthorized, anyone]) {
     this.roles = await RolesMock.new();
   });
 
+  it('reverts when querying roles for the null account', async function () {
+    await assertRevert(this.roles.has(ZERO_ADDRESS));
+  });
+
   context('initially', function () {
     it('doesn\'t pre-assign roles', async function () {
       (await this.roles.has(authorized)).should.equal(false);
@@ -30,8 +36,8 @@ contract('Roles', function ([_, authorized, otherAuthorized, anyone]) {
         (await this.roles.has(authorized)).should.equal(true);
       });
 
-      it('doesn\'t revert when adding roles to the null account', async function () {
-        await this.roles.add(ZERO_ADDRESS);
+      it('reverts when adding roles to the null account', async function () {
+        await assertRevert(this.roles.add(ZERO_ADDRESS));
       });
     });
   });
@@ -53,8 +59,8 @@ contract('Roles', function ([_, authorized, otherAuthorized, anyone]) {
         await this.roles.remove(anyone);
       });
 
-      it('doesn\'t revert when removing roles from the null account', async function () {
-        await this.roles.remove(ZERO_ADDRESS);
+      it('reverts when removing roles from the null account', async function () {
+        await assertRevert(this.roles.remove(ZERO_ADDRESS));
       });
     });
   });

test/access/roles/PublicRole.behavior.js

@@ -19,6 +19,10 @@ function shouldBehaveLikePublicRole (authorized, otherAuthorized, [anyone], role
       (await this.contract[`is${rolename}`](anyone)).should.equal(false);
     });
 
+    it('reverts when querying roles for the null account', async function () {
+      await assertRevert(this.contract[`is${rolename}`](ZERO_ADDRESS));
+    });
+
     describe('access control', function () {
       context('from authorized account', function () {
         const from = authorized;
@@ -53,8 +57,8 @@ function shouldBehaveLikePublicRole (authorized, otherAuthorized, [anyone], role
         (await this.contract[`is${rolename}`](authorized)).should.equal(true);
       });
 
-      it('doesn\'t revert when adding role to the null account', async function () {
-        await this.contract[`add${rolename}`](ZERO_ADDRESS, { from: authorized });
+      it('reverts when adding role to the null account', async function () {
+        await assertRevert(this.contract[`add${rolename}`](ZERO_ADDRESS, { from: authorized }));
       });
     });
 
@@ -74,8 +78,8 @@ function shouldBehaveLikePublicRole (authorized, otherAuthorized, [anyone], role
         await this.contract[`remove${rolename}`](anyone);
       });
 
-      it('doesn\'t revert when removing role from the null account', async function () {
-        await this.contract[`remove${rolename}`](ZERO_ADDRESS);
+      it('reverts when removing role from the null account', async function () {
+        await assertRevert(this.contract[`remove${rolename}`](ZERO_ADDRESS));
       });
     });