Fix/whitelisted crowdsale (#981)

* fix: swithc WhitelistedCrowdsale to use Whitelist.sol

* feat: refactor whitelist.sol, rbac.sol and whitelistedcrowdsale.sol

* feat: add event arg assets and update whitelist

* fix: update modifier comment and also test isWhitelisted

* fix: remove onlyWhitelisted backwards compat attempt, fix explicit inheritance

* fix: remove underscore prefix from event args

* fix: user access/Whitelist

contracts/access/Whitelist.sol

@@ -11,84 +11,80 @@ import "../ownership/rbac/RBAC.sol";
  * This simplifies the implementation of "user permissions".
  */
 contract Whitelist is Ownable, RBAC {
-  event WhitelistedAddressAdded(address addr);
-  event WhitelistedAddressRemoved(address addr);
-
   string public constant ROLE_WHITELISTED = "whitelist";
 
   /**
-   * @dev Throws if called by any account that's not whitelisted.
+   * @dev Throws if operator is not whitelisted.
+   * @param _operator address
    */
-  modifier onlyWhitelisted() {
-    checkRole(msg.sender, ROLE_WHITELISTED);
+  modifier onlyIfWhitelisted(address _operator) {
+    checkRole(_operator, ROLE_WHITELISTED);
     _;
   }
 
   /**
    * @dev add an address to the whitelist
-   * @param addr address
+   * @param _operator address
    * @return true if the address was added to the whitelist, false if the address was already in the whitelist
    */
-  function addAddressToWhitelist(address addr)
+  function addAddressToWhitelist(address _operator)
     onlyOwner
     public
   {
-    addRole(addr, ROLE_WHITELISTED);
-    emit WhitelistedAddressAdded(addr);
+    addRole(_operator, ROLE_WHITELISTED);
   }
 
   /**
    * @dev getter to determine if address is in whitelist
    */
-  function whitelist(address addr)
+  function whitelist(address _operator)
     public
     view
     returns (bool)
   {
-    return hasRole(addr, ROLE_WHITELISTED);
+    return hasRole(_operator, ROLE_WHITELISTED);
   }
 
   /**
    * @dev add addresses to the whitelist
-   * @param addrs addresses
+   * @param _operators addresses
    * @return true if at least one address was added to the whitelist,
    * false if all addresses were already in the whitelist
    */
-  function addAddressesToWhitelist(address[] addrs)
+  function addAddressesToWhitelist(address[] _operators)
     onlyOwner
     public
   {
-    for (uint256 i = 0; i < addrs.length; i++) {
-      addAddressToWhitelist(addrs[i]);
+    for (uint256 i = 0; i < _operators.length; i++) {
+      addAddressToWhitelist(_operators[i]);
     }
   }
 
   /**
    * @dev remove an address from the whitelist
-   * @param addr address
+   * @param _operator address
    * @return true if the address was removed from the whitelist,
    * false if the address wasn't in the whitelist in the first place
    */
-  function removeAddressFromWhitelist(address addr)
+  function removeAddressFromWhitelist(address _operator)
     onlyOwner
     public
   {
-    removeRole(addr, ROLE_WHITELISTED);
-    emit WhitelistedAddressRemoved(addr);
+    removeRole(_operator, ROLE_WHITELISTED);
   }
 
   /**
    * @dev remove addresses from the whitelist
-   * @param addrs addresses
+   * @param _operators addresses
    * @return true if at least one address was removed from the whitelist,
    * false if all addresses weren't in the whitelist in the first place
    */
-  function removeAddressesFromWhitelist(address[] addrs)
+  function removeAddressesFromWhitelist(address[] _operators)
     onlyOwner
     public
   {
-    for (uint256 i = 0; i < addrs.length; i++) {
-      removeAddressFromWhitelist(addrs[i]);
+    for (uint256 i = 0; i < _operators.length; i++) {
+      removeAddressFromWhitelist(_operators[i]);
     }
   }
 

contracts/crowdsale/validation/WhitelistedCrowdsale.sol

 pragma solidity ^0.4.24;
 
 import "../Crowdsale.sol";
-import "../../ownership/Ownable.sol";
+import "../../access/Whitelist.sol";
 
 
 /**
  * @title WhitelistedCrowdsale
  * @dev Crowdsale in which only whitelisted users can contribute.
  */
-contract WhitelistedCrowdsale is Crowdsale, Ownable {
-
-  mapping(address => bool) public whitelist;
-
-  /**
-   * @dev Reverts if beneficiary is not whitelisted. Can be used when extending this contract.
-   */
-  modifier isWhitelisted(address _beneficiary) {
-    require(whitelist[_beneficiary]);
-    _;
-  }
-
-  /**
-   * @dev Adds single address to whitelist.
-   * @param _beneficiary Address to be added to the whitelist
-   */
-  function addToWhitelist(address _beneficiary) external onlyOwner {
-    whitelist[_beneficiary] = true;
-  }
-
-  /**
-   * @dev Adds list of addresses to whitelist. Not overloaded due to limitations with truffle testing.
-   * @param _beneficiaries Addresses to be added to the whitelist
-   */
-  function addManyToWhitelist(address[] _beneficiaries) external onlyOwner {
-    for (uint256 i = 0; i < _beneficiaries.length; i++) {
-      whitelist[_beneficiaries[i]] = true;
-    }
-  }
-
-  /**
-   * @dev Removes single address from whitelist.
-   * @param _beneficiary Address to be removed to the whitelist
-   */
-  function removeFromWhitelist(address _beneficiary) external onlyOwner {
-    whitelist[_beneficiary] = false;
-  }
-
+contract WhitelistedCrowdsale is Whitelist, Crowdsale {
   /**
    * @dev Extend parent behavior requiring beneficiary to be in whitelist.
    * @param _beneficiary Token beneficiary
@@ -55,8 +18,8 @@ contract WhitelistedCrowdsale is Crowdsale, Ownable {
     address _beneficiary,
     uint256 _weiAmount
   )
+    onlyIfWhitelisted(_beneficiary)
     internal
-    isWhitelisted(_beneficiary)
   {
     super._preValidatePurchase(_beneficiary, _weiAmount);
   }

contracts/mocks/WhitelistMock.sol

@@ -5,7 +5,7 @@ import "../access/Whitelist.sol";
 contract WhitelistMock is Whitelist {
 
   function onlyWhitelistedCanDoThis()
-    onlyWhitelisted
+    onlyIfWhitelisted(msg.sender)
     view
     external
   {

contracts/mocks/WhitelistedCrowdsaleImpl.sol

@@ -2,18 +2,18 @@ pragma solidity ^0.4.24;
 
 import "../token/ERC20/ERC20.sol";
 import "../crowdsale/validation/WhitelistedCrowdsale.sol";
+import "../crowdsale/Crowdsale.sol";
 
 
-contract WhitelistedCrowdsaleImpl is WhitelistedCrowdsale {
+contract WhitelistedCrowdsaleImpl is Crowdsale, WhitelistedCrowdsale {
 
   constructor (
     uint256 _rate,
     address _wallet,
     ERC20 _token
   )
-    public
     Crowdsale(_rate, _wallet, _token)
+    public
   {
   }
-
 }

contracts/ownership/rbac/RBAC.sol

@@ -19,83 +19,83 @@ contract RBAC {
 
   mapping (string => Roles.Role) private roles;
 
-  event RoleAdded(address addr, string roleName);
-  event RoleRemoved(address addr, string roleName);
+  event RoleAdded(address indexed operator, string role);
+  event RoleRemoved(address indexed operator, string role);
 
   /**
    * @dev reverts if addr does not have role
-   * @param addr address
-   * @param roleName the name of the role
+   * @param _operator address
+   * @param _role the name of the role
    * // reverts
    */
-  function checkRole(address addr, string roleName)
+  function checkRole(address _operator, string _role)
     view
     public
   {
-    roles[roleName].check(addr);
+    roles[_role].check(_operator);
   }
 
   /**
    * @dev determine if addr has role
-   * @param addr address
-   * @param roleName the name of the role
+   * @param _operator address
+   * @param _role the name of the role
    * @return bool
    */
-  function hasRole(address addr, string roleName)
+  function hasRole(address _operator, string _role)
     view
     public
     returns (bool)
   {
-    return roles[roleName].has(addr);
+    return roles[_role].has(_operator);
   }
 
   /**
    * @dev add a role to an address
-   * @param addr address
-   * @param roleName the name of the role
+   * @param _operator address
+   * @param _role the name of the role
    */
-  function addRole(address addr, string roleName)
+  function addRole(address _operator, string _role)
     internal
   {
-    roles[roleName].add(addr);
-    emit RoleAdded(addr, roleName);
+    roles[_role].add(_operator);
+    emit RoleAdded(_operator, _role);
   }
 
   /**
    * @dev remove a role from an address
-   * @param addr address
-   * @param roleName the name of the role
+   * @param _operator address
+   * @param _role the name of the role
    */
-  function removeRole(address addr, string roleName)
+  function removeRole(address _operator, string _role)
     internal
   {
-    roles[roleName].remove(addr);
-    emit RoleRemoved(addr, roleName);
+    roles[_role].remove(_operator);
+    emit RoleRemoved(_operator, _role);
   }
 
   /**
    * @dev modifier to scope access to a single role (uses msg.sender as addr)
-   * @param roleName the name of the role
+   * @param _role the name of the role
    * // reverts
    */
-  modifier onlyRole(string roleName)
+  modifier onlyRole(string _role)
   {
-    checkRole(msg.sender, roleName);
+    checkRole(msg.sender, _role);
     _;
   }
 
   /**
    * @dev modifier to scope access to a set of roles (uses msg.sender as addr)
-   * @param roleNames the names of the roles to scope access to
+   * @param _roles the names of the roles to scope access to
    * // reverts
    *
    * @TODO - when solidity supports dynamic arrays as arguments to modifiers, provide this
    *  see: https://github.com/ethereum/solidity/issues/2467
    */
-  // modifier onlyRoles(string[] roleNames) {
+  // modifier onlyRoles(string[] _roles) {
   //     bool hasAnyRole = false;
-  //     for (uint8 i = 0; i < roleNames.length; i++) {
-  //         if (hasRole(msg.sender, roleNames[i])) {
+  //     for (uint8 i = 0; i < _roles.length; i++) {
+  //         if (hasRole(msg.sender, _roles[i])) {
   //             hasAnyRole = true;
   //             break;
   //         }

test/crowdsale/WhitelistedCrowdsale.test.js

@@ -19,23 +19,24 @@ contract('WhitelistedCrowdsale', function ([_, wallet, authorized, unauthorized,
       this.token = await SimpleToken.new();
       this.crowdsale = await WhitelistedCrowdsale.new(rate, wallet, this.token.address);
       await this.token.transfer(this.crowdsale.address, tokenSupply);
-      await this.crowdsale.addToWhitelist(authorized);
+      await this.crowdsale.addAddressToWhitelist(authorized);
     });
 
     describe('accepting payments', function () {
       it('should accept payments to whitelisted (from whichever buyers)', async function () {
+        await this.crowdsale.sendTransaction({ value, from: authorized }).should.be.fulfilled;
         await this.crowdsale.buyTokens(authorized, { value: value, from: authorized }).should.be.fulfilled;
         await this.crowdsale.buyTokens(authorized, { value: value, from: unauthorized }).should.be.fulfilled;
       });
 
       it('should reject payments to not whitelisted (from whichever buyers)', async function () {
-        await this.crowdsale.send(value).should.be.rejected;
+        await this.crowdsale.sendTransaction({ value, from: unauthorized }).should.be.rejected;
         await this.crowdsale.buyTokens(unauthorized, { value: value, from: unauthorized }).should.be.rejected;
         await this.crowdsale.buyTokens(unauthorized, { value: value, from: authorized }).should.be.rejected;
       });
 
       it('should reject payments to addresses removed from whitelist', async function () {
-        await this.crowdsale.removeFromWhitelist(authorized);
+        await this.crowdsale.removeAddressFromWhitelist(authorized);
         await this.crowdsale.buyTokens(authorized, { value: value, from: authorized }).should.be.rejected;
       });
     });
@@ -55,7 +56,7 @@ contract('WhitelistedCrowdsale', function ([_, wallet, authorized, unauthorized,
       this.token = await SimpleToken.new();
       this.crowdsale = await WhitelistedCrowdsale.new(rate, wallet, this.token.address);
       await this.token.transfer(this.crowdsale.address, tokenSupply);
-      await this.crowdsale.addManyToWhitelist([authorized, anotherAuthorized]);
+      await this.crowdsale.addAddressesToWhitelist([authorized, anotherAuthorized]);
     });
 
     describe('accepting payments', function () {
@@ -73,7 +74,7 @@ contract('WhitelistedCrowdsale', function ([_, wallet, authorized, unauthorized,
       });
 
       it('should reject payments to addresses removed from whitelist', async function () {
-        await this.crowdsale.removeFromWhitelist(anotherAuthorized);
+        await this.crowdsale.removeAddressFromWhitelist(anotherAuthorized);
         await this.crowdsale.buyTokens(authorized, { value: value, from: authorized }).should.be.fulfilled;
         await this.crowdsale.buyTokens(anotherAuthorized, { value: value, from: authorized }).should.be.rejected;
       });

test/helpers/expectEvent.js

-const assert = require('chai').assert;
+const should = require('chai').should();
 
-const inLogs = async (logs, eventName) => {
+const inLogs = async (logs, eventName, eventArgs = {}) => {
   const event = logs.find(e => e.event === eventName);
-  assert.exists(event);
+  should.exist(event);
+  for (const [k, v] of Object.entries(eventArgs)) {
+    should.exist(event.args[k]);
+    event.args[k].should.eq(v);
+  }
   return event;
 };
 
-const inTransaction = async (tx, eventName) => {
+const inTransaction = async (tx, eventName, eventArgs = {}) => {
   const { logs } = await tx;
-  return inLogs(logs, eventName);
+  return inLogs(logs, eventName, eventArgs);
 };
 
 module.exports = {

test/ownership/Whitelist.test.js

@@ -8,8 +8,6 @@ require('chai')
   .should();
 
 contract('Whitelist', function (accounts) {
-  let mock;
-
   const [
     owner,
     whitelistedAddress1,
@@ -20,73 +18,78 @@ contract('Whitelist', function (accounts) {
   const whitelistedAddresses = [whitelistedAddress1, whitelistedAddress2];
 
   before(async function () {
-    mock = await WhitelistMock.new();
+    this.mock = await WhitelistMock.new();
+    this.role = await this.mock.ROLE_WHITELISTED();
   });
 
-  context('in normal conditions', () => {
+  context('in normal conditions', function () {
     it('should add address to the whitelist', async function () {
       await expectEvent.inTransaction(
-        mock.addAddressToWhitelist(whitelistedAddress1, { from: owner }),
-        'WhitelistedAddressAdded'
+        this.mock.addAddressToWhitelist(whitelistedAddress1, { from: owner }),
+        'RoleAdded',
+        { role: this.role },
       );
-      const isWhitelisted = await mock.whitelist(whitelistedAddress1);
+      const isWhitelisted = await this.mock.whitelist(whitelistedAddress1);
       isWhitelisted.should.be.equal(true);
     });
 
     it('should add addresses to the whitelist', async function () {
       await expectEvent.inTransaction(
-        mock.addAddressesToWhitelist(whitelistedAddresses, { from: owner }),
-        'WhitelistedAddressAdded'
+        this.mock.addAddressesToWhitelist(whitelistedAddresses, { from: owner }),
+        'RoleAdded',
+        { role: this.role },
       );
       for (let addr of whitelistedAddresses) {
-        const isWhitelisted = await mock.whitelist(addr);
+        const isWhitelisted = await this.mock.whitelist(addr);
         isWhitelisted.should.be.equal(true);
       }
     });
 
     it('should remove address from the whitelist', async function () {
       await expectEvent.inTransaction(
-        mock.removeAddressFromWhitelist(whitelistedAddress1, { from: owner }),
-        'WhitelistedAddressRemoved'
+        this.mock.removeAddressFromWhitelist(whitelistedAddress1, { from: owner }),
+        'RoleRemoved',
+        { role: this.role },
       );
-      let isWhitelisted = await mock.whitelist(whitelistedAddress1);
+      let isWhitelisted = await this.mock.whitelist(whitelistedAddress1);
       isWhitelisted.should.be.equal(false);
     });
 
     it('should remove addresses from the the whitelist', async function () {
       await expectEvent.inTransaction(
-        mock.removeAddressesFromWhitelist(whitelistedAddresses, { from: owner }),
-        'WhitelistedAddressRemoved'
+        this.mock.removeAddressesFromWhitelist(whitelistedAddresses, { from: owner }),
+        'RoleRemoved',
+        { role: this.role },
       );
       for (let addr of whitelistedAddresses) {
-        const isWhitelisted = await mock.whitelist(addr);
+        const isWhitelisted = await this.mock.whitelist(addr);
         isWhitelisted.should.be.equal(false);
       }
     });
 
-    it('should allow whitelisted address to call #onlyWhitelistedCanDoThis', async () => {
-      await mock.addAddressToWhitelist(whitelistedAddress1, { from: owner });
-      await mock.onlyWhitelistedCanDoThis({ from: whitelistedAddress1 })
+    it('should allow whitelisted address to call #onlyWhitelistedCanDoThis', async function () {
+      await this.mock.addAddressToWhitelist(whitelistedAddress1, { from: owner });
+      await this.mock.onlyWhitelistedCanDoThis({ from: whitelistedAddress1 })
         .should.be.fulfilled;
     });
   });
 
-  context('in adversarial conditions', () => {
-    it('should not allow "anyone" to add to the whitelist', async () => {
+  context('in adversarial conditions', function () {
+    it('should not allow "anyone" to add to the whitelist', async function () {
       await expectThrow(
-        mock.addAddressToWhitelist(whitelistedAddress1, { from: anyone })
+        this.mock.addAddressToWhitelist(whitelistedAddress1, { from: anyone })
       );
     });
 
-    it('should not allow "anyone" to remove from the whitelist', async () => {
+    it('should not allow "anyone" to remove from the whitelist', async function () {
       await expectThrow(
-        mock.removeAddressFromWhitelist(whitelistedAddress1, { from: anyone })
+        this.mock.removeAddressFromWhitelist(whitelistedAddress1, { from: anyone })
       );
     });
 
-    it('should not allow "anyone" to call #onlyWhitelistedCanDoThis', async () => {
+    it('should not allow "anyone" to call #onlyWhitelistedCanDoThis', async function () {
       await expectThrow(
-        mock.onlyWhitelistedCanDoThis({ from: anyone })
+        this.mock.onlyWhitelistedCanDoThis({ from: anyone })
       );
     });
   });