Refundable post delivery crowdsale (#1543)

* Fixed unnecessary dependency of RefundableCrowdsaleImpl on ERC20Mintable.

* Added PostDeliveryRefundableCrowdsale.

* Renamed to RefundablePostDeliveryCrowdsale.

* Added deprecation warning.

contracts/crowdsale/distribution/RefundableCrowdsale.sol

@@ -6,21 +6,13 @@ import "../../payment/escrow/RefundEscrow.sol";
 
 /**
  * @title RefundableCrowdsale
- * @dev Extension of Crowdsale contract that adds a funding goal, and
- * the possibility of users getting a refund if goal is not met.
- * WARNING: note that if you allow tokens to be traded before the goal
- * is met, then an attack is possible in which the attacker purchases
- * tokens from the crowdsale and when they sees that the goal is
- * unlikely to be met, they sell their tokens (possibly at a discount).
- * The attacker will be refunded when the crowdsale is finalized, and
- * the users that purchased from them will be left with worthless
- * tokens. There are many possible ways to avoid this, like making the
- * the crowdsale inherit from PostDeliveryCrowdsale, or imposing
- * restrictions on token trading until the crowdsale is finalized.
- * This is being discussed in
- * https://github.com/OpenZeppelin/openzeppelin-solidity/issues/877
- * This contract will be updated when we agree on a general solution
- * for this problem.
+ * @dev Extension of Crowdsale contract that adds a funding goal, and the possibility of users getting a refund if goal
+ * is not met.
+ *
+ * Deprecated, use RefundablePostDeliveryCrowdsale instead. Note that if you allow tokens to be traded before the goal
+ * is met, then an attack is possible in which the attacker purchases tokens from the crowdsale and when they sees that
+ * the goal is unlikely to be met, they sell their tokens (possibly at a discount). The attacker will be refunded when
+ * the crowdsale is finalized, and the users that purchased from them will be left with worthless tokens.
  */
 contract RefundableCrowdsale is FinalizableCrowdsale {
     using SafeMath for uint256;

contracts/crowdsale/distribution/RefundablePostDeliveryCrowdsale.sol

@@ -0,0 +1,20 @@
+pragma solidity ^0.4.24;
+
+import "./RefundableCrowdsale.sol";
+import "./PostDeliveryCrowdsale.sol";
+
+
+/**
+ * @title RefundablePostDeliveryCrowdsale
+ * @dev Extension of RefundableCrowdsale contract that only delivers the tokens
+ * once the crowdsale has closed and the goal met, preventing refunds to be issued
+ * to token holders.
+ */
+contract RefundablePostDeliveryCrowdsale is RefundableCrowdsale, PostDeliveryCrowdsale {
+    function withdrawTokens(address beneficiary) public {
+        require(finalized());
+        require(goalReached());
+
+        super.withdrawTokens(beneficiary);
+    }
+}

contracts/mocks/RefundableCrowdsaleImpl.sol

@@ -1,6 +1,6 @@
 pragma solidity ^0.4.24;
 
-import "../token/ERC20/ERC20Mintable.sol";
+import "../token/ERC20/IERC20.sol";
 import "../crowdsale/distribution/RefundableCrowdsale.sol";
 
 contract RefundableCrowdsaleImpl is RefundableCrowdsale {
@@ -9,7 +9,7 @@ contract RefundableCrowdsaleImpl is RefundableCrowdsale {
         uint256 closingTime,
         uint256 rate,
         address wallet,
-        ERC20Mintable token,
+        IERC20 token,
         uint256 goal
     )
         public

contracts/mocks/RefundablePostDeliveryCrowdsaleImpl.sol

@@ -0,0 +1,20 @@
+pragma solidity ^0.4.24;
+
+import "../token/ERC20/IERC20.sol";
+import "../crowdsale/distribution/RefundablePostDeliveryCrowdsale.sol";
+
+contract RefundablePostDeliveryCrowdsaleImpl is RefundablePostDeliveryCrowdsale {
+    constructor (
+        uint256 openingTime,
+        uint256 closingTime,
+        uint256 rate,
+        address wallet,
+        IERC20 token,
+        uint256 goal
+    )
+        public
+        Crowdsale(rate, wallet, token)
+        TimedCrowdsale(openingTime, closingTime)
+        RefundableCrowdsale(goal)
+    {}
+}

test/crowdsale/RefundablePostDeliveryCrowdsale.test.js

@@ -0,0 +1,103 @@
+const time = require('../helpers/time');
+const shouldFail = require('../helpers/shouldFail');
+const { ether } = require('../helpers/ether');
+
+const BigNumber = web3.BigNumber;
+
+require('chai')
+  .use(require('chai-bignumber')(BigNumber))
+  .should();
+
+const RefundablePostDeliveryCrowdsaleImpl = artifacts.require('RefundablePostDeliveryCrowdsaleImpl');
+const SimpleToken = artifacts.require('SimpleToken');
+
+contract('RefundablePostDeliveryCrowdsale', function ([_, investor, wallet, purchaser]) {
+  const rate = new BigNumber(1);
+  const tokenSupply = new BigNumber('1e22');
+  const goal = ether(100);
+
+  before(async function () {
+    // Advance to the next block to correctly read time in the solidity "now" function interpreted by ganache
+    await time.advanceBlock();
+  });
+
+  beforeEach(async function () {
+    this.openingTime = (await time.latest()) + time.duration.weeks(1);
+    this.closingTime = this.openingTime + time.duration.weeks(1);
+    this.afterClosingTime = this.closingTime + time.duration.seconds(1);
+    this.token = await SimpleToken.new();
+    this.crowdsale = await RefundablePostDeliveryCrowdsaleImpl.new(
+      this.openingTime, this.closingTime, rate, wallet, this.token.address, goal
+    );
+    await this.token.transfer(this.crowdsale.address, tokenSupply);
+  });
+
+  context('after opening time', function () {
+    beforeEach(async function () {
+      await time.increaseTo(this.openingTime);
+    });
+
+    context('with bought tokens below the goal', function () {
+      const value = goal.sub(1);
+
+      beforeEach(async function () {
+        await this.crowdsale.buyTokens(investor, { value: value, from: purchaser });
+      });
+
+      it('does not immediately deliver tokens to beneficiaries', async function () {
+        (await this.crowdsale.balanceOf(investor)).should.be.bignumber.equal(value);
+        (await this.token.balanceOf(investor)).should.be.bignumber.equal(0);
+      });
+
+      it('does not allow beneficiaries to withdraw tokens before crowdsale ends', async function () {
+        await shouldFail.reverting(this.crowdsale.withdrawTokens(investor));
+      });
+
+      context('after closing time and finalization', function () {
+        beforeEach(async function () {
+          await time.increaseTo(this.afterClosingTime);
+          await this.crowdsale.finalize();
+        });
+
+        it('rejects token withdrawals', async function () {
+          await shouldFail.reverting(this.crowdsale.withdrawTokens(investor));
+        });
+      });
+    });
+
+    context('with bought tokens matching the goal', function () {
+      const value = goal;
+
+      beforeEach(async function () {
+        await this.crowdsale.buyTokens(investor, { value: value, from: purchaser });
+      });
+
+      it('does not immediately deliver tokens to beneficiaries', async function () {
+        (await this.crowdsale.balanceOf(investor)).should.be.bignumber.equal(value);
+        (await this.token.balanceOf(investor)).should.be.bignumber.equal(0);
+      });
+
+      it('does not allow beneficiaries to withdraw tokens before crowdsale ends', async function () {
+        await shouldFail.reverting(this.crowdsale.withdrawTokens(investor));
+      });
+
+      context('after closing time and finalization', function () {
+        beforeEach(async function () {
+          await time.increaseTo(this.afterClosingTime);
+          await this.crowdsale.finalize();
+        });
+
+        it('allows beneficiaries to withdraw tokens', async function () {
+          await this.crowdsale.withdrawTokens(investor);
+          (await this.crowdsale.balanceOf(investor)).should.be.bignumber.equal(0);
+          (await this.token.balanceOf(investor)).should.be.bignumber.equal(value);
+        });
+
+        it('rejects multiple withdrawals', async function () {
+          await this.crowdsale.withdrawTokens(investor);
+          await shouldFail.reverting(this.crowdsale.withdrawTokens(investor));
+        });
+      });
+    });
+  });
+});