From 2441fd7d17bffa1944f6f539b2cddd6d19997a31 Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Mon, 3 Sep 2018 07:53:28 -0600 Subject: [PATCH] Move contracts to subdirectories (#1253) * Move contracts to subdirectories Fixes #1177. This Change also removes the LimitBalance contract. * fix import * move MerkleProof to cryptography * Fix import --- contracts/LimitBalance.sol | 31 ---------- contracts/access/SignatureBouncer.sol | 4 +- .../BreakInvariantBounty.sol} | 8 +-- .../ECDSA.sol} | 2 +- contracts/{ => cryptography}/MerkleProof.sol | 0 contracts/mocks/AutoIncrementingImpl.sol | 2 +- .../{ECRecoveryMock.sol => ECDSAMock.sol} | 6 +- .../mocks/InsecureInvariantTargetBounty.sol | 20 +++++++ contracts/mocks/InsecureTargetBounty.sol | 17 ------ contracts/mocks/LimitBalanceMock.sol | 13 ---- contracts/mocks/MerkleProofWrapper.sol | 2 +- contracts/mocks/ReentrancyMock.sol | 2 +- .../mocks/SecureInvariantTargetBounty.sol | 20 +++++++ contracts/mocks/SecureTargetBounty.sol | 17 ------ contracts/token/ERC721/ERC721Basic.sol | 4 +- .../{AddressUtils.sol => utils/Address.sol} | 2 +- contracts/{ => utils}/AutoIncrementing.sol | 0 contracts/{ => utils}/ReentrancyGuard.sol | 0 ...y.test.js => BreakInvariantBounty.test.js} | 10 ++-- test/LimitBalance.test.js | 59 ------------------- .../{ECRecovery.test.js => ECDSA.test.js} | 6 +- 21 files changed, 64 insertions(+), 161 deletions(-) delete mode 100644 contracts/LimitBalance.sol rename contracts/{Bounty.sol => bounties/BreakInvariantBounty.sol} (91%) rename contracts/{ECRecovery.sol => cryptography/ECDSA.sol} (98%) rename contracts/{ => cryptography}/MerkleProof.sol (100%) rename contracts/mocks/{ECRecoveryMock.sol => ECDSAMock.sol} (78%) create mode 100644 contracts/mocks/InsecureInvariantTargetBounty.sol delete mode 100644 contracts/mocks/InsecureTargetBounty.sol delete mode 100644 contracts/mocks/LimitBalanceMock.sol create mode 100644 contracts/mocks/SecureInvariantTargetBounty.sol delete mode 100644 contracts/mocks/SecureTargetBounty.sol rename contracts/{AddressUtils.sol => utils/Address.sol} (97%) rename contracts/{ => utils}/AutoIncrementing.sol (100%) rename contracts/{ => utils}/ReentrancyGuard.sol (100%) rename test/{Bounty.test.js => BreakInvariantBounty.test.js} (88%) delete mode 100644 test/LimitBalance.test.js rename test/library/{ECRecovery.test.js => ECDSA.test.js} (94%) diff --git a/contracts/LimitBalance.sol b/contracts/LimitBalance.sol deleted file mode 100644 index c3d495a3c72..00000000000 --- a/contracts/LimitBalance.sol +++ /dev/null @@ -1,31 +0,0 @@ -pragma solidity ^0.4.24; - - -/** - * @title LimitBalance - * @dev Simple contract to limit the balance of child contract. - * Note this doesn't prevent other contracts to send funds by using selfdestruct(address); - * See: https://github.com/ConsenSys/smart-contract-best-practices#remember-that-ether-can-be-forcibly-sent-to-an-account - */ -contract LimitBalance { - - uint256 public limit; - - /** - * @dev Constructor that sets the passed value as a limit. - * @param _limit uint256 to represent the limit. - */ - constructor(uint256 _limit) public { - limit = _limit; - } - - /** - * @dev Checks if limit was reached. Case true, it throws. - */ - modifier limitedPayable() { - require(address(this).balance <= limit); - _; - - } - -} diff --git a/contracts/access/SignatureBouncer.sol b/contracts/access/SignatureBouncer.sol index 9a6092ea6d2..3dc373afbc9 100644 --- a/contracts/access/SignatureBouncer.sol +++ b/contracts/access/SignatureBouncer.sol @@ -2,7 +2,7 @@ pragma solidity ^0.4.24; import "../ownership/Ownable.sol"; import "../access/rbac/RBAC.sol"; -import "../ECRecovery.sol"; +import "../cryptography/ECDSA.sol"; /** @@ -30,7 +30,7 @@ import "../ECRecovery.sol"; * much more complex. See https://ethereum.stackexchange.com/a/50616 for more details. */ contract SignatureBouncer is Ownable, RBAC { - using ECRecovery for bytes32; + using ECDSA for bytes32; string public constant ROLE_BOUNCER = "bouncer"; uint internal constant METHOD_ID_SIZE = 4; diff --git a/contracts/Bounty.sol b/contracts/bounties/BreakInvariantBounty.sol similarity index 91% rename from contracts/Bounty.sol rename to contracts/bounties/BreakInvariantBounty.sol index e34d6d118d8..1677fb5abb2 100644 --- a/contracts/Bounty.sol +++ b/contracts/bounties/BreakInvariantBounty.sol @@ -1,15 +1,15 @@ pragma solidity ^0.4.24; -import "./payment/PullPayment.sol"; -import "./lifecycle/Destructible.sol"; +import "../payment/PullPayment.sol"; +import "../lifecycle/Destructible.sol"; /** - * @title Bounty + * @title BreakInvariantBounty * @dev This bounty will pay out to a researcher if they break invariant logic of the contract. */ -contract Bounty is PullPayment, Destructible { +contract BreakInvariantBounty is PullPayment, Destructible { bool public claimed; mapping(address => address) public researchers; diff --git a/contracts/ECRecovery.sol b/contracts/cryptography/ECDSA.sol similarity index 98% rename from contracts/ECRecovery.sol rename to contracts/cryptography/ECDSA.sol index 66c49450746..fc14498bc4c 100644 --- a/contracts/ECRecovery.sol +++ b/contracts/cryptography/ECDSA.sol @@ -8,7 +8,7 @@ pragma solidity ^0.4.24; * See https://github.com/ethereum/solidity/issues/864 */ -library ECRecovery { +library ECDSA { /** * @dev Recover signer address from a message by using their signature diff --git a/contracts/MerkleProof.sol b/contracts/cryptography/MerkleProof.sol similarity index 100% rename from contracts/MerkleProof.sol rename to contracts/cryptography/MerkleProof.sol diff --git a/contracts/mocks/AutoIncrementingImpl.sol b/contracts/mocks/AutoIncrementingImpl.sol index 9aae333f0c5..86c498f6954 100644 --- a/contracts/mocks/AutoIncrementingImpl.sol +++ b/contracts/mocks/AutoIncrementingImpl.sol @@ -1,6 +1,6 @@ pragma solidity ^0.4.24; -import "../AutoIncrementing.sol"; +import "../utils/AutoIncrementing.sol"; contract AutoIncrementingImpl { diff --git a/contracts/mocks/ECRecoveryMock.sol b/contracts/mocks/ECDSAMock.sol similarity index 78% rename from contracts/mocks/ECRecoveryMock.sol rename to contracts/mocks/ECDSAMock.sol index a2c5ebcd868..42570569e0e 100644 --- a/contracts/mocks/ECRecoveryMock.sol +++ b/contracts/mocks/ECDSAMock.sol @@ -1,11 +1,11 @@ pragma solidity ^0.4.24; -import "../ECRecovery.sol"; +import "../cryptography/ECDSA.sol"; -contract ECRecoveryMock { - using ECRecovery for bytes32; +contract ECDSAMock { + using ECDSA for bytes32; function recover(bytes32 _hash, bytes _signature) public diff --git a/contracts/mocks/InsecureInvariantTargetBounty.sol b/contracts/mocks/InsecureInvariantTargetBounty.sol new file mode 100644 index 00000000000..26ceb23b926 --- /dev/null +++ b/contracts/mocks/InsecureInvariantTargetBounty.sol @@ -0,0 +1,20 @@ +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 "../../contracts/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(); + } +} diff --git a/contracts/mocks/InsecureTargetBounty.sol b/contracts/mocks/InsecureTargetBounty.sol deleted file mode 100644 index 447fba58402..00000000000 --- a/contracts/mocks/InsecureTargetBounty.sol +++ /dev/null @@ -1,17 +0,0 @@ -pragma solidity ^0.4.24; - -import {Bounty, Target} from "../../contracts/Bounty.sol"; - - -contract InsecureTargetMock is Target { - function checkInvariant() public returns(bool) { - return false; - } -} - - -contract InsecureTargetBounty is Bounty { - function deployContract() internal returns (address) { - return new InsecureTargetMock(); - } -} diff --git a/contracts/mocks/LimitBalanceMock.sol b/contracts/mocks/LimitBalanceMock.sol deleted file mode 100644 index bc28d1ab6fb..00000000000 --- a/contracts/mocks/LimitBalanceMock.sol +++ /dev/null @@ -1,13 +0,0 @@ -pragma solidity ^0.4.24; - - -import "../LimitBalance.sol"; - - -// mock class using LimitBalance -contract LimitBalanceMock is LimitBalance(1000) { - - function limitedDeposit() public payable limitedPayable { - } - -} diff --git a/contracts/mocks/MerkleProofWrapper.sol b/contracts/mocks/MerkleProofWrapper.sol index bf963dea3c9..fe72d75f87f 100644 --- a/contracts/mocks/MerkleProofWrapper.sol +++ b/contracts/mocks/MerkleProofWrapper.sol @@ -1,6 +1,6 @@ pragma solidity ^0.4.24; -import { MerkleProof } from "../MerkleProof.sol"; +import { MerkleProof } from "../cryptography/MerkleProof.sol"; contract MerkleProofWrapper { diff --git a/contracts/mocks/ReentrancyMock.sol b/contracts/mocks/ReentrancyMock.sol index 670190558b3..93afdd6dc28 100644 --- a/contracts/mocks/ReentrancyMock.sol +++ b/contracts/mocks/ReentrancyMock.sol @@ -1,6 +1,6 @@ pragma solidity ^0.4.24; -import "../ReentrancyGuard.sol"; +import "../utils/ReentrancyGuard.sol"; import "./ReentrancyAttack.sol"; diff --git a/contracts/mocks/SecureInvariantTargetBounty.sol b/contracts/mocks/SecureInvariantTargetBounty.sol new file mode 100644 index 00000000000..b44dab43c35 --- /dev/null +++ b/contracts/mocks/SecureInvariantTargetBounty.sol @@ -0,0 +1,20 @@ +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 "../../contracts/bounties/BreakInvariantBounty.sol"; + + +contract SecureInvariantTargetMock is Target { + function checkInvariant() public returns(bool) { + return true; + } +} + + +contract SecureInvariantTargetBounty is BreakInvariantBounty { + function deployContract() internal returns (address) { + return new SecureInvariantTargetMock(); + } +} diff --git a/contracts/mocks/SecureTargetBounty.sol b/contracts/mocks/SecureTargetBounty.sol deleted file mode 100644 index 450e48532ac..00000000000 --- a/contracts/mocks/SecureTargetBounty.sol +++ /dev/null @@ -1,17 +0,0 @@ -pragma solidity ^0.4.24; - -import {Bounty, Target} from "../../contracts/Bounty.sol"; - - -contract SecureTargetMock is Target { - function checkInvariant() public returns(bool) { - return true; - } -} - - -contract SecureTargetBounty is Bounty { - function deployContract() internal returns (address) { - return new SecureTargetMock(); - } -} diff --git a/contracts/token/ERC721/ERC721Basic.sol b/contracts/token/ERC721/ERC721Basic.sol index 58a40cae893..65b748b4050 100644 --- a/contracts/token/ERC721/ERC721Basic.sol +++ b/contracts/token/ERC721/ERC721Basic.sol @@ -3,7 +3,7 @@ pragma solidity ^0.4.24; import "./IERC721Basic.sol"; import "./IERC721Receiver.sol"; import "../../math/SafeMath.sol"; -import "../../AddressUtils.sol"; +import "../../utils/Address.sol"; import "../../introspection/SupportsInterfaceWithLookup.sol"; @@ -14,7 +14,7 @@ import "../../introspection/SupportsInterfaceWithLookup.sol"; contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic { using SafeMath for uint256; - using AddressUtils for address; + using Address for address; // Equals to `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))` // which can be also obtained as `IERC721Receiver(0).onERC721Received.selector` diff --git a/contracts/AddressUtils.sol b/contracts/utils/Address.sol similarity index 97% rename from contracts/AddressUtils.sol rename to contracts/utils/Address.sol index b07447d389f..7c972200518 100644 --- a/contracts/AddressUtils.sol +++ b/contracts/utils/Address.sol @@ -4,7 +4,7 @@ pragma solidity ^0.4.24; /** * Utility library of inline functions on addresses */ -library AddressUtils { +library Address { /** * Returns whether the target address is a contract diff --git a/contracts/AutoIncrementing.sol b/contracts/utils/AutoIncrementing.sol similarity index 100% rename from contracts/AutoIncrementing.sol rename to contracts/utils/AutoIncrementing.sol diff --git a/contracts/ReentrancyGuard.sol b/contracts/utils/ReentrancyGuard.sol similarity index 100% rename from contracts/ReentrancyGuard.sol rename to contracts/utils/ReentrancyGuard.sol diff --git a/test/Bounty.test.js b/test/BreakInvariantBounty.test.js similarity index 88% rename from test/Bounty.test.js rename to test/BreakInvariantBounty.test.js index 0f1aa42126d..1f0e36aa9e8 100644 --- a/test/Bounty.test.js +++ b/test/BreakInvariantBounty.test.js @@ -2,8 +2,8 @@ const { ethGetBalance, ethSendTransaction } = require('./helpers/web3'); const expectEvent = require('./helpers/expectEvent'); const { assertRevert } = require('./helpers/assertRevert'); -const SecureTargetBounty = artifacts.require('SecureTargetBounty'); -const InsecureTargetBounty = artifacts.require('InsecureTargetBounty'); +const SecureInvariantTargetBounty = artifacts.require('SecureInvariantTargetBounty'); +const InsecureInvariantTargetBounty = artifacts.require('InsecureInvariantTargetBounty'); require('chai') .use(require('chai-bignumber')(web3.BigNumber)) @@ -17,10 +17,10 @@ const sendReward = async (from, to, value) => ethSendTransaction({ const reward = new web3.BigNumber(web3.toWei(1, 'ether')); -contract('Bounty', function ([_, owner, researcher, nonTarget]) { +contract('BreakInvariantBounty', function ([_, owner, researcher, nonTarget]) { context('against secure contract', function () { beforeEach(async function () { - this.bounty = await SecureTargetBounty.new({ from: owner }); + this.bounty = await SecureInvariantTargetBounty.new({ from: owner }); }); it('can set reward', async function () { @@ -53,7 +53,7 @@ contract('Bounty', function ([_, owner, researcher, nonTarget]) { context('against broken contract', function () { beforeEach(async function () { - this.bounty = await InsecureTargetBounty.new(); + this.bounty = await InsecureInvariantTargetBounty.new(); const result = await this.bounty.createTarget({ from: researcher }); const event = expectEvent.inLogs(result.logs, 'TargetCreated'); diff --git a/test/LimitBalance.test.js b/test/LimitBalance.test.js deleted file mode 100644 index f096916a9f8..00000000000 --- a/test/LimitBalance.test.js +++ /dev/null @@ -1,59 +0,0 @@ -const { assertRevert } = require('./helpers/assertRevert'); -const { ethGetBalance } = require('./helpers/web3'); - -const LimitBalanceMock = artifacts.require('LimitBalanceMock'); - -const BigNumber = web3.BigNumber; - -require('chai') - .use(require('chai-bignumber')(BigNumber)) - .should(); - -contract('LimitBalance', function () { - let limitBalance; - - beforeEach(async function () { - limitBalance = await LimitBalanceMock.new(); - }); - - const LIMIT = 1000; - - it('should expose limit', async function () { - const limit = await limitBalance.limit(); - limit.should.be.bignumber.equal(LIMIT); - }); - - it('should allow sending below limit', async function () { - const amount = 1; - await limitBalance.limitedDeposit({ value: amount }); - - const balance = await ethGetBalance(limitBalance.address); - balance.should.be.bignumber.equal(amount); - }); - - it('shouldnt allow sending above limit', async function () { - const amount = 1110; - await assertRevert(limitBalance.limitedDeposit({ value: amount })); - }); - - it('should allow multiple sends below limit', async function () { - const amount = 500; - await limitBalance.limitedDeposit({ value: amount }); - - const balance = await ethGetBalance(limitBalance.address); - balance.should.be.bignumber.equal(amount); - - await limitBalance.limitedDeposit({ value: amount }); - const updatedBalance = await ethGetBalance(limitBalance.address); - updatedBalance.should.be.bignumber.equal(amount * 2); - }); - - it('shouldnt allow multiple sends above limit', async function () { - const amount = 500; - await limitBalance.limitedDeposit({ value: amount }); - - const balance = await ethGetBalance(limitBalance.address); - balance.should.be.bignumber.equal(amount); - await assertRevert(limitBalance.limitedDeposit({ value: amount + 1 })); - }); -}); diff --git a/test/library/ECRecovery.test.js b/test/library/ECDSA.test.js similarity index 94% rename from test/library/ECRecovery.test.js rename to test/library/ECDSA.test.js index 35be4437b45..7fe7056538a 100644 --- a/test/library/ECRecovery.test.js +++ b/test/library/ECDSA.test.js @@ -1,7 +1,7 @@ const { signMessage, toEthSignedMessageHash } = require('../helpers/sign'); const { expectThrow } = require('../helpers/expectThrow'); -const ECRecoveryMock = artifacts.require('ECRecoveryMock'); +const ECDSAMock = artifacts.require('ECDSAMock'); require('chai') .should(); @@ -9,9 +9,9 @@ require('chai') const TEST_MESSAGE = web3.sha3('OpenZeppelin'); const WRONG_MESSAGE = web3.sha3('Nope'); -contract('ECRecovery', function ([_, anyone]) { +contract('ECDSA', function ([_, anyone]) { beforeEach(async function () { - this.mock = await ECRecoveryMock.new(); + this.mock = await ECDSAMock.new(); }); it('recover v0', async function () {