From 41f84f8b402efcda35f84ea9d7ffca4bb5f86499 Mon Sep 17 00:00:00 2001
From: Aniket <30843294+Aniket-Engg@users.noreply.github.com>
Date: Mon, 8 Oct 2018 19:21:06 +0530
Subject: [PATCH] Removed selfdestruct from BreakInvariantBounty (#1385)

* signing prefix added

* Minor improvement

* Tests changed

* Successfully tested

* Minor improvements

* Minor improvements

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

This reverts commit a6889776f46adca374b6ebf014aa7b0038112a9d.

* updates

* fixes #1384

* introduced claimable and cancelBounty

* cancelBounty tests

* Update BreakInvariantBounty.test.js
---
 contracts/bounties/BreakInvariantBounty.sol | 26 +++++----
 test/BreakInvariantBounty.test.js           | 63 ++++++++++++++-------
 2 files changed, 58 insertions(+), 31 deletions(-)

diff --git a/contracts/bounties/BreakInvariantBounty.sol b/contracts/bounties/BreakInvariantBounty.sol
index 6fd3b6e67..52077a539 100644
--- a/contracts/bounties/BreakInvariantBounty.sol
+++ b/contracts/bounties/BreakInvariantBounty.sol
@@ -8,24 +8,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;
   }
 
   /**
@@ -45,20 +46,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();
   }
 
   /**
diff --git a/test/BreakInvariantBounty.test.js b/test/BreakInvariantBounty.test.js
index 50ae96aaa..5995f64f5 100644
--- a/test/BreakInvariantBounty.test.js
+++ b/test/BreakInvariantBounty.test.js
@@ -1,4 +1,5 @@
 const { ethGetBalance, ethSendTransaction } = require('./helpers/web3');
+const { ether } = require('./helpers/ether');
 const { sendEther } = require('./helpers/sendTransaction');
 const { balanceDifference } = require('./helpers/balanceDiff');
 const expectEvent = require('./helpers/expectEvent');
@@ -11,7 +12,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 () {
@@ -28,24 +29,9 @@ contract('BreakInvariantBounty', function ([_, owner, researcher, anyone, nonTar
       await sendEther(owner, this.bounty.address, 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 () {
@@ -83,8 +69,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 () {
@@ -104,5 +90,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 }));
+        });
+      });
+    });
   });
 });