Removed selfdestruct from BreakInvariantBounty (OpenZeppelin#1385)

* signing prefix added

* Minor improvement

* Tests changed

* Successfully tested

* Minor improvements

* Minor improvements

* Revert "Dangling commas are now required. (OpenZeppelin#1359)"

This reverts commit a688977.

* updates

* fixes OpenZeppelin#1384

* introduced claimable and cancelBounty

* cancelBounty tests

* Update BreakInvariantBounty.test.js

(cherry picked from commit 41f84f8)

contracts/drafts/BreakInvariantBounty.sol

@@ -10,24 +10,25 @@ import "../ownership/Ownable.sol";
  * @dev This bounty will pay out to a researcher if they break invariant logic of the contract.
  */
 contract BreakInvariantBounty is PullPayment, Ownable {
-  bool private _claimed;
+  bool private _claimable = true;
   mapping(address => address) private _researchers;
 
   event TargetCreated(address createdAddress);
+  event BountyCanceled();
 
   /**
    * @dev Fallback function allowing the contract to receive funds, if they haven't already been claimed.
    */
   function() external payable {
-    require(!_claimed);
+    require(_claimable);
   }
 
   /**
-   * @dev Determine if the bounty was claimed.
-   * @return true if the bounty was claimed, false otherwise.
+   * @dev Determine if the bounty is claimable.
+   * @return false if the bounty was claimed, true otherwise.
    */
-  function claimed() public view returns(bool) {
-    return _claimed;
+  function claimable() public view returns(bool) {
+    return _claimable;
   }
 
   /**
@@ -47,20 +48,23 @@ contract BreakInvariantBounty is PullPayment, Ownable {
    * @param target contract
    */
   function claim(Target target) public {
-    require(!_claimed);
+    require(_claimable);
     address researcher = _researchers[target];
     require(researcher != address(0));
     // Check Target contract invariants
     require(!target.checkInvariant());
     _asyncTransfer(researcher, address(this).balance);
-    _claimed = true;
+    _claimable = false;
   }
 
   /**
-   * @dev Transfers the current balance to the owner and terminates the contract.
+   * @dev Cancels the bounty and transfers all funds to the owner
    */
-  function destroy() public onlyOwner {
-    selfdestruct(owner());
+  function cancelBounty() public onlyOwner{
+    require(_claimable);
+    _asyncTransfer(owner(), address(this).balance);
+    _claimable = false;
+    emit BountyCanceled();
   }
 
   /**

test/drafts/BreakInvariantBounty.test.js

@@ -1,6 +1,8 @@
-const { ethGetBalance, ethSendTransaction } = require('../helpers/web3');
-const expectEvent = require('../helpers/expectEvent');
-const { assertRevert } = require('../helpers/assertRevert');
+const { ethGetBalance, ethSendTransaction } = require('./helpers/web3');
+const { ether } = require('./helpers/ether');
+const { balanceDifference } = require('./helpers/balanceDiff');
+const expectEvent = require('./helpers/expectEvent');
+const { assertRevert } = require('./helpers/assertRevert');
 
 const BreakInvariantBountyMock = artifacts.require('BreakInvariantBountyMock');
 const TargetMock = artifacts.require('TargetMock');
@@ -9,7 +11,7 @@ require('chai')
   .use(require('chai-bignumber')(web3.BigNumber))
   .should();
 
-const reward = new web3.BigNumber(web3.toWei(1, 'ether'));
+const reward = ether(1);
 
 contract('BreakInvariantBounty', function ([_, owner, researcher, anyone, nonTarget]) {
   beforeEach(async function () {
@@ -26,24 +28,9 @@ contract('BreakInvariantBounty', function ([_, owner, researcher, anyone, nonTar
       await ethSendTransaction({ from: owner, to: this.bounty.address, value: reward });
     });
 
-    describe('destroy', function () {
-      it('returns all balance to the owner', async function () {
-        const ownerPreBalance = await ethGetBalance(owner);
-        await this.bounty.destroy({ from: owner, gasPrice: 0 });
-        const ownerPostBalance = await ethGetBalance(owner);
-
-        (await ethGetBalance(this.bounty.address)).should.be.bignumber.equal(0);
-        ownerPostBalance.sub(ownerPreBalance).should.be.bignumber.equal(reward);
-      });
-
-      it('reverts when called by anyone', async function () {
-        await assertRevert(this.bounty.destroy({ from: anyone }));
-      });
-    });
-
     describe('claim', function () {
-      it('is initially unclaimed', async function () {
-        (await this.bounty.claimed()).should.equal(false);
+      it('is initially claimable', async function () {
+        (await this.bounty.claimable()).should.equal(true);
       });
 
       it('can create claimable target', async function () {
@@ -85,8 +72,8 @@ contract('BreakInvariantBounty', function ([_, owner, researcher, anyone, nonTar
               await this.bounty.claim(this.target.address, { from: researcher });
             });
 
-            it('is claimed', async function () {
-              (await this.bounty.claimed()).should.equal(true);
+            it('is not claimable', async function () {
+              (await this.bounty.claimable()).should.equal(false);
             });
 
             it('no longer accepts rewards', async function () {
@@ -106,5 +93,42 @@ contract('BreakInvariantBounty', function ([_, owner, researcher, anyone, nonTar
         });
       });
     });
+
+    describe('cancelBounty', function () {
+      context('before canceling', function () {
+        it('is claimable', async function () {
+          (await this.bounty.claimable()).should.equal(true);
+        });
+
+        it('can be canceled by the owner', async function () {
+          const { logs } = await this.bounty.cancelBounty({ from: owner });
+          expectEvent.inLogs(logs, 'BountyCanceled');
+          (await balanceDifference(owner, () => this.bounty.withdrawPayments(owner)))
+            .should.be.bignumber.equal(reward);
+        });
+
+        it('reverts when canceled by anyone', async function () {
+          await assertRevert(this.bounty.cancelBounty({ from: anyone }));
+        });
+      });
+
+      context('after canceling', async function () {
+        beforeEach(async function () {
+          await this.bounty.cancelBounty({ from: owner });
+        });
+
+        it('is not claimable', async function () {
+          (await this.bounty.claimable()).should.equal(false);
+        });
+
+        it('no longer accepts rewards', async function () {
+          await assertRevert(ethSendTransaction({ from: owner, to: this.bounty.address, value: reward }));
+        });
+
+        it('reverts when recanceled', async function () {
+          await assertRevert(this.bounty.cancelBounty({ from: owner }));
+        });
+      });
+    });
   });
 });