From e53296c93b4d762dde60345332b32ea0077a4573 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Thu, 6 Sep 2018 15:53:34 -0300 Subject: [PATCH 1/5] Added CapperRole. --- contracts/access/rbac/CapperRole.sol | 35 +++++ .../IndividuallyCappedCrowdsale.sol | 8 +- contracts/mocks/CapperRoleMock.sol | 18 +++ .../mocks/IndividuallyCappedCrowdsaleImpl.sol | 3 +- test/access/rbac/CapperRole.test.js | 11 ++ .../IndividuallyCappedCrowdsale.test.js | 148 +++++++++++------- 6 files changed, 161 insertions(+), 62 deletions(-) create mode 100644 contracts/access/rbac/CapperRole.sol create mode 100644 contracts/mocks/CapperRoleMock.sol create mode 100644 test/access/rbac/CapperRole.test.js diff --git a/contracts/access/rbac/CapperRole.sol b/contracts/access/rbac/CapperRole.sol new file mode 100644 index 00000000000..d73ebc97a4a --- /dev/null +++ b/contracts/access/rbac/CapperRole.sol @@ -0,0 +1,35 @@ +pragma solidity ^0.4.24; + +import "./Roles.sol"; + + +contract CapperRole { + using Roles for Roles.Role; + + Roles.Role private cappers; + + constructor() public { + cappers.add(msg.sender); + } + + modifier onlyCapper() { + require(isCapper(msg.sender)); + _; + } + + function isCapper(address _account) public view returns (bool) { + return cappers.has(_account); + } + + function addCapper(address _account) public onlyCapper { + cappers.add(_account); + } + + function renounceCapper() public { + cappers.remove(msg.sender); + } + + function _removeCapper(address _account) internal { + cappers.remove(_account); + } +} diff --git a/contracts/crowdsale/validation/IndividuallyCappedCrowdsale.sol b/contracts/crowdsale/validation/IndividuallyCappedCrowdsale.sol index f0c3fdd4c43..e3375d565da 100644 --- a/contracts/crowdsale/validation/IndividuallyCappedCrowdsale.sol +++ b/contracts/crowdsale/validation/IndividuallyCappedCrowdsale.sol @@ -2,14 +2,14 @@ pragma solidity ^0.4.24; import "../../math/SafeMath.sol"; import "../Crowdsale.sol"; -import "../../ownership/Ownable.sol"; +import "../../access/rbac/CapperRole.sol"; /** * @title IndividuallyCappedCrowdsale * @dev Crowdsale with per-user caps. */ -contract IndividuallyCappedCrowdsale is Ownable, Crowdsale { +contract IndividuallyCappedCrowdsale is Crowdsale, CapperRole { using SafeMath for uint256; mapping(address => uint256) private contributions_; @@ -20,7 +20,7 @@ contract IndividuallyCappedCrowdsale is Ownable, Crowdsale { * @param _beneficiary Address to be capped * @param _cap Wei limit for individual contribution */ - function setUserCap(address _beneficiary, uint256 _cap) external onlyOwner { + function setUserCap(address _beneficiary, uint256 _cap) external onlyCapper { caps_[_beneficiary] = _cap; } @@ -34,7 +34,7 @@ contract IndividuallyCappedCrowdsale is Ownable, Crowdsale { uint256 _cap ) external - onlyOwner + onlyCapper { for (uint256 i = 0; i < _beneficiaries.length; i++) { caps_[_beneficiaries[i]] = _cap; diff --git a/contracts/mocks/CapperRoleMock.sol b/contracts/mocks/CapperRoleMock.sol new file mode 100644 index 00000000000..08d2aff7b0a --- /dev/null +++ b/contracts/mocks/CapperRoleMock.sol @@ -0,0 +1,18 @@ +pragma solidity ^0.4.24; + +import "../access/rbac/CapperRole.sol"; + + +contract CapperRoleMock is CapperRole { + function removeCapper(address _account) public { + _removeCapper(_account); + } + + function onlyCapperMock() public view onlyCapper { + } + + // Causes a compilation error if super._removeCapper is not internal + function _removeCapper(address _account) internal { + super._removeCapper(_account); + } +} diff --git a/contracts/mocks/IndividuallyCappedCrowdsaleImpl.sol b/contracts/mocks/IndividuallyCappedCrowdsaleImpl.sol index b4b470f7ca6..696677bb7fa 100644 --- a/contracts/mocks/IndividuallyCappedCrowdsaleImpl.sol +++ b/contracts/mocks/IndividuallyCappedCrowdsaleImpl.sol @@ -2,9 +2,10 @@ pragma solidity ^0.4.24; import "../token/ERC20/IERC20.sol"; import "../crowdsale/validation/IndividuallyCappedCrowdsale.sol"; +import "./CapperRoleMock.sol"; -contract IndividuallyCappedCrowdsaleImpl is IndividuallyCappedCrowdsale { +contract IndividuallyCappedCrowdsaleImpl is IndividuallyCappedCrowdsale, CapperRoleMock { constructor ( uint256 _rate, diff --git a/test/access/rbac/CapperRole.test.js b/test/access/rbac/CapperRole.test.js new file mode 100644 index 00000000000..117b25b03d9 --- /dev/null +++ b/test/access/rbac/CapperRole.test.js @@ -0,0 +1,11 @@ +const { shouldBehaveLikePublicRole } = require('../../access/rbac/PublicRole.behavior'); +const CapperRoleMock = artifacts.require('CapperRoleMock'); + +contract('CapperRole', function ([_, capper, otherCapper, ...otherAccounts]) { + beforeEach(async function () { + this.contract = await CapperRoleMock.new({ from: capper }); + await this.contract.addCapper(otherCapper, { from: capper }); + }); + + shouldBehaveLikePublicRole(capper, otherCapper, otherAccounts, 'capper'); +}); diff --git a/test/crowdsale/IndividuallyCappedCrowdsale.test.js b/test/crowdsale/IndividuallyCappedCrowdsale.test.js index 8deeca37700..bec1b3668dd 100644 --- a/test/crowdsale/IndividuallyCappedCrowdsale.test.js +++ b/test/crowdsale/IndividuallyCappedCrowdsale.test.js @@ -8,10 +8,11 @@ require('chai') .use(require('chai-bignumber')(BigNumber)) .should(); -const CappedCrowdsale = artifacts.require('IndividuallyCappedCrowdsaleImpl'); +const IndividuallyCappedCrowdsaleImpl = artifacts.require('IndividuallyCappedCrowdsaleImpl'); const SimpleToken = artifacts.require('SimpleToken'); +const { shouldBehaveLikePublicRole } = require('../access/rbac/PublicRole.behavior'); -contract('IndividuallyCappedCrowdsale', function ([_, wallet, alice, bob, charlie]) { +contract('IndividuallyCappedCrowdsale', function ([_, capper, otherCapper, wallet, alice, bob, charlie, anyone, ...otherAccounts]) { const rate = new BigNumber(1); const capAlice = ether(10); const capBob = ether(2); @@ -19,84 +20,117 @@ contract('IndividuallyCappedCrowdsale', function ([_, wallet, alice, bob, charli const lessThanCapBoth = ether(1); const tokenSupply = new BigNumber('1e22'); - describe('individual capping', function () { + beforeEach(async function () { + this.token = await SimpleToken.new(); + this.crowdsale = await IndividuallyCappedCrowdsaleImpl.new(rate, wallet, this.token.address, { from: capper }); + }); + + describe('capper role', function () { beforeEach(async function () { - this.token = await SimpleToken.new(); - this.crowdsale = await CappedCrowdsale.new(rate, wallet, this.token.address); - await this.crowdsale.setUserCap(alice, capAlice); - await this.crowdsale.setUserCap(bob, capBob); - await this.token.transfer(this.crowdsale.address, tokenSupply); + this.contract = this.crowdsale; + await this.contract.addCapper(otherCapper, { from: capper }); }); - describe('accepting payments', function () { - it('should accept payments within cap', async function () { - await this.crowdsale.buyTokens(alice, { value: lessThanCapAlice }); - await this.crowdsale.buyTokens(bob, { value: lessThanCapBoth }); - }); + shouldBehaveLikePublicRole(capper, otherCapper, otherAccounts, 'capper'); + }); - it('should reject payments outside cap', async function () { - await this.crowdsale.buyTokens(alice, { value: capAlice }); - await expectThrow(this.crowdsale.buyTokens(alice, { value: 1 }), EVMRevert); - }); + describe('individual caps', function () { + it('sets a cap when the sender is a capper', async function () { + await this.crowdsale.setUserCap(alice, capAlice, { from: capper }); + (await this.crowdsale.getUserCap(alice)).should.be.bignumber.equal(capAlice); + }); - it('should reject payments that exceed cap', async function () { - await expectThrow(this.crowdsale.buyTokens(alice, { value: capAlice.plus(1) }), EVMRevert); - await expectThrow(this.crowdsale.buyTokens(bob, { value: capBob.plus(1) }), EVMRevert); - }); + it('reverts when a non-capper sets a cap', async function () { + await expectThrow(this.crowdsale.setUserCap(alice, capAlice, { from: anyone }), EVMRevert); + }); - it('should manage independent caps', async function () { - await this.crowdsale.buyTokens(alice, { value: lessThanCapAlice }); - await expectThrow(this.crowdsale.buyTokens(bob, { value: lessThanCapAlice }), EVMRevert); + context('with individual caps', function () { + beforeEach(async function () { + await this.crowdsale.setUserCap(alice, capAlice, { from: capper }); + await this.crowdsale.setUserCap(bob, capBob, { from: capper }); + await this.token.transfer(this.crowdsale.address, tokenSupply); }); - it('should default to a cap of zero', async function () { - await expectThrow(this.crowdsale.buyTokens(charlie, { value: lessThanCapBoth }), EVMRevert); + describe('accepting payments', function () { + it('should accept payments within cap', async function () { + await this.crowdsale.buyTokens(alice, { value: lessThanCapAlice }); + await this.crowdsale.buyTokens(bob, { value: lessThanCapBoth }); + }); + + it('should reject payments outside cap', async function () { + await this.crowdsale.buyTokens(alice, { value: capAlice }); + await expectThrow(this.crowdsale.buyTokens(alice, { value: 1 }), EVMRevert); + }); + + it('should reject payments that exceed cap', async function () { + await expectThrow(this.crowdsale.buyTokens(alice, { value: capAlice.plus(1) }), EVMRevert); + await expectThrow(this.crowdsale.buyTokens(bob, { value: capBob.plus(1) }), EVMRevert); + }); + + it('should manage independent caps', async function () { + await this.crowdsale.buyTokens(alice, { value: lessThanCapAlice }); + await expectThrow(this.crowdsale.buyTokens(bob, { value: lessThanCapAlice }), EVMRevert); + }); + + it('should default to a cap of zero', async function () { + await expectThrow(this.crowdsale.buyTokens(charlie, { value: lessThanCapBoth }), EVMRevert); + }); }); - }); - describe('reporting state', function () { - it('should report correct cap', async function () { - (await this.crowdsale.getUserCap(alice)).should.be.bignumber.equal(capAlice); - }); + describe('reporting state', function () { + it('should report correct cap', async function () { + (await this.crowdsale.getUserCap(alice)).should.be.bignumber.equal(capAlice); + }); - it('should report actual contribution', async function () { - await this.crowdsale.buyTokens(alice, { value: lessThanCapAlice }); - (await this.crowdsale.getUserContribution(alice)).should.be.bignumber.equal(lessThanCapAlice); + it('should report actual contribution', async function () { + await this.crowdsale.buyTokens(alice, { value: lessThanCapAlice }); + (await this.crowdsale.getUserContribution(alice)).should.be.bignumber.equal(lessThanCapAlice); + }); }); }); }); describe('group capping', function () { - beforeEach(async function () { - this.token = await SimpleToken.new(); - this.crowdsale = await CappedCrowdsale.new(rate, wallet, this.token.address); - await this.crowdsale.setGroupCap([bob, charlie], capBob); - await this.token.transfer(this.crowdsale.address, tokenSupply); + it('sets caps when the sender is a capper', async function () { + await this.crowdsale.setGroupCap([bob, charlie], capBob, { from: capper }); + (await this.crowdsale.getUserCap(bob)).should.be.bignumber.equal(capBob); + (await this.crowdsale.getUserCap(charlie)).should.be.bignumber.equal(capBob); }); - describe('accepting payments', function () { - it('should accept payments within cap', async function () { - await this.crowdsale.buyTokens(bob, { value: lessThanCapBoth }); - await this.crowdsale.buyTokens(charlie, { value: lessThanCapBoth }); - }); + it('reverts when a non-capper set caps', async function () { + await expectThrow(this.crowdsale.setGroupCap([bob, charlie], capBob, { from: anyone }), EVMRevert); + }); - it('should reject payments outside cap', async function () { - await this.crowdsale.buyTokens(bob, { value: capBob }); - await expectThrow(this.crowdsale.buyTokens(bob, { value: 1 }), EVMRevert); - await this.crowdsale.buyTokens(charlie, { value: capBob }); - await expectThrow(this.crowdsale.buyTokens(charlie, { value: 1 }), EVMRevert); + context('with group caps', function () { + beforeEach(async function () { + await this.crowdsale.setGroupCap([bob, charlie], capBob, { from: capper }); + await this.token.transfer(this.crowdsale.address, tokenSupply); }); - it('should reject payments that exceed cap', async function () { - await expectThrow(this.crowdsale.buyTokens(bob, { value: capBob.plus(1) }), EVMRevert); - await expectThrow(this.crowdsale.buyTokens(charlie, { value: capBob.plus(1) }), EVMRevert); + describe('accepting payments', function () { + it('should accept payments within cap', async function () { + await this.crowdsale.buyTokens(bob, { value: lessThanCapBoth }); + await this.crowdsale.buyTokens(charlie, { value: lessThanCapBoth }); + }); + + it('should reject payments outside cap', async function () { + await this.crowdsale.buyTokens(bob, { value: capBob }); + await expectThrow(this.crowdsale.buyTokens(bob, { value: 1 }), EVMRevert); + await this.crowdsale.buyTokens(charlie, { value: capBob }); + await expectThrow(this.crowdsale.buyTokens(charlie, { value: 1 }), EVMRevert); + }); + + it('should reject payments that exceed cap', async function () { + await expectThrow(this.crowdsale.buyTokens(bob, { value: capBob.plus(1) }), EVMRevert); + await expectThrow(this.crowdsale.buyTokens(charlie, { value: capBob.plus(1) }), EVMRevert); + }); }); - }); - describe('reporting state', function () { - it('should report correct cap', async function () { - (await this.crowdsale.getUserCap(bob)).should.be.bignumber.equal(capBob); - (await this.crowdsale.getUserCap(charlie)).should.be.bignumber.equal(capBob); + describe('reporting state', function () { + it('should report correct cap', async function () { + (await this.crowdsale.getUserCap(bob)).should.be.bignumber.equal(capBob); + (await this.crowdsale.getUserCap(charlie)).should.be.bignumber.equal(capBob); + }); }); }); }); From 14e9db0b5c5324592c508be64df2d1666a464420 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Thu, 6 Sep 2018 16:07:01 -0300 Subject: [PATCH 2/5] RefundEscrow is now Secondary. --- contracts/payment/RefundEscrow.sol | 14 +++++----- test/payment/RefundEscrow.test.js | 42 +++++++++++++++--------------- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/contracts/payment/RefundEscrow.sol b/contracts/payment/RefundEscrow.sol index 802dd471128..fc46ee03a19 100644 --- a/contracts/payment/RefundEscrow.sol +++ b/contracts/payment/RefundEscrow.sol @@ -1,16 +1,16 @@ pragma solidity ^0.4.23; import "./ConditionalEscrow.sol"; -import "../ownership/Ownable.sol"; +import "../ownership/Secondary.sol"; /** * @title RefundEscrow * @dev Escrow that holds funds for a beneficiary, deposited from multiple parties. - * The contract owner may close the deposit period, and allow for either withdrawal + * The primary account may close the deposit period, and allow for either withdrawal * by the beneficiary, or refunds to the depositors. */ -contract RefundEscrow is Ownable, ConditionalEscrow { +contract RefundEscrow is Secondary, ConditionalEscrow { enum State { Active, Refunding, Closed } event Closed(); @@ -32,14 +32,14 @@ contract RefundEscrow is Ownable, ConditionalEscrow { /** * @return the current state of the escrow. */ - function state() public view returns(State) { + function state() public view returns (State) { return state_; } /** * @return the beneficiary of the escrow. */ - function beneficiary() public view returns(address) { + function beneficiary() public view returns (address) { return beneficiary_; } @@ -56,7 +56,7 @@ contract RefundEscrow is Ownable, ConditionalEscrow { * @dev Allows for the beneficiary to withdraw their funds, rejecting * further deposits. */ - function close() public onlyOwner { + function close() public onlyPrimary { require(state_ == State.Active); state_ = State.Closed; emit Closed(); @@ -65,7 +65,7 @@ contract RefundEscrow is Ownable, ConditionalEscrow { /** * @dev Allows for refunds to take place, rejecting further deposits. */ - function enableRefunds() public onlyOwner { + function enableRefunds() public onlyPrimary { require(state_ == State.Active); state_ = State.Refunding; emit RefundsEnabled(); diff --git a/test/payment/RefundEscrow.test.js b/test/payment/RefundEscrow.test.js index 7730d8242a0..e412816c405 100644 --- a/test/payment/RefundEscrow.test.js +++ b/test/payment/RefundEscrow.test.js @@ -11,20 +11,20 @@ require('chai') const RefundEscrow = artifacts.require('RefundEscrow'); -contract('RefundEscrow', function ([_, owner, beneficiary, refundee1, refundee2]) { +contract('RefundEscrow', function ([_, primary, beneficiary, refundee1, refundee2]) { const amount = web3.toWei(54.0, 'ether'); const refundees = [refundee1, refundee2]; const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000'; it('requires a non-null beneficiary', async function () { await expectThrow( - RefundEscrow.new(ZERO_ADDRESS, { from: owner }) + RefundEscrow.new(ZERO_ADDRESS, { from: primary }) ); }); context('once deployed', function () { beforeEach(async function () { - this.escrow = await RefundEscrow.new(beneficiary, { from: owner }); + this.escrow = await RefundEscrow.new(beneficiary, { from: primary }); }); context('active state', function () { @@ -34,39 +34,39 @@ contract('RefundEscrow', function ([_, owner, beneficiary, refundee1, refundee2] }); it('accepts deposits', async function () { - await this.escrow.deposit(refundee1, { from: owner, value: amount }); + await this.escrow.deposit(refundee1, { from: primary, value: amount }); (await this.escrow.depositsOf(refundee1)).should.be.bignumber.equal(amount); }); it('does not refund refundees', async function () { - await this.escrow.deposit(refundee1, { from: owner, value: amount }); + await this.escrow.deposit(refundee1, { from: primary, value: amount }); await expectThrow(this.escrow.withdraw(refundee1), EVMRevert); }); it('does not allow beneficiary withdrawal', async function () { - await this.escrow.deposit(refundee1, { from: owner, value: amount }); + await this.escrow.deposit(refundee1, { from: primary, value: amount }); await expectThrow(this.escrow.beneficiaryWithdraw(), EVMRevert); }); }); - it('only owner can enter closed state', async function () { + it('only the primary account can enter closed state', async function () { await expectThrow(this.escrow.close({ from: beneficiary }), EVMRevert); - const receipt = await this.escrow.close({ from: owner }); + const receipt = await this.escrow.close({ from: primary }); expectEvent.inLogs(receipt.logs, 'Closed'); }); context('closed state', function () { beforeEach(async function () { - await Promise.all(refundees.map(refundee => this.escrow.deposit(refundee, { from: owner, value: amount }))); + await Promise.all(refundees.map(refundee => this.escrow.deposit(refundee, { from: primary, value: amount }))); - await this.escrow.close({ from: owner }); + await this.escrow.close({ from: primary }); }); it('rejects deposits', async function () { - await expectThrow(this.escrow.deposit(refundee1, { from: owner, value: amount }), EVMRevert); + await expectThrow(this.escrow.deposit(refundee1, { from: primary, value: amount }), EVMRevert); }); it('does not refund refundees', async function () { @@ -82,37 +82,37 @@ contract('RefundEscrow', function ([_, owner, beneficiary, refundee1, refundee2] }); it('prevents entering the refund state', async function () { - await expectThrow(this.escrow.enableRefunds({ from: owner }), EVMRevert); + await expectThrow(this.escrow.enableRefunds({ from: primary }), EVMRevert); }); it('prevents re-entering the closed state', async function () { - await expectThrow(this.escrow.close({ from: owner }), EVMRevert); + await expectThrow(this.escrow.close({ from: primary }), EVMRevert); }); }); - it('only owner can enter refund state', async function () { + it('only the primary account can enter refund state', async function () { await expectThrow(this.escrow.enableRefunds({ from: beneficiary }), EVMRevert); - const receipt = await this.escrow.enableRefunds({ from: owner }); + const receipt = await this.escrow.enableRefunds({ from: primary }); expectEvent.inLogs(receipt.logs, 'RefundsEnabled'); }); context('refund state', function () { beforeEach(async function () { - await Promise.all(refundees.map(refundee => this.escrow.deposit(refundee, { from: owner, value: amount }))); + await Promise.all(refundees.map(refundee => this.escrow.deposit(refundee, { from: primary, value: amount }))); - await this.escrow.enableRefunds({ from: owner }); + await this.escrow.enableRefunds({ from: primary }); }); it('rejects deposits', async function () { - await expectThrow(this.escrow.deposit(refundee1, { from: owner, value: amount }), EVMRevert); + await expectThrow(this.escrow.deposit(refundee1, { from: primary, value: amount }), EVMRevert); }); it('refunds refundees', async function () { for (const refundee of [refundee1, refundee2]) { const refundeeInitialBalance = await ethGetBalance(refundee); - await this.escrow.withdraw(refundee, { from: owner }); + await this.escrow.withdraw(refundee, { from: primary }); const refundeeFinalBalance = await ethGetBalance(refundee); refundeeFinalBalance.sub(refundeeInitialBalance).should.be.bignumber.equal(amount); @@ -124,11 +124,11 @@ contract('RefundEscrow', function ([_, owner, beneficiary, refundee1, refundee2] }); it('prevents entering the closed state', async function () { - await expectThrow(this.escrow.close({ from: owner }), EVMRevert); + await expectThrow(this.escrow.close({ from: primary }), EVMRevert); }); it('prevents re-entering the refund state', async function () { - await expectThrow(this.escrow.enableRefunds({ from: owner }), EVMRevert); + await expectThrow(this.escrow.enableRefunds({ from: primary }), EVMRevert); }); }); }); From 7fbc928592d2a9ae219fa1943f287585a2c123bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Thu, 6 Sep 2018 16:20:26 -0300 Subject: [PATCH 3/5] FinalizableCrowdsale is no longer Ownable. --- .../distribution/FinalizableCrowdsale.sol | 11 +++++----- .../distribution/RefundableCrowdsale.sol | 2 +- test/crowdsale/FinalizableCrowdsale.test.js | 21 +++++++------------ test/crowdsale/RefundableCrowdsale.test.js | 10 ++++----- 4 files changed, 19 insertions(+), 25 deletions(-) diff --git a/contracts/crowdsale/distribution/FinalizableCrowdsale.sol b/contracts/crowdsale/distribution/FinalizableCrowdsale.sol index 693e0b498e1..d8aa1eeabae 100644 --- a/contracts/crowdsale/distribution/FinalizableCrowdsale.sol +++ b/contracts/crowdsale/distribution/FinalizableCrowdsale.sol @@ -1,16 +1,15 @@ pragma solidity ^0.4.24; import "../../math/SafeMath.sol"; -import "../../ownership/Ownable.sol"; import "../validation/TimedCrowdsale.sol"; /** * @title FinalizableCrowdsale - * @dev Extension of Crowdsale where an owner can do extra work - * after finishing. + * @dev Extension of Crowdsale with a one-off finalization action, where one + * can do extra work after finishing. */ -contract FinalizableCrowdsale is Ownable, TimedCrowdsale { +contract FinalizableCrowdsale is TimedCrowdsale { using SafeMath for uint256; bool private finalized_ = false; @@ -20,7 +19,7 @@ contract FinalizableCrowdsale is Ownable, TimedCrowdsale { /** * @return true if the crowdsale is finalized, false otherwise. */ - function finalized() public view returns(bool) { + function finalized() public view returns (bool) { return finalized_; } @@ -28,7 +27,7 @@ contract FinalizableCrowdsale is Ownable, TimedCrowdsale { * @dev Must be called after crowdsale ends, to do some extra finalization * work. Calls the contract's finalization function. */ - function finalize() public onlyOwner { + function finalize() public { require(!finalized_); require(hasClosed()); diff --git a/contracts/crowdsale/distribution/RefundableCrowdsale.sol b/contracts/crowdsale/distribution/RefundableCrowdsale.sol index 6279fbbdeb9..6e8f368238f 100644 --- a/contracts/crowdsale/distribution/RefundableCrowdsale.sol +++ b/contracts/crowdsale/distribution/RefundableCrowdsale.sol @@ -57,7 +57,7 @@ contract RefundableCrowdsale is FinalizableCrowdsale { } /** - * @dev escrow finalization task, called when owner calls finalize() + * @dev escrow finalization task, called when finalize() is called */ function _finalization() internal { if (goalReached()) { diff --git a/test/crowdsale/FinalizableCrowdsale.test.js b/test/crowdsale/FinalizableCrowdsale.test.js index e6f2914ea58..df7a743dd20 100644 --- a/test/crowdsale/FinalizableCrowdsale.test.js +++ b/test/crowdsale/FinalizableCrowdsale.test.js @@ -13,7 +13,7 @@ const should = require('chai') const FinalizableCrowdsale = artifacts.require('FinalizableCrowdsaleImpl'); const ERC20 = artifacts.require('ERC20'); -contract('FinalizableCrowdsale', function ([_, owner, wallet, thirdparty]) { +contract('FinalizableCrowdsale', function ([_, wallet, anyone]) { const rate = new BigNumber(1000); before(async function () { @@ -28,33 +28,28 @@ contract('FinalizableCrowdsale', function ([_, owner, wallet, thirdparty]) { this.token = await ERC20.new(); this.crowdsale = await FinalizableCrowdsale.new( - this.openingTime, this.closingTime, rate, wallet, this.token.address, { from: owner } + this.openingTime, this.closingTime, rate, wallet, this.token.address ); }); it('cannot be finalized before ending', async function () { - await expectThrow(this.crowdsale.finalize({ from: owner }), EVMRevert); + await expectThrow(this.crowdsale.finalize({ from: anyone }), EVMRevert); }); - it('cannot be finalized by third party after ending', async function () { + it('can be finalized by anyone after ending', async function () { await increaseTimeTo(this.afterClosingTime); - await expectThrow(this.crowdsale.finalize({ from: thirdparty }), EVMRevert); - }); - - it('can be finalized by owner after ending', async function () { - await increaseTimeTo(this.afterClosingTime); - await this.crowdsale.finalize({ from: owner }); + await this.crowdsale.finalize({ from: anyone }); }); it('cannot be finalized twice', async function () { await increaseTimeTo(this.afterClosingTime); - await this.crowdsale.finalize({ from: owner }); - await expectThrow(this.crowdsale.finalize({ from: owner }), EVMRevert); + await this.crowdsale.finalize({ from: anyone }); + await expectThrow(this.crowdsale.finalize({ from: anyone }), EVMRevert); }); it('logs finalized', async function () { await increaseTimeTo(this.afterClosingTime); - const { logs } = await this.crowdsale.finalize({ from: owner }); + const { logs } = await this.crowdsale.finalize({ from: anyone }); const event = logs.find(e => e.event === 'CrowdsaleFinalized'); should.exist(event); }); diff --git a/test/crowdsale/RefundableCrowdsale.test.js b/test/crowdsale/RefundableCrowdsale.test.js index c031073353f..3a5305ceb5d 100644 --- a/test/crowdsale/RefundableCrowdsale.test.js +++ b/test/crowdsale/RefundableCrowdsale.test.js @@ -15,7 +15,7 @@ require('chai') const RefundableCrowdsale = artifacts.require('RefundableCrowdsaleImpl'); const SimpleToken = artifacts.require('SimpleToken'); -contract('RefundableCrowdsale', function ([_, owner, wallet, investor, purchaser]) { +contract('RefundableCrowdsale', function ([_, wallet, investor, purchaser, anyone]) { const rate = new BigNumber(1); const goal = ether(50); const lessThanGoal = ether(45); @@ -38,7 +38,7 @@ contract('RefundableCrowdsale', function ([_, owner, wallet, investor, purchaser it('rejects a goal of zero', async function () { await expectThrow( RefundableCrowdsale.new( - this.openingTime, this.closingTime, rate, wallet, this.token.address, 0, { from: owner } + this.openingTime, this.closingTime, rate, wallet, this.token.address, 0, ), EVMRevert, ); @@ -47,7 +47,7 @@ contract('RefundableCrowdsale', function ([_, owner, wallet, investor, purchaser context('with crowdsale', function () { beforeEach(async function () { this.crowdsale = await RefundableCrowdsale.new( - this.openingTime, this.closingTime, rate, wallet, this.token.address, goal, { from: owner } + this.openingTime, this.closingTime, rate, wallet, this.token.address, goal ); await this.token.transfer(this.crowdsale.address, tokenSupply); @@ -76,7 +76,7 @@ contract('RefundableCrowdsale', function ([_, owner, wallet, investor, purchaser context('after closing time and finalization', function () { beforeEach(async function () { await increaseTimeTo(this.afterClosingTime); - await this.crowdsale.finalize({ from: owner }); + await this.crowdsale.finalize({ from: anyone }); }); it('refunds', async function () { @@ -96,7 +96,7 @@ contract('RefundableCrowdsale', function ([_, owner, wallet, investor, purchaser context('after closing time and finalization', function () { beforeEach(async function () { await increaseTimeTo(this.afterClosingTime); - await this.crowdsale.finalize({ from: owner }); + await this.crowdsale.finalize({ from: anyone }); }); it('denies refunds', async function () { From f81f1e7e1e8f3dc057f918baa9c7f87860a6a808 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Thu, 6 Sep 2018 16:48:02 -0300 Subject: [PATCH 4/5] Removed Whitelist and WhitelistedCrowdsale, redesign needed. --- contracts/access/Whitelist.sol | 94 ------------------- .../validation/WhitelistedCrowdsale.sol | 27 ------ contracts/mocks/WhitelistMock.sol | 14 --- contracts/mocks/WhitelistedCrowdsaleImpl.sol | 19 ---- test/access/Whitelist.test.js | 65 ------------- test/crowdsale/WhitelistedCrowdsale.test.js | 89 ------------------ 6 files changed, 308 deletions(-) delete mode 100644 contracts/access/Whitelist.sol delete mode 100644 contracts/crowdsale/validation/WhitelistedCrowdsale.sol delete mode 100644 contracts/mocks/WhitelistMock.sol delete mode 100644 contracts/mocks/WhitelistedCrowdsaleImpl.sol delete mode 100644 test/access/Whitelist.test.js delete mode 100644 test/crowdsale/WhitelistedCrowdsale.test.js diff --git a/contracts/access/Whitelist.sol b/contracts/access/Whitelist.sol deleted file mode 100644 index 3d9b3da5c6f..00000000000 --- a/contracts/access/Whitelist.sol +++ /dev/null @@ -1,94 +0,0 @@ -pragma solidity ^0.4.24; - - -import "../ownership/Ownable.sol"; -import "../access/rbac/RBAC.sol"; - - -/** - * @title Whitelist - * @dev The Whitelist contract has a whitelist of addresses, and provides basic authorization control functions. - * This simplifies the implementation of "user permissions". - */ -contract Whitelist is Ownable, RBAC { - - // Name of the whitelisted role. - string private constant ROLE_WHITELISTED = "whitelist"; - - /** - * @dev Throws if operator is not whitelisted. - * @param _operator address - */ - modifier onlyIfWhitelisted(address _operator) { - checkRole(_operator, ROLE_WHITELISTED); - _; - } - - /** - * @dev add an address to the whitelist - * @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 _operator) - public - onlyOwner - { - _addRole(_operator, ROLE_WHITELISTED); - } - - /** - * @dev Determine if an account is whitelisted. - * @return true if the account is whitelisted, false otherwise. - */ - function isWhitelisted(address _operator) - public - view - returns (bool) - { - return hasRole(_operator, ROLE_WHITELISTED); - } - - /** - * @dev add addresses to the whitelist - * @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[] _operators) - public - onlyOwner - { - for (uint256 i = 0; i < _operators.length; i++) { - addAddressToWhitelist(_operators[i]); - } - } - - /** - * @dev remove an address from the whitelist - * @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 _operator) - public - onlyOwner - { - _removeRole(_operator, ROLE_WHITELISTED); - } - - /** - * @dev remove addresses from the whitelist - * @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[] _operators) - public - onlyOwner - { - 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 deleted file mode 100644 index e3d1de08e7f..00000000000 --- a/contracts/crowdsale/validation/WhitelistedCrowdsale.sol +++ /dev/null @@ -1,27 +0,0 @@ -pragma solidity ^0.4.24; - -import "../Crowdsale.sol"; -import "../../access/Whitelist.sol"; - - -/** - * @title WhitelistedCrowdsale - * @dev Crowdsale in which only whitelisted users can contribute. - */ -contract WhitelistedCrowdsale is Whitelist, Crowdsale { - /** - * @dev Extend parent behavior requiring beneficiary to be in whitelist. - * @param _beneficiary Token beneficiary - * @param _weiAmount Amount of wei contributed - */ - function _preValidatePurchase( - address _beneficiary, - uint256 _weiAmount - ) - internal - onlyIfWhitelisted(_beneficiary) - { - super._preValidatePurchase(_beneficiary, _weiAmount); - } - -} diff --git a/contracts/mocks/WhitelistMock.sol b/contracts/mocks/WhitelistMock.sol deleted file mode 100644 index 081b6631bb2..00000000000 --- a/contracts/mocks/WhitelistMock.sol +++ /dev/null @@ -1,14 +0,0 @@ -pragma solidity ^0.4.24; - -import "../access/Whitelist.sol"; - - -contract WhitelistMock is Whitelist { - - function onlyWhitelistedCanDoThis() - external - onlyIfWhitelisted(msg.sender) - view - { - } -} diff --git a/contracts/mocks/WhitelistedCrowdsaleImpl.sol b/contracts/mocks/WhitelistedCrowdsaleImpl.sol deleted file mode 100644 index 25cd5118aea..00000000000 --- a/contracts/mocks/WhitelistedCrowdsaleImpl.sol +++ /dev/null @@ -1,19 +0,0 @@ -pragma solidity ^0.4.24; - -import "../token/ERC20/IERC20.sol"; -import "../crowdsale/validation/WhitelistedCrowdsale.sol"; -import "../crowdsale/Crowdsale.sol"; - - -contract WhitelistedCrowdsaleImpl is Crowdsale, WhitelistedCrowdsale { - - constructor ( - uint256 _rate, - address _wallet, - IERC20 _token - ) - Crowdsale(_rate, _wallet, _token) - public - { - } -} diff --git a/test/access/Whitelist.test.js b/test/access/Whitelist.test.js deleted file mode 100644 index f1113ffc3c9..00000000000 --- a/test/access/Whitelist.test.js +++ /dev/null @@ -1,65 +0,0 @@ -const { expectThrow } = require('../helpers/expectThrow'); - -const WhitelistMock = artifacts.require('WhitelistMock'); - -require('chai') - .should(); - -contract('Whitelist', function ([_, owner, whitelistedAddress1, whitelistedAddress2, anyone]) { - const whitelistedAddresses = [whitelistedAddress1, whitelistedAddress2]; - - beforeEach(async function () { - this.mock = await WhitelistMock.new({ from: owner }); - }); - - context('in normal conditions', function () { - it('should add address to the whitelist', async function () { - await this.mock.addAddressToWhitelist(whitelistedAddress1, { from: owner }); - (await this.mock.isWhitelisted(whitelistedAddress1)).should.equal(true); - }); - - it('should add addresses to the whitelist', async function () { - await this.mock.addAddressesToWhitelist(whitelistedAddresses, { from: owner }); - for (const addr of whitelistedAddresses) { - (await this.mock.isWhitelisted(addr)).should.equal(true); - } - }); - - it('should remove address from the whitelist', async function () { - await this.mock.removeAddressFromWhitelist(whitelistedAddress1, { from: owner }); - (await this.mock.isWhitelisted(whitelistedAddress1)).should.equal(false); - }); - - it('should remove addresses from the the whitelist', async function () { - await this.mock.removeAddressesFromWhitelist(whitelistedAddresses, { from: owner }); - for (const addr of whitelistedAddresses) { - (await this.mock.isWhitelisted(addr)).should.equal(false); - } - }); - - it('should allow whitelisted address to call #onlyWhitelistedCanDoThis', async function () { - await this.mock.addAddressToWhitelist(whitelistedAddress1, { from: owner }); - await this.mock.onlyWhitelistedCanDoThis({ from: whitelistedAddress1 }); - }); - }); - - context('in adversarial conditions', function () { - it('should not allow "anyone" to add to the whitelist', async function () { - await expectThrow( - this.mock.addAddressToWhitelist(whitelistedAddress1, { from: anyone }) - ); - }); - - it('should not allow "anyone" to remove from the whitelist', async function () { - await expectThrow( - this.mock.removeAddressFromWhitelist(whitelistedAddress1, { from: anyone }) - ); - }); - - it('should not allow "anyone" to call #onlyWhitelistedCanDoThis', async function () { - await expectThrow( - this.mock.onlyWhitelistedCanDoThis({ from: anyone }) - ); - }); - }); -}); diff --git a/test/crowdsale/WhitelistedCrowdsale.test.js b/test/crowdsale/WhitelistedCrowdsale.test.js deleted file mode 100644 index 3cf9b3206b9..00000000000 --- a/test/crowdsale/WhitelistedCrowdsale.test.js +++ /dev/null @@ -1,89 +0,0 @@ -const { ether } = require('../helpers/ether'); -const { expectThrow } = require('../helpers/expectThrow'); - -const BigNumber = web3.BigNumber; - -require('chai') - .should(); - -const WhitelistedCrowdsale = artifacts.require('WhitelistedCrowdsaleImpl'); -const SimpleToken = artifacts.require('SimpleToken'); - -contract('WhitelistedCrowdsale', function ([_, wallet, authorized, unauthorized, anotherAuthorized]) { - const rate = 1; - const value = ether(42); - const tokenSupply = new BigNumber('1e22'); - - describe('single user whitelisting', function () { - beforeEach(async function () { - 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.addAddressToWhitelist(authorized); - }); - - describe('accepting payments', function () { - it('should accept payments to whitelisted (from whichever buyers)', async function () { - await this.crowdsale.sendTransaction({ value, from: authorized }); - await this.crowdsale.buyTokens(authorized, { value: value, from: authorized }); - await this.crowdsale.buyTokens(authorized, { value: value, from: unauthorized }); - }); - - it('should reject payments to not whitelisted (from whichever buyers)', async function () { - await expectThrow(this.crowdsale.sendTransaction({ value, from: unauthorized })); - await expectThrow(this.crowdsale.buyTokens(unauthorized, { value: value, from: unauthorized })); - await expectThrow(this.crowdsale.buyTokens(unauthorized, { value: value, from: authorized })); - }); - - it('should reject payments to addresses removed from whitelist', async function () { - await this.crowdsale.removeAddressFromWhitelist(authorized); - await expectThrow(this.crowdsale.buyTokens(authorized, { value: value, from: authorized })); - }); - }); - - describe('reporting whitelisted', function () { - it('should correctly report whitelisted addresses', async function () { - (await this.crowdsale.isWhitelisted(authorized)).should.equal(true); - (await this.crowdsale.isWhitelisted(unauthorized)).should.equal(false); - }); - }); - }); - - describe('many user whitelisting', function () { - beforeEach(async function () { - 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.addAddressesToWhitelist([authorized, anotherAuthorized]); - }); - - describe('accepting payments', function () { - it('should accept payments to whitelisted (from whichever buyers)', async function () { - await this.crowdsale.buyTokens(authorized, { value: value, from: authorized }); - await this.crowdsale.buyTokens(authorized, { value: value, from: unauthorized }); - await this.crowdsale.buyTokens(anotherAuthorized, { value: value, from: authorized }); - await this.crowdsale.buyTokens(anotherAuthorized, { value: value, from: unauthorized }); - }); - - it('should reject payments to not whitelisted (with whichever buyers)', async function () { - await expectThrow(this.crowdsale.send(value)); - await expectThrow(this.crowdsale.buyTokens(unauthorized, { value: value, from: unauthorized })); - await expectThrow(this.crowdsale.buyTokens(unauthorized, { value: value, from: authorized })); - }); - - it('should reject payments to addresses removed from whitelist', async function () { - await this.crowdsale.removeAddressFromWhitelist(anotherAuthorized); - await this.crowdsale.buyTokens(authorized, { value: value, from: authorized }); - await expectThrow(this.crowdsale.buyTokens(anotherAuthorized, { value: value, from: authorized })); - }); - }); - - describe('reporting whitelisted', function () { - it('should correctly report whitelisted addresses', async function () { - (await this.crowdsale.isWhitelisted(authorized)).should.equal(true); - (await this.crowdsale.isWhitelisted(anotherAuthorized)).should.equal(true); - (await this.crowdsale.isWhitelisted(unauthorized)).should.equal(false); - }); - }); - }); -}); From bd42b6cc0169470f8a1275fc71afeca7c5432b42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Thu, 6 Sep 2018 17:52:44 -0300 Subject: [PATCH 5/5] Fixed linter errors, disabled lbrace due to it being buggy. --- .soliumrc.json | 1 + contracts/mocks/IndividuallyCappedCrowdsaleImpl.sol | 6 +++--- test/crowdsale/IndividuallyCappedCrowdsale.test.js | 11 ++++++----- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/.soliumrc.json b/.soliumrc.json index 6a08c646678..be4afce1095 100644 --- a/.soliumrc.json +++ b/.soliumrc.json @@ -4,6 +4,7 @@ "rules": { "error-reason": "off", "indentation": ["error", 2], + "lbrace": "off", "linebreak-style": ["error", "unix"], "max-len": ["error", 79], "no-constant": ["error"], diff --git a/contracts/mocks/IndividuallyCappedCrowdsaleImpl.sol b/contracts/mocks/IndividuallyCappedCrowdsaleImpl.sol index 696677bb7fa..552e9f8c5ca 100644 --- a/contracts/mocks/IndividuallyCappedCrowdsaleImpl.sol +++ b/contracts/mocks/IndividuallyCappedCrowdsaleImpl.sol @@ -5,9 +5,10 @@ import "../crowdsale/validation/IndividuallyCappedCrowdsale.sol"; import "./CapperRoleMock.sol"; -contract IndividuallyCappedCrowdsaleImpl is IndividuallyCappedCrowdsale, CapperRoleMock { +contract IndividuallyCappedCrowdsaleImpl + is IndividuallyCappedCrowdsale, CapperRoleMock { - constructor ( + constructor( uint256 _rate, address _wallet, IERC20 _token @@ -16,5 +17,4 @@ contract IndividuallyCappedCrowdsaleImpl is IndividuallyCappedCrowdsale, CapperR Crowdsale(_rate, _wallet, _token) { } - } diff --git a/test/crowdsale/IndividuallyCappedCrowdsale.test.js b/test/crowdsale/IndividuallyCappedCrowdsale.test.js index bec1b3668dd..64eddec9748 100644 --- a/test/crowdsale/IndividuallyCappedCrowdsale.test.js +++ b/test/crowdsale/IndividuallyCappedCrowdsale.test.js @@ -12,7 +12,8 @@ const IndividuallyCappedCrowdsaleImpl = artifacts.require('IndividuallyCappedCro const SimpleToken = artifacts.require('SimpleToken'); const { shouldBehaveLikePublicRole } = require('../access/rbac/PublicRole.behavior'); -contract('IndividuallyCappedCrowdsale', function ([_, capper, otherCapper, wallet, alice, bob, charlie, anyone, ...otherAccounts]) { +contract('IndividuallyCappedCrowdsale', function ( + [_, capper, otherCapper, wallet, alice, bob, charlie, anyone, ...otherAccounts]) { const rate = new BigNumber(1); const capAlice = ether(10); const capBob = ether(2); @@ -35,12 +36,12 @@ contract('IndividuallyCappedCrowdsale', function ([_, capper, otherCapper, walle }); describe('individual caps', function () { - it('sets a cap when the sender is a capper', async function () { + it('sets a cap when the sender is a capper', async function () { await this.crowdsale.setUserCap(alice, capAlice, { from: capper }); (await this.crowdsale.getUserCap(alice)).should.be.bignumber.equal(capAlice); }); - it('reverts when a non-capper sets a cap', async function () { + it('reverts when a non-capper sets a cap', async function () { await expectThrow(this.crowdsale.setUserCap(alice, capAlice, { from: anyone }), EVMRevert); }); @@ -91,13 +92,13 @@ contract('IndividuallyCappedCrowdsale', function ([_, capper, otherCapper, walle }); describe('group capping', function () { - it('sets caps when the sender is a capper', async function () { + it('sets caps when the sender is a capper', async function () { await this.crowdsale.setGroupCap([bob, charlie], capBob, { from: capper }); (await this.crowdsale.getUserCap(bob)).should.be.bignumber.equal(capBob); (await this.crowdsale.getUserCap(charlie)).should.be.bignumber.equal(capBob); }); - it('reverts when a non-capper set caps', async function () { + it('reverts when a non-capper set caps', async function () { await expectThrow(this.crowdsale.setGroupCap([bob, charlie], capBob, { from: anyone }), EVMRevert); });