From fa5ecd03cb9b75aa62b5d930962332203de0c8c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 26 Sep 2018 16:32:50 -0300 Subject: [PATCH] Improved bounty tests. (#1350) * Improved bounty tests. * Fixed linter errors. * Addressed review comments. (cherry picked from commit ae109f69ccf4b7264d8ca3dc0d2c9b658744bc0e) --- ...ounty.sol => BreakInvariantBountyMock.sol} | 19 ++- .../mocks/InsecureInvariantTargetBounty.sol | 20 --- test/drafts/BreakInvariantBounty.test.js | 159 +++++++++--------- 3 files changed, 97 insertions(+), 101 deletions(-) rename contracts/mocks/{SecureInvariantTargetBounty.sol => BreakInvariantBountyMock.sol} (51%) delete mode 100644 contracts/mocks/InsecureInvariantTargetBounty.sol diff --git a/contracts/mocks/SecureInvariantTargetBounty.sol b/contracts/mocks/BreakInvariantBountyMock.sol similarity index 51% rename from contracts/mocks/SecureInvariantTargetBounty.sol rename to contracts/mocks/BreakInvariantBountyMock.sol index c23dc86b9..af3e60c57 100644 --- a/contracts/mocks/SecureInvariantTargetBounty.sol +++ b/contracts/mocks/BreakInvariantBountyMock.sol @@ -6,15 +6,24 @@ pragma solidity ^0.4.24; import {BreakInvariantBounty, Target} from "../drafts/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(); } } diff --git a/contracts/mocks/InsecureInvariantTargetBounty.sol b/contracts/mocks/InsecureInvariantTargetBounty.sol deleted file mode 100644 index 1bcbda34f..000000000 --- a/contracts/mocks/InsecureInvariantTargetBounty.sol +++ /dev/null @@ -1,20 +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 "../drafts/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(); - } -} diff --git a/test/drafts/BreakInvariantBounty.test.js b/test/drafts/BreakInvariantBounty.test.js index 747db7e85..d80bca92a 100644 --- a/test/drafts/BreakInvariantBounty.test.js +++ b/test/drafts/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 }); - }); - - it('can set reward', async function () { - await sendReward(owner, this.bounty.address, reward); - - const balance = await ethGetBalance(this.bounty.address); - balance.should.be.bignumber.equal(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); - - const balance = await ethGetBalance(this.bounty.address); - balance.should.be.bignumber.equal(reward); - }); - - it('cannot claim reward', async function () { - await assertRevert( - this.bounty.claim(this.targetAddress, { from: researcher }), - ); - }); - }); +contract('BreakInvariantBounty', function ([_, owner, researcher, anyone, nonTarget]) { + beforeEach(async function () { + this.bounty = await BreakInvariantBountyMock.new({ from: owner }); }); - context('against broken contract', function () { + 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); + }); + + context('with reward', 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); + await ethSendTransaction({ from: owner, to: this.bounty.address, value: reward }); }); - it('can claim reward', async function () { - await this.bounty.claim(this.targetAddress, { from: researcher }); - const claim = await this.bounty.claimed(); + 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); - 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); - }); - - it('cannot claim reward from non-target', async function () { - await assertRevert( - this.bounty.claim(nonTarget, { from: researcher }) - ); - }); - - context('reward claimed', function () { - beforeEach(async function () { - await this.bounty.claim(this.targetAddress, { from: researcher }); + (await ethGetBalance(this.bounty.address)).should.be.bignumber.equal(0); + ownerPostBalance.sub(ownerPreBalance).should.be.bignumber.equal(reward); }); - it('should no longer be payable', async function () { - await assertRevert( - sendReward(owner, this.bounty.address, 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('can create claimable target', async function () { + const { logs } = await this.bounty.createTarget({ from: researcher }); + expectEvent.inLogs(logs, 'TargetCreated'); + }); + + 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 })); + }); + }); + }); + }); + + context('with non-target', function () { + it('reverts when claiming reward', async function () { + await assertRevert(this.bounty.claim(nonTarget, { from: researcher })); + }); }); }); });