Improved bounty tests. (#1350)

* Improved bounty tests.

* Fixed linter errors.

* Addressed review comments.

contracts/mocks/SecureInvariantTargetBounty.sol → contracts/mocks/BreakInvariantBountyMock.sol

@@ -5,14 +5,24 @@ pragma solidity ^0.4.24;
 // solium-disable-next-line max-len
 import {BreakInvariantBounty, Target} from "../bounties/BreakInvariantBounty.sol";
 
-contract SecureInvariantTargetMock is Target {
-  function checkInvariant() public returns(bool) {
+contract TargetMock is Target {
+  bool private exploited;
+
+  function exploitVulnerability() public {
+    exploited = true;
+  }
+
+  function checkInvariant() public returns (bool) {
+    if (exploited) {
+      return false;
+    }
+
     return true;
   }
 }
 
-contract SecureInvariantTargetBounty is BreakInvariantBounty {
+contract BreakInvariantBountyMock is BreakInvariantBounty {
   function _deployContract() internal returns (address) {
-    return new SecureInvariantTargetMock();
+    return new TargetMock();
   }
 }

contracts/mocks/InsecureInvariantTargetBounty.sol

@@ -1,18 +0,0 @@
-pragma solidity ^0.4.24;
-
-// When this line is split, truffle parsing fails.
-// See: https://github.com/ethereum/solidity/issues/4871
-// solium-disable-next-line max-len
-import {BreakInvariantBounty, Target} from "../bounties/BreakInvariantBounty.sol";
-
-contract InsecureInvariantTargetMock is Target {
-  function checkInvariant() public returns(bool) {
-    return false;
-  }
-}
-
-contract InsecureInvariantTargetBounty is BreakInvariantBounty {
-  function _deployContract() internal returns (address) {
-    return new InsecureInvariantTargetMock();
-  }
-}

test/BreakInvariantBounty.test.js

@@ -2,97 +2,104 @@ const { ethGetBalance, ethSendTransaction } = require('./helpers/web3');
 const expectEvent = require('./helpers/expectEvent');
 const { assertRevert } = require('./helpers/assertRevert');
 
-const SecureInvariantTargetBounty = artifacts.require('SecureInvariantTargetBounty');
-const InsecureInvariantTargetBounty = artifacts.require('InsecureInvariantTargetBounty');
+const BreakInvariantBountyMock = artifacts.require('BreakInvariantBountyMock');
+const TargetMock = artifacts.require('TargetMock');
 
 require('chai')
   .use(require('chai-bignumber')(web3.BigNumber))
   .should();
 
-const sendReward = async (from, to, value) => ethSendTransaction({
-  from,
-  to,
-  value,
-});
-
 const reward = new web3.BigNumber(web3.toWei(1, 'ether'));
 
-contract('BreakInvariantBounty', function ([_, owner, researcher, nonTarget]) {
-  context('against secure contract', function () {
-    beforeEach(async function () {
-      this.bounty = await SecureInvariantTargetBounty.new({ from: owner });
-    });
+contract('BreakInvariantBounty', function ([_, owner, researcher, anyone, nonTarget]) {
+  beforeEach(async function () {
+    this.bounty = await BreakInvariantBountyMock.new({ from: owner });
+  });
 
-    it('can set reward', async function () {
-      await sendReward(owner, this.bounty.address, reward);
+  it('can set reward', async function () {
+    await ethSendTransaction({ from: owner, to: this.bounty.address, value: reward });
+    (await ethGetBalance(this.bounty.address)).should.be.bignumber.equal(reward);
+  });
 
-      const balance = await ethGetBalance(this.bounty.address);
-      balance.should.be.bignumber.equal(reward);
+  context('with reward', function () {
+    beforeEach(async function () {
+      await ethSendTransaction({ from: owner, to: this.bounty.address, value: reward });
     });
 
-    context('with reward', function () {
-      beforeEach(async function () {
-        const result = await this.bounty.createTarget({ from: researcher });
-        const event = expectEvent.inLogs(result.logs, 'TargetCreated');
-
-        this.targetAddress = event.args.createdAddress;
-
-        await sendReward(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);
 
-        const balance = await ethGetBalance(this.bounty.address);
-        balance.should.be.bignumber.equal(reward);
+        (await ethGetBalance(this.bounty.address)).should.be.bignumber.equal(0);
+        ownerPostBalance.sub(ownerPreBalance).should.be.bignumber.equal(reward);
       });
 
-      it('cannot claim reward', async function () {
-        await assertRevert(
-          this.bounty.claim(this.targetAddress, { from: researcher }),
-        );
+      it('reverts when called by anyone', async function () {
+        await assertRevert(this.bounty.destroy({ from: anyone }));
       });
     });
-  });
-
-  context('against broken contract', function () {
-    beforeEach(async function () {
-      this.bounty = await InsecureInvariantTargetBounty.new();
 
-      const result = await this.bounty.createTarget({ from: researcher });
-      const event = expectEvent.inLogs(result.logs, 'TargetCreated');
-
-      this.targetAddress = event.args.createdAddress;
-      await sendReward(owner, this.bounty.address, reward);
-    });
-
-    it('can claim reward', async function () {
-      await this.bounty.claim(this.targetAddress, { from: researcher });
-      const claim = await this.bounty.claimed();
-
-      claim.should.equal(true);
-
-      const researcherPrevBalance = await ethGetBalance(researcher);
-
-      await this.bounty.withdrawPayments(researcher, { gasPrice: 0 });
-      const updatedBalance = await ethGetBalance(this.bounty.address);
-      updatedBalance.should.be.bignumber.equal(0);
-
-      const researcherCurrBalance = await ethGetBalance(researcher);
-      researcherCurrBalance.sub(researcherPrevBalance).should.be.bignumber.equal(reward);
-    });
+    describe('claim', function () {
+      it('is initially unclaimed', async function () {
+        (await this.bounty.claimed()).should.equal(false);
+      });
 
-    it('cannot claim reward from non-target', async function () {
-      await assertRevert(
-        this.bounty.claim(nonTarget, { from: researcher })
-      );
-    });
+      it('can create claimable target', async function () {
+        const { logs } = await this.bounty.createTarget({ from: researcher });
+        expectEvent.inLogs(logs, 'TargetCreated');
+      });
 
-    context('reward claimed', function () {
-      beforeEach(async function () {
-        await this.bounty.claim(this.targetAddress, { from: researcher });
+      context('with target', async function () {
+        beforeEach(async function () {
+          const { logs } = await this.bounty.createTarget({ from: researcher });
+          const event = expectEvent.inLogs(logs, 'TargetCreated');
+          this.target = TargetMock.at(event.args.createdAddress);
+        });
+
+        context('before exploiting vulnerability', async function () {
+          it('reverts when claiming reward', async function () {
+            await assertRevert(this.bounty.claim(this.target.address, { from: researcher }));
+          });
+        });
+
+        context('after exploiting vulnerability', async function () {
+          beforeEach(async function () {
+            await this.target.exploitVulnerability({ from: researcher });
+          });
+
+          it('sends the reward to the researcher', async function () {
+            await this.bounty.claim(this.target.address, { from: anyone });
+
+            const researcherPreBalance = await ethGetBalance(researcher);
+            await this.bounty.withdrawPayments(researcher);
+            const researcherPostBalance = await ethGetBalance(researcher);
+
+            researcherPostBalance.sub(researcherPreBalance).should.be.bignumber.equal(reward);
+            (await ethGetBalance(this.bounty.address)).should.be.bignumber.equal(0);
+          });
+
+          context('after claiming', async function () {
+            beforeEach(async function () {
+              await this.bounty.claim(this.target.address, { from: researcher });
+            });
+
+            it('is claimed', async function () {
+              (await this.bounty.claimed()).should.equal(true);
+            });
+
+            it('no longer accepts rewards', async function () {
+              await assertRevert(ethSendTransaction({ from: owner, to: this.bounty.address, value: reward }));
+            });
+          });
+        });
       });
 
-      it('should no longer be payable', async function () {
-        await assertRevert(
-          sendReward(owner, this.bounty.address, reward)
-        );
+      context('with non-target', function () {
+        it('reverts when claiming reward', async function () {
+          await assertRevert(this.bounty.claim(nonTarget, { from: researcher }));
+        });
       });
     });
   });