From 92b695f2fb258a5669181d1d8cf8e73aa756f22c Mon Sep 17 00:00:00 2001 From: Matt Condon Date: Fri, 15 Jun 2018 14:11:50 -0700 Subject: [PATCH] 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 | 44 +++++++-------- .../validation/WhitelistedCrowdsale.sol | 43 +-------------- contracts/mocks/WhitelistMock.sol | 2 +- contracts/mocks/WhitelistedCrowdsaleImpl.sol | 6 +- contracts/ownership/rbac/RBAC.sol | 54 +++++++++--------- test/crowdsale/WhitelistedCrowdsale.test.js | 11 ++-- test/helpers/expectEvent.js | 14 +++-- test/ownership/Whitelist.test.js | 55 ++++++++++--------- 8 files changed, 98 insertions(+), 131 deletions(-) diff --git a/contracts/access/Whitelist.sol b/contracts/access/Whitelist.sol index 312d5f075..7f9c60f7a 100644 --- a/contracts/access/Whitelist.sol +++ b/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]); } } diff --git a/contracts/crowdsale/validation/WhitelistedCrowdsale.sol b/contracts/crowdsale/validation/WhitelistedCrowdsale.sol index c5bddf951..37063382c 100644 --- a/contracts/crowdsale/validation/WhitelistedCrowdsale.sol +++ b/contracts/crowdsale/validation/WhitelistedCrowdsale.sol @@ -1,51 +1,14 @@ 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); } diff --git a/contracts/mocks/WhitelistMock.sol b/contracts/mocks/WhitelistMock.sol index 899816f99..d6c2eef8e 100644 --- a/contracts/mocks/WhitelistMock.sol +++ b/contracts/mocks/WhitelistMock.sol @@ -5,7 +5,7 @@ import "../access/Whitelist.sol"; contract WhitelistMock is Whitelist { function onlyWhitelistedCanDoThis() - onlyWhitelisted + onlyIfWhitelisted(msg.sender) view external { diff --git a/contracts/mocks/WhitelistedCrowdsaleImpl.sol b/contracts/mocks/WhitelistedCrowdsaleImpl.sol index 2d5a5eea5..4cfd739ec 100644 --- a/contracts/mocks/WhitelistedCrowdsaleImpl.sol +++ b/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 { } - } diff --git a/contracts/ownership/rbac/RBAC.sol b/contracts/ownership/rbac/RBAC.sol index b749d1faa..89c85efb0 100644 --- a/contracts/ownership/rbac/RBAC.sol +++ b/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; // } diff --git a/test/crowdsale/WhitelistedCrowdsale.test.js b/test/crowdsale/WhitelistedCrowdsale.test.js index e62016ace..0097b046d 100644 --- a/test/crowdsale/WhitelistedCrowdsale.test.js +++ b/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; }); diff --git a/test/helpers/expectEvent.js b/test/helpers/expectEvent.js index 72f91b7b9..ab1d6751b 100644 --- a/test/helpers/expectEvent.js +++ b/test/helpers/expectEvent.js @@ -1,14 +1,18 @@ -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 = { diff --git a/test/ownership/Whitelist.test.js b/test/ownership/Whitelist.test.js index 81ae6733c..15e083c55 100644 --- a/test/ownership/Whitelist.test.js +++ b/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 }) ); }); });