diff --git a/contracts/access/roles/CapperRole.sol b/contracts/access/roles/CapperRole.sol index 4cc9f01fc..297c084b1 100644 --- a/contracts/access/roles/CapperRole.sol +++ b/contracts/access/roles/CapperRole.sol @@ -13,7 +13,7 @@ contract CapperRole is Initializable { Roles.Role private cappers; function initialize() public initializer { - cappers.add(msg.sender); + _addCapper(msg.sender); } modifier onlyCapper() { @@ -26,12 +26,16 @@ contract CapperRole is Initializable { } function addCapper(address account) public onlyCapper { - cappers.add(account); - emit CapperAdded(account); + _addCapper(account); } function renounceCapper() public { - cappers.remove(msg.sender); + _removeCapper(msg.sender); + } + + function _addCapper(address account) internal { + cappers.add(account); + emit CapperAdded(account); } function _removeCapper(address account) internal { diff --git a/contracts/access/roles/MinterRole.sol b/contracts/access/roles/MinterRole.sol index bebcd1d79..ddace6862 100644 --- a/contracts/access/roles/MinterRole.sol +++ b/contracts/access/roles/MinterRole.sol @@ -13,7 +13,7 @@ contract MinterRole is Initializable { Roles.Role private minters; function initialize() public initializer { - minters.add(msg.sender); + _addMinter(msg.sender); } modifier onlyMinter() { @@ -26,12 +26,16 @@ contract MinterRole is Initializable { } function addMinter(address account) public onlyMinter { - minters.add(account); - emit MinterAdded(account); + _addMinter(account); } function renounceMinter() public { - minters.remove(msg.sender); + _removeMinter(msg.sender); + } + + function _addMinter(address account) internal { + minters.add(account); + emit MinterAdded(account); } function _removeMinter(address account) internal { diff --git a/contracts/access/roles/PauserRole.sol b/contracts/access/roles/PauserRole.sol index 995416b9c..615bd767d 100644 --- a/contracts/access/roles/PauserRole.sol +++ b/contracts/access/roles/PauserRole.sol @@ -13,7 +13,7 @@ contract PauserRole is Initializable { Roles.Role private pausers; function initialize() public initializer { - pausers.add(msg.sender); + _addPauser(msg.sender); } modifier onlyPauser() { @@ -26,12 +26,16 @@ contract PauserRole is Initializable { } function addPauser(address account) public onlyPauser { - pausers.add(account); - emit PauserAdded(account); + _addPauser(account); } function renouncePauser() public { - pausers.remove(msg.sender); + _removePauser(msg.sender); + } + + function _addPauser(address account) internal { + pausers.add(account); + emit PauserAdded(account); } function _removePauser(address account) internal { diff --git a/contracts/access/roles/SignerRole.sol b/contracts/access/roles/SignerRole.sol index 6fe12f88e..ad74f03c9 100644 --- a/contracts/access/roles/SignerRole.sol +++ b/contracts/access/roles/SignerRole.sol @@ -13,7 +13,7 @@ contract SignerRole is Initializable { Roles.Role private signers; function initialize() public initializer { - signers.add(msg.sender); + _addSigner(msg.sender); } modifier onlySigner() { @@ -26,12 +26,16 @@ contract SignerRole is Initializable { } function addSigner(address account) public onlySigner { - signers.add(account); - emit SignerAdded(account); + _addSigner(account); } function renounceSigner() public { - signers.remove(msg.sender); + _removeSigner(msg.sender); + } + + function _addSigner(address account) internal { + signers.add(account); + emit SignerAdded(account); } function _removeSigner(address account) internal { 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/contracts/payment/Escrow.sol b/contracts/payment/Escrow.sol index d55c31c1c..11aee27a5 100644 --- a/contracts/payment/Escrow.sol +++ b/contracts/payment/Escrow.sol @@ -45,7 +45,6 @@ contract Escrow is Initializable, Secondary { */ function withdraw(address payee) public onlyPrimary { uint256 payment = _deposits[payee]; - assert(address(this).balance >= payment); _deposits[payee] = 0; diff --git a/contracts/payment/SplitPayment.sol b/contracts/payment/SplitPayment.sol index 64344846c..2c72c8072 100644 --- a/contracts/payment/SplitPayment.sol +++ b/contracts/payment/SplitPayment.sol @@ -86,7 +86,6 @@ contract SplitPayment is Initializable { ); require(payment != 0); - assert(address(this).balance >= payment); _released[account] = _released[account].add(payment); _totalReleased = _totalReleased.add(payment); diff --git a/contracts/token/ERC20/ERC20Mintable.sol b/contracts/token/ERC20/ERC20Mintable.sol index 7d6f270a6..f15927e24 100644 --- a/contracts/token/ERC20/ERC20Mintable.sol +++ b/contracts/token/ERC20/ERC20Mintable.sol @@ -10,26 +10,10 @@ import "../../access/roles/MinterRole.sol"; * @dev ERC20 minting logic */ contract ERC20Mintable is Initializable, ERC20, MinterRole { - event MintingFinished(); - - bool private _mintingFinished = false; - - modifier onlyBeforeMintingFinished() { - require(!_mintingFinished); - _; - } - function initialize() public initializer { MinterRole.initialize(); } - /** - * @return true if the minting is finished. - */ - function mintingFinished() public view returns(bool) { - return _mintingFinished; - } - /** * @dev Function to mint tokens * @param to The address that will receive the minted tokens. @@ -42,25 +26,9 @@ contract ERC20Mintable is Initializable, ERC20, MinterRole { ) public onlyMinter - onlyBeforeMintingFinished returns (bool) { _mint(to, amount); return true; } - - /** - * @dev Function to stop minting new tokens. - * @return True if the operation was successful. - */ - function finishMinting() - public - onlyMinter - onlyBeforeMintingFinished - returns (bool) - { - _mintingFinished = true; - emit MintingFinished(); - return true; - } } diff --git a/contracts/token/ERC721/ERC721Mintable.sol b/contracts/token/ERC721/ERC721Mintable.sol index 02e498bd0..331a24f89 100644 --- a/contracts/token/ERC721/ERC721Mintable.sol +++ b/contracts/token/ERC721/ERC721Mintable.sol @@ -10,26 +10,10 @@ import "../../access/roles/MinterRole.sol"; * @dev ERC721 minting logic */ contract ERC721Mintable is Initializable, ERC721Full, MinterRole { - event MintingFinished(); - - bool private _mintingFinished = false; - - modifier onlyBeforeMintingFinished() { - require(!_mintingFinished); - _; - } - function initialize() public initializer { MinterRole.initialize(); } - /** - * @return true if the minting is finished. - */ - function mintingFinished() public view returns(bool) { - return _mintingFinished; - } - /** * @dev Function to mint tokens * @param to The address that will receive the minted tokens. @@ -42,7 +26,6 @@ contract ERC721Mintable is Initializable, ERC721Full, MinterRole { ) public onlyMinter - onlyBeforeMintingFinished returns (bool) { _mint(to, tokenId); @@ -56,26 +39,10 @@ contract ERC721Mintable is Initializable, ERC721Full, MinterRole { ) public onlyMinter - onlyBeforeMintingFinished returns (bool) { mint(to, tokenId); _setTokenURI(tokenId, tokenURI); return true; } - - /** - * @dev Function to stop minting new tokens. - * @return True if the operation was successful. - */ - function finishMinting() - public - onlyMinter - onlyBeforeMintingFinished - returns (bool) - { - _mintingFinished = true; - emit MintingFinished(); - return true; - } } diff --git a/test/access/roles/PublicRole.behavior.js b/test/access/roles/PublicRole.behavior.js index 1e489a468..f1c723b78 100644 --- a/test/access/roles/PublicRole.behavior.js +++ b/test/access/roles/PublicRole.behavior.js @@ -89,6 +89,11 @@ function shouldBehaveLikePublicRole (authorized, otherAuthorized, [anyone], role (await this.contract[`is${rolename}`](authorized)).should.equal(false); }); + it(`emits a ${rolename}Removed event`, async function () { + const { logs } = await this.contract[`renounce${rolename}`]({ from: authorized }); + expectEvent.inLogs(logs, `${rolename}Removed`, { account: authorized }); + }); + it('doesn\'t revert when renouncing unassigned role', async function () { await this.contract[`renounce${rolename}`]({ from: anyone }); }); 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 })); + }); }); }); }); diff --git a/test/token/ERC20/behaviors/ERC20Mintable.behavior.js b/test/token/ERC20/behaviors/ERC20Mintable.behavior.js index 5046fc62e..16126d99a 100644 --- a/test/token/ERC20/behaviors/ERC20Mintable.behavior.js +++ b/test/token/ERC20/behaviors/ERC20Mintable.behavior.js @@ -11,136 +11,44 @@ function shouldBehaveLikeERC20Mintable (minter, [anyone]) { const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000'; describe('as a mintable token', function () { - describe('mintingFinished', function () { - context('when token minting is not finished', function () { - it('returns false', async function () { - (await this.token.mintingFinished()).should.equal(false); - }); - }); - - context('when token minting is finished', function () { - beforeEach(async function () { - await this.token.finishMinting({ from: minter }); - }); - - it('returns true', async function () { - (await this.token.mintingFinished()).should.equal(true); - }); - }); - }); - - describe('finishMinting', function () { - context('when the sender has minting permission', function () { - const from = minter; - - context('when token minting was not finished', function () { - it('finishes token minting', async function () { - await this.token.finishMinting({ from }); - - (await this.token.mintingFinished()).should.equal(true); - }); - - it('emits a mint finished event', async function () { - const { logs } = await this.token.finishMinting({ from }); - - await expectEvent.inLogs(logs, 'MintingFinished'); - }); - }); - - context('when token minting was already finished', function () { - beforeEach(async function () { - await this.token.finishMinting({ from }); - }); - - it('reverts', async function () { - await assertRevert(this.token.finishMinting({ from })); - }); - }); - }); - - context('when the sender doesn\'t have minting permission', function () { - const from = anyone; - - context('when token minting was not finished', function () { - it('reverts', async function () { - await assertRevert(this.token.finishMinting({ from })); - }); - }); - - context('when token minting was already finished', function () { - beforeEach(async function () { - await this.token.finishMinting({ from: minter }); - }); - - it('reverts', async function () { - await assertRevert(this.token.finishMinting({ from })); - }); - }); - }); - }); - describe('mint', function () { const amount = 100; context('when the sender has minting permission', function () { const from = minter; - context('when token minting is not finished', function () { - context('for a zero amount', function () { - shouldMint(0); - }); - - context('for a non-zero amount', function () { - shouldMint(amount); - }); - - function shouldMint (amount) { - beforeEach(async function () { - ({ logs: this.logs } = await this.token.mint(anyone, amount, { from })); - }); - - it('mints the requested amount', async function () { - (await this.token.balanceOf(anyone)).should.be.bignumber.equal(amount); - }); - - it('emits a mint and a transfer event', async function () { - const transferEvent = expectEvent.inLogs(this.logs, 'Transfer', { - from: ZERO_ADDRESS, - to: anyone, - }); - transferEvent.args.value.should.be.bignumber.equal(amount); - }); - } + context('for a zero amount', function () { + shouldMint(0); }); - context('when token minting is finished', function () { + context('for a non-zero amount', function () { + shouldMint(amount); + }); + + function shouldMint (amount) { beforeEach(async function () { - await this.token.finishMinting({ from: minter }); + ({ logs: this.logs } = await this.token.mint(anyone, amount, { from })); }); - it('reverts', async function () { - await assertRevert(this.token.mint(anyone, amount, { from })); + it('mints the requested amount', async function () { + (await this.token.balanceOf(anyone)).should.be.bignumber.equal(amount); }); - }); + + it('emits a mint and a transfer event', async function () { + const transferEvent = expectEvent.inLogs(this.logs, 'Transfer', { + from: ZERO_ADDRESS, + to: anyone, + }); + transferEvent.args.value.should.be.bignumber.equal(amount); + }); + } }); context('when the sender doesn\'t have minting permission', function () { const from = anyone; - context('when token minting is not finished', function () { - it('reverts', async function () { - await assertRevert(this.token.mint(anyone, amount, { from })); - }); - }); - - context('when token minting is already finished', function () { - beforeEach(async function () { - await this.token.finishMinting({ from: minter }); - }); - - it('reverts', async function () { - await assertRevert(this.token.mint(anyone, amount, { from })); - }); + it('reverts', async function () { + await assertRevert(this.token.mint(anyone, amount, { from })); }); }); }); diff --git a/test/token/ERC721/ERC721Holder.test.js b/test/token/ERC721/ERC721Holder.test.js new file mode 100644 index 000000000..9f6eb856a --- /dev/null +++ b/test/token/ERC721/ERC721Holder.test.js @@ -0,0 +1,19 @@ +const ERC721Holder = artifacts.require('ERC721Holder.sol'); +const ERC721Mintable = artifacts.require('ERC721MintableBurnableImpl.sol'); + +require('chai') + .should(); + +contract('ERC721Holder', function ([creator]) { + it('receives an ERC721 token', async function () { + const token = await ERC721Mintable.new({ from: creator }); + const tokenId = 1; + await token.mint(creator, tokenId, { from: creator }); + + const receiver = await ERC721Holder.new(); + await token.approve(receiver.address, tokenId, { from: creator }); + await token.safeTransferFrom(creator, receiver.address, tokenId); + + (await token.ownerOf(tokenId)).should.be.equal(receiver.address); + }); +}); diff --git a/test/token/ERC721/ERC721MintBurn.behavior.js b/test/token/ERC721/ERC721MintBurn.behavior.js index cadfe87cc..31db1d176 100644 --- a/test/token/ERC721/ERC721MintBurn.behavior.js +++ b/test/token/ERC721/ERC721MintBurn.behavior.js @@ -117,35 +117,6 @@ function shouldBehaveLikeMintAndBurnERC721 ( }); }); }); - - describe('finishMinting', function () { - it('allows the minter to finish minting', async function () { - const { logs } = await this.token.finishMinting({ from: minter }); - expectEvent.inLogs(logs, 'MintingFinished'); - }); - }); - - context('mintingFinished', function () { - beforeEach(async function () { - await this.token.finishMinting({ from: minter }); - }); - - describe('mint', function () { - it('reverts', async function () { - await assertRevert( - this.token.mint(owner, thirdTokenId, { from: minter }) - ); - }); - }); - - describe('mintWithTokenURI', function () { - it('reverts', async function () { - await assertRevert( - this.token.mintWithTokenURI(owner, thirdTokenId, MOCK_URI, { from: minter }) - ); - }); - }); - }); }); }