diff --git a/contracts/access/Roles.sol b/contracts/access/Roles.sol index f5523a3e1..b0212c2be 100644 --- a/contracts/access/Roles.sol +++ b/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]; } } diff --git a/contracts/drafts/SignatureBouncer.sol b/contracts/drafts/SignatureBouncer.sol index d583c7399..a584b7aad 100644 --- a/contracts/drafts/SignatureBouncer.sol +++ b/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); } } diff --git a/test/access/Roles.test.js b/test/access/Roles.test.js index be13be072..7d5573cec 100644 --- a/test/access/Roles.test.js +++ b/test/access/Roles.test.js @@ -1,3 +1,5 @@ +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)); }); }); }); diff --git a/test/access/roles/PublicRole.behavior.js b/test/access/roles/PublicRole.behavior.js index 34cdbe5dd..1e489a468 100644 --- a/test/access/roles/PublicRole.behavior.js +++ b/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)); }); });