From 525a5522b0699fd7ec21e4b2d6dc90ba3854ec24 Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Tue, 14 Mar 2017 14:09:59 +0000 Subject: [PATCH 01/49] Add TokenKillable --- contracts/lifecycle/TokenKillable.sol | 30 +++++++++++++++++++++++++ test/TokenKillable.js | 32 +++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) create mode 100644 contracts/lifecycle/TokenKillable.sol create mode 100644 test/TokenKillable.js diff --git a/contracts/lifecycle/TokenKillable.sol b/contracts/lifecycle/TokenKillable.sol new file mode 100644 index 000000000..e0dfbfe71 --- /dev/null +++ b/contracts/lifecycle/TokenKillable.sol @@ -0,0 +1,30 @@ +pragma solidity ^0.4.8; + + +import "../ownership/Ownable.sol"; +import "../token/ERC20Basic.sol"; + +/// @title TokenKillable: +/// @author Remco Bloemen +///.Base contract that can be killed by owner. All funds in contract including +/// listed tokens will be sent to the owner +contract TokenKillable is Ownable { + + /// @notice Terminate contract and refund to owner + /// @param tokens List of addresses of ERC20 or ERC20Basic token contracts to + // refund + /// @notice The called token contracts could try to re-enter this contract. + // Only supply token contracts you + function kill(address[] tokens) onlyOwner { + + // Transfer tokens to owner + for(uint i = 0; i < tokens.length; i++) { + ERC20Basic token = ERC20Basic(tokens[i]); + uint256 balance = token.balanceOf(this); + token.transfer(owner, balance); + } + + // Transfer Eth to owner and terminate contract + selfdestruct(owner); + } +} diff --git a/test/TokenKillable.js b/test/TokenKillable.js new file mode 100644 index 000000000..88b06c1b0 --- /dev/null +++ b/test/TokenKillable.js @@ -0,0 +1,32 @@ +'use strict'; + +var TokenKillable = artifacts.require('../contracts/lifecycle/TokenKillable.sol'); +var StandardTokenMock = artifacts.require("./helpers/StandardTokenMock.sol"); +require('./helpers/transactionMined.js'); + +contract('TokenKillable', function(accounts) { + + it('should send balance to owner after death', async function() { + let killable = await TokenKillable.new({from: accounts[0], value: web3.toWei('10','ether')}); + let owner = await killable.owner(); + let initBalance = web3.eth.getBalance(owner); + await killable.kill([], {from: owner}); + let newBalance = web3.eth.getBalance(owner); + assert.isTrue(newBalance > initBalance); + }); + + it('should send tokens to owner after death', async function() { + let killable = await TokenKillable.new({from: accounts[0], value: web3.toWei('10','ether')}); + let owner = await killable.owner(); + let token = await StandardTokenMock.new(killable.address, 100); + let initContractBalance = await token.balanceOf(killable.address); + let initOwnerBalance = await token.balanceOf(owner); + assert.equal(initContractBalance, 100); + assert.equal(initOwnerBalance, 0); + await killable.kill([token.address], {from: owner}); + let newContractBalance = await token.balanceOf(killable.address); + let newOwnerBalance = await token.balanceOf(owner); + assert.equal(newContractBalance, 0); + assert.equal(newOwnerBalance, 100); + }); +}); From ce25e2db987089e6903a73091925253dac4fa1c0 Mon Sep 17 00:00:00 2001 From: Manuel Araoz Date: Mon, 20 Mar 2017 11:58:56 -0300 Subject: [PATCH 02/49] try to fix travis 1 --- test/StandardToken.js | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/test/StandardToken.js b/test/StandardToken.js index 96cda8b3e..c9bad8ead 100644 --- a/test/StandardToken.js +++ b/test/StandardToken.js @@ -1,26 +1,28 @@ +'use strict'; + const assertJump = require('./helpers/assertJump'); -var StandardTokenMock = artifacts.require("./helpers/StandardTokenMock.sol"); +var StandardTokenMock = artifacts.require('./helpers/StandardTokenMock.sol'); contract('StandardToken', function(accounts) { - it("should return the correct totalSupply after construction", async function() { + it('should return the correct totalSupply after construction', async function() { let token = await StandardTokenMock.new(accounts[0], 100); let totalSupply = await token.totalSupply(); assert.equal(totalSupply, 100); - }) + }); - it("should return the correct allowance amount after approval", async function() { + it.only('should return the correct allowance amount after approval', async function() { let token = await StandardTokenMock.new(); - let approve = await token.approve(accounts[1], 100); + await token.approve(accounts[1], 100); let allowance = await token.allowance(accounts[0], accounts[1]); assert.equal(allowance, 100); }); - it("should return correct balances after transfer", async function() { + it('should return correct balances after transfer', async function() { let token = await StandardTokenMock.new(accounts[0], 100); - let transfer = await token.transfer(accounts[1], 100); + await token.transfer(accounts[1], 100); let balance0 = await token.balanceOf(accounts[0]); assert.equal(balance0, 0); @@ -28,20 +30,20 @@ contract('StandardToken', function(accounts) { assert.equal(balance1, 100); }); - it("should throw an error when trying to transfer more than balance", async function() { + it('should throw an error when trying to transfer more than balance', async function() { let token = await StandardTokenMock.new(accounts[0], 100); try { - let transfer = await token.transfer(accounts[1], 101); + await token.transfer(accounts[1], 101); } catch(error) { return assertJump(error); } assert.fail('should have thrown before'); }); - it("should return correct balances after transfering from another account", async function() { + it('should return correct balances after transfering from another account', async function() { let token = await StandardTokenMock.new(accounts[0], 100); - let approve = await token.approve(accounts[1], 100); - let transferFrom = await token.transferFrom(accounts[0], accounts[2], 100, {from: accounts[1]}); + await token.approve(accounts[1], 100); + await token.transferFrom(accounts[0], accounts[2], 100, {from: accounts[1]}); let balance0 = await token.balanceOf(accounts[0]); assert.equal(balance0, 0); @@ -53,11 +55,11 @@ contract('StandardToken', function(accounts) { assert.equal(balance2, 0); }); - it("should throw an error when trying to transfer more than allowed", async function() { + it('should throw an error when trying to transfer more than allowed', async function() { let token = await StandardTokenMock.new(); - let approve = await token.approve(accounts[1], 99); + await token.approve(accounts[1], 99); try { - let transfer = await token.transferFrom(accounts[0], accounts[2], 100, {from: accounts[1]}); + await token.transferFrom(accounts[0], accounts[2], 100, {from: accounts[1]}); } catch (error) { return assertJump(error); } From 960500a0785a4ec1a845525c11075514589b0c6b Mon Sep 17 00:00:00 2001 From: Manuel Araoz Date: Mon, 20 Mar 2017 15:19:14 -0300 Subject: [PATCH 03/49] try to fix travis 2 --- test/StandardToken.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/StandardToken.js b/test/StandardToken.js index c9bad8ead..e3f578726 100644 --- a/test/StandardToken.js +++ b/test/StandardToken.js @@ -12,7 +12,7 @@ contract('StandardToken', function(accounts) { assert.equal(totalSupply, 100); }); - it.only('should return the correct allowance amount after approval', async function() { + it('should return the correct allowance amount after approval', async function() { let token = await StandardTokenMock.new(); await token.approve(accounts[1], 100); let allowance = await token.allowance(accounts[0], accounts[1]); From d1af3ef1b3a746ddf3cd0773830ff1887e8f1693 Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Thu, 23 Mar 2017 12:23:27 +0000 Subject: [PATCH 04/49] Add HasNoEther --- contracts/ownership/HasNoEther.sol | 42 ++++++++++++++++++++ test/HasNoEther.js | 63 ++++++++++++++++++++++++++++++ test/helpers/ForceEther.sol | 13 ++++++ test/helpers/HasNoEtherTest.sol | 17 ++++++++ test/helpers/expectThrow.js | 12 ++++++ test/helpers/toPromise.js | 4 ++ 6 files changed, 151 insertions(+) create mode 100644 contracts/ownership/HasNoEther.sol create mode 100644 test/HasNoEther.js create mode 100644 test/helpers/ForceEther.sol create mode 100644 test/helpers/HasNoEtherTest.sol create mode 100644 test/helpers/expectThrow.js create mode 100644 test/helpers/toPromise.js diff --git a/contracts/ownership/HasNoEther.sol b/contracts/ownership/HasNoEther.sol new file mode 100644 index 000000000..eb462a61e --- /dev/null +++ b/contracts/ownership/HasNoEther.sol @@ -0,0 +1,42 @@ +pragma solidity ^0.4.8; + +import "./Ownable.sol"; + +/// @title Contracts that should not own Ether +/// @author Remco Bloemen +/// +/// This tries to block incoming ether to prevent accidental +/// loss of Ether. Should Ether end up in the contrat, it will +/// allow the owner to reclaim this ether. +/// +/// @notice Ether can still be send to this contract by: +/// * calling functions labeled `payable` +/// * `selfdestruct(contract_address)` +/// * mining directly to the contract address +contract HasNoEther is Ownable { + + /// Constructor that rejects incoming Ether + /// @dev The flag `payabe` is added so we can access `msg.value` + /// without compiler warning. If we leave out payable, then + /// Solidity will allow inheriting contracts to implement a + /// payable constructor. By doing it this way we prevent a + /// payable constructor from working. + /// Alternatively we could use assembly to access msg.value. + function HasNoEther() payable { + if(msg.value > 0) { + throw; + } + } + + /// Disallow direct send by settings a default function without `payable` + function() external { + } + + /// Transfer all Ether owned by the contract to the owner + /// @dev What if owner is itself a contract marked HasNoEther? + function reclaimEther() external onlyOwner { + if(!owner.send(this.balance)) { + throw; + } + } +} diff --git a/test/HasNoEther.js b/test/HasNoEther.js new file mode 100644 index 000000000..ac19634b3 --- /dev/null +++ b/test/HasNoEther.js @@ -0,0 +1,63 @@ +'use strict'; +import expectThrow from './helpers/expectThrow'; +import toPromise from './helpers/toPromise'; +const HasNoEther = artifacts.require('../contracts/lifecycle/HasNoEther.sol'); +const HasNoEtherTest = artifacts.require('../helpers/HasNoEtherTest.sol'); +const ForceEther = artifacts.require('../helpers/ForceEther.sol'); + +contract('HasNoEther', function(accounts) { + const amount = web3.toWei('1', 'ether'); + + it('should be constructorable', async function() { + let hasNoEther = await HasNoEtherTest.new(); + }); + + it('should not accept ether in constructor', async function() { + await expectThrow(HasNoEtherTest.new({value: amount})); + }); + + it('should not accept ether', async function() { + let hasNoEther = await HasNoEtherTest.new(); + + await expectThrow( + toPromise(web3.eth.sendTransaction)({ + from: accounts[1], + to: hasNoEther.address, + value: amount, + }), + ); + }); + + it('should allow owner to reclaim ether', async function() { + // Create contract + let hasNoEther = await HasNoEtherTest.new(); + const startBalance = await web3.eth.getBalance(hasNoEther.address); + assert.equal(startBalance, 0); + + // Force ether into it + await ForceEther.new(hasNoEther.address, {value: amount}); + const forcedBalance = await web3.eth.getBalance(hasNoEther.address); + assert.equal(forcedBalance, amount); + + // Reclaim + const ownerStartBalance = await web3.eth.getBalance(accounts[0]); + await hasNoEther.reclaimEther(); + const ownerFinalBalance = await web3.eth.getBalance(accounts[0]); + const finalBalance = await web3.eth.getBalance(hasNoEther.address); + assert.equal(finalBalance, 0); + assert.isAbove(ownerFinalBalance, ownerStartBalance); + }); + + it('should allow only owner to reclaim ether', async function() { + // Create contract + let hasNoEther = await HasNoEtherTest.new({from: accounts[0]}); + + // Force ether into it + await ForceEther.new(hasNoEther.address, {value: amount}); + const forcedBalance = await web3.eth.getBalance(hasNoEther.address); + assert.equal(forcedBalance, amount); + + // Reclaim + await expectThrow(hasNoEther.reclaimEther({from: accounts[1]})); + }); +}); diff --git a/test/helpers/ForceEther.sol b/test/helpers/ForceEther.sol new file mode 100644 index 000000000..cb73b6349 --- /dev/null +++ b/test/helpers/ForceEther.sol @@ -0,0 +1,13 @@ +pragma solidity ^0.4.8; + +// @title Force Ether into a contract. +// @notice even +// if the contract is not payable. +// @notice To use, construct the contract with the target as argument. +// @author Remco Bloemen +contract ForceEther { + function ForceEther(address target) payable { + // Selfdestruct transfers all Ether to the arget address + selfdestruct(target); + } +} diff --git a/test/helpers/HasNoEtherTest.sol b/test/helpers/HasNoEtherTest.sol new file mode 100644 index 000000000..bcafed326 --- /dev/null +++ b/test/helpers/HasNoEtherTest.sol @@ -0,0 +1,17 @@ +pragma solidity ^0.4.8; + +import "../../contracts/ownership/HasNoEther.sol"; + +contract HasNoEtherTest is HasNoEther { + + // Constructor with explicit payable — should still fail + function HasNoEtherTest() payable { + + } + + // Default function with explicit payable — should still fail + function() external payable { + throw; + } + +} diff --git a/test/helpers/expectThrow.js b/test/helpers/expectThrow.js new file mode 100644 index 000000000..8d0142d4b --- /dev/null +++ b/test/helpers/expectThrow.js @@ -0,0 +1,12 @@ +export default async promise => { + try { + await promise; + } catch (error) { + // TODO: Check jump destination to destinguish between a throw + // and an actual invalid jump. + const invalidJump = error.message.search('invalid JUMP') >= 0; + assert(invalidJump, "Expected throw, got '" + error + "' instead"); + return; + } + assert.fail('Expected throw not received'); +}; diff --git a/test/helpers/toPromise.js b/test/helpers/toPromise.js new file mode 100644 index 000000000..6a24dc362 --- /dev/null +++ b/test/helpers/toPromise.js @@ -0,0 +1,4 @@ +export default func => + (...args) => + new Promise((accept, reject) => + func(...args, (error, data) => error ? reject(error) : accept(data))); From 9ff82aecef62b402fa7b7837af8089e42fa01eb6 Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Thu, 23 Mar 2017 12:54:42 +0000 Subject: [PATCH 05/49] Remove non-compiling payable default --- test/helpers/HasNoEtherTest.sol | 6 ------ 1 file changed, 6 deletions(-) diff --git a/test/helpers/HasNoEtherTest.sol b/test/helpers/HasNoEtherTest.sol index bcafed326..6b29e325d 100644 --- a/test/helpers/HasNoEtherTest.sol +++ b/test/helpers/HasNoEtherTest.sol @@ -6,12 +6,6 @@ contract HasNoEtherTest is HasNoEther { // Constructor with explicit payable — should still fail function HasNoEtherTest() payable { - - } - - // Default function with explicit payable — should still fail - function() external payable { - throw; } } From 94d3c447b788c09b6c79cbe8de6caf126af7a593 Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Thu, 23 Mar 2017 15:22:48 +0000 Subject: [PATCH 06/49] Add HasNoTokens --- contracts/ownership/HasNoTokens.sol | 26 +++++++++++++++++++ test/HasNoTokens.js | 40 +++++++++++++++++++++++++++++ test/helpers/ERC23TokenMock.sol | 33 ++++++++++++++++++++++++ test/helpers/expectThrow.js | 10 +++++++- 4 files changed, 108 insertions(+), 1 deletion(-) create mode 100644 contracts/ownership/HasNoTokens.sol create mode 100644 test/HasNoTokens.js create mode 100644 test/helpers/ERC23TokenMock.sol diff --git a/contracts/ownership/HasNoTokens.sol b/contracts/ownership/HasNoTokens.sol new file mode 100644 index 000000000..9b376930b --- /dev/null +++ b/contracts/ownership/HasNoTokens.sol @@ -0,0 +1,26 @@ +pragma solidity ^0.4.8; + +import "./Ownable.sol"; +import "../token/ERC20Basic.sol"; + +/// @title Contracts that should not own Tokens +/// @author Remco Bloemen +/// +/// This blocks incoming ERC23 tokens to prevent accidental +/// loss of tokens. Should tokens (any ERC20Basic compatible) +/// end up in the contract, it allows the owner to reclaim +/// the tokens. +contract HasNoTokens is Ownable { + + /// Reject all ERC23 compatible tokens + function tokenFallback(address from_, uint value_, bytes data_) external { + throw; + } + + /// Reclaim all ERC20Basic compatible tokens + function reclaimToken(address tokenAddr) external onlyOwner { + ERC20Basic tokenInst = ERC20Basic(tokenAddr); + uint256 balance = tokenInst.balanceOf(this); + tokenInst.transfer(owner, balance); + } +} diff --git a/test/HasNoTokens.js b/test/HasNoTokens.js new file mode 100644 index 000000000..5afe29ef0 --- /dev/null +++ b/test/HasNoTokens.js @@ -0,0 +1,40 @@ +'use strict'; +import expectThrow from './helpers/expectThrow'; +import toPromise from './helpers/toPromise'; +const HasNoTokens = artifacts.require('../contracts/lifecycle/HasNoTokens.sol'); +const ERC23TokenMock = artifacts.require('./helpers/ERC23TokenMock.sol'); + +contract('HasNoTokens', function(accounts) { + let hasNoTokens = null; + let token = null; + + beforeEach(async () => { + // Create contract and token + hasNoTokens = await HasNoTokens.new(); + token = await ERC23TokenMock.new(accounts[0], 100); + + // Force token into contract + await token.transfer(hasNoTokens.address, 10); + const startBalance = await token.balanceOf(hasNoTokens.address); + assert.equal(startBalance, 10); + }); + + it('should not accept ERC23 tokens', async function() { + await expectThrow(token.transferERC23(hasNoTokens.address, 10, '')); + }); + + it('should allow owner to reclaim tokens', async function() { + const ownerStartBalance = await token.balanceOf(accounts[0]); + await hasNoTokens.reclaimToken(token.address); + const ownerFinalBalance = await token.balanceOf(accounts[0]); + const finalBalance = await token.balanceOf(hasNoTokens.address); + assert.equal(finalBalance, 0); + assert.equal(ownerFinalBalance - ownerStartBalance, 10); + }); + + it('should allow only owner to reclaim tokens', async function() { + await expectThrow( + hasNoTokens.reclaimToken(token.address, {from: accounts[1]}), + ); + }); +}); diff --git a/test/helpers/ERC23TokenMock.sol b/test/helpers/ERC23TokenMock.sol new file mode 100644 index 000000000..417c2494b --- /dev/null +++ b/test/helpers/ERC23TokenMock.sol @@ -0,0 +1,33 @@ +pragma solidity ^0.4.8; + + +import '../../contracts/token/BasicToken.sol'; + + +contract ERC23ContractInterface { + function tokenFallback(address _from, uint _value, bytes _data) external; +} + +contract ERC23TokenMock is BasicToken { + + function ERC23TokenMock(address initialAccount, uint initialBalance) { + balances[initialAccount] = initialBalance; + totalSupply = initialBalance; + } + + // ERC23 compatible transfer function (except the name) + function transferERC23(address _to, uint _value, bytes _data) + returns (bool success) + { + transfer(_to, _value); + bool is_contract = false; + assembly { + is_contract := not(iszero(extcodesize(_to))) + } + if(is_contract) { + ERC23ContractInterface receiver = ERC23ContractInterface(_to); + receiver.tokenFallback(msg.sender, _value, _data); + } + return true; + } +} diff --git a/test/helpers/expectThrow.js b/test/helpers/expectThrow.js index 8d0142d4b..45bdcfdb0 100644 --- a/test/helpers/expectThrow.js +++ b/test/helpers/expectThrow.js @@ -5,7 +5,15 @@ export default async promise => { // TODO: Check jump destination to destinguish between a throw // and an actual invalid jump. const invalidJump = error.message.search('invalid JUMP') >= 0; - assert(invalidJump, "Expected throw, got '" + error + "' instead"); + // TODO: When we contract A calls contract B, and B throws, instead + // of an 'invalid jump', we get an 'out of gas' error. How do + // we distinguish this from an actual out of gas event? (The + // testrpc log actually show an 'invalid jump' event.) + const outOfGas = error.message.search('out of gas') >= 0; + assert( + invalidJump || outOfGas, + "Expected throw, got '" + error + "' instead", + ); return; } assert.fail('Expected throw not received'); From 166a1070e57d9aeab59c042d77ebd070f31a0851 Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Thu, 23 Mar 2017 15:43:52 +0000 Subject: [PATCH 07/49] Add HasNoContracts --- contracts/ownership/HasNoContracts.sol | 18 +++++++++++++ test/HasNoContracts.js | 35 ++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) create mode 100644 contracts/ownership/HasNoContracts.sol create mode 100644 test/HasNoContracts.js diff --git a/contracts/ownership/HasNoContracts.sol b/contracts/ownership/HasNoContracts.sol new file mode 100644 index 000000000..842638dc2 --- /dev/null +++ b/contracts/ownership/HasNoContracts.sol @@ -0,0 +1,18 @@ +pragma solidity ^0.4.8; + +import "./Ownable.sol"; + +/// @title Contracts that should not own Contracts +/// @author Remco Bloemen +/// +/// Should contracts (anything Ownable) end up being owned by +/// this contract, it allows the owner of this contract to +/// reclaim ownership of the contracts. +contract HasNoContracts is Ownable { + + /// Reclaim ownership of Ownable contracts + function reclaimContract(address contractAddr) external onlyOwner { + Ownable contractInst = Ownable(contractAddr); + contractInst.transferOwnership(owner); + } +} diff --git a/test/HasNoContracts.js b/test/HasNoContracts.js new file mode 100644 index 000000000..cfe879ad4 --- /dev/null +++ b/test/HasNoContracts.js @@ -0,0 +1,35 @@ +'use strict'; +import expectThrow from './helpers/expectThrow'; +import toPromise from './helpers/toPromise'; +const Ownable = artifacts.require('../contracts/ownership/Ownable.sol'); +const HasNoContracts = artifacts.require( + '../contracts/ownership/HasNoContracts.sol', +); + +contract('HasNoContracts', function(accounts) { + let hasNoContracts = null; + let ownable = null; + + beforeEach(async () => { + // Create contract and token + hasNoContracts = await HasNoContracts.new(); + ownable = await Ownable.new(); + + // Force ownership into contract + await ownable.transferOwnership(hasNoContracts.address); + const owner = await ownable.owner(); + assert.equal(owner, hasNoContracts.address); + }); + + it('should allow owner to reclaim contracts', async function() { + await hasNoContracts.reclaimContract(ownable.address); + const owner = await ownable.owner(); + assert.equal(owner, accounts[0]); + }); + + it('should allow only owner to reclaim contracts', async function() { + await expectThrow( + hasNoContracts.reclaimContract(ownable.address, {from: accounts[1]}), + ); + }); +}); From ac3b3652c38086b0cc909a8cd6bf4e6235b66d43 Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Thu, 23 Mar 2017 15:48:22 +0000 Subject: [PATCH 08/49] Add NoOwner --- contracts/ownership/NoOwner.sol | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 contracts/ownership/NoOwner.sol diff --git a/contracts/ownership/NoOwner.sol b/contracts/ownership/NoOwner.sol new file mode 100644 index 000000000..f7a567e56 --- /dev/null +++ b/contracts/ownership/NoOwner.sol @@ -0,0 +1,14 @@ +pragma solidity ^0.4.8; + +import "./HasNoEther.sol"; +import "./HasNoTokens.sol"; +import "./HasNoContracts.sol"; + +/// @title Base contract for contracts that should not own things. +/// @author Remco Bloemen +/// +/// Solves a class of errors where a contract accidentally +/// becomes owner of Ether, Tokens or Owned contracts. See +/// respective base contracts for details. +contract NoOwner is HasNoEther, HasNoTokens, HasNoContracts { +} From a2bd1bb7f6a6d68029e02ec42ad3309fd2ba331b Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Fri, 24 Mar 2017 11:01:06 +0000 Subject: [PATCH 09/49] Add ReentrancyGuard --- contracts/ReentrancyGuard.sol | 28 +++++++++++++++++++ test/ReentrancyGuard.js | 31 +++++++++++++++++++++ test/helpers/ReentrancyAttack.sol | 11 ++++++++ test/helpers/ReentrancyMock.sol | 46 +++++++++++++++++++++++++++++++ test/helpers/expectThrow.js | 20 ++++++++++++++ 5 files changed, 136 insertions(+) create mode 100644 contracts/ReentrancyGuard.sol create mode 100644 test/ReentrancyGuard.js create mode 100644 test/helpers/ReentrancyAttack.sol create mode 100644 test/helpers/ReentrancyMock.sol create mode 100644 test/helpers/expectThrow.js diff --git a/contracts/ReentrancyGuard.sol b/contracts/ReentrancyGuard.sol new file mode 100644 index 000000000..02c3c49c8 --- /dev/null +++ b/contracts/ReentrancyGuard.sol @@ -0,0 +1,28 @@ +pragma solidity ^0.4.8; + +/// @title Helps contracts guard agains rentrancy attacks. +/// @author Remco Bloemen +/// @notice If you mark a function `nonReentrant`, you should also +/// mark it `external`. +contract ReentrancyGuard { + + /// @dev We use a single lock for the whole contract. + bool private rentrancy_lock = false; + + /// Prevent contract from calling itself, directly or indirectly. + /// @notice If you mark a function `nonReentrant`, you should also + /// mark it `external`. Calling one nonReentrant function from + /// another is not supported. Instead, you can implement a + /// `private` function doing the actual work, and a `external` + /// wrapper marked as `nonReentrant`. + modifier nonReentrant() { + if(rentrancy_lock == false) { + rentrancy_lock = true; + _; + rentrancy_lock = false; + } else { + throw; + } + } + +} diff --git a/test/ReentrancyGuard.js b/test/ReentrancyGuard.js new file mode 100644 index 000000000..b3145df1d --- /dev/null +++ b/test/ReentrancyGuard.js @@ -0,0 +1,31 @@ +'use strict'; +import expectThrow from './helpers/expectThrow'; +const ReentrancyMock = artifacts.require('./helper/ReentrancyMock.sol'); +const ReentrancyAttack = artifacts.require('./helper/ReentrancyAttack.sol'); + +contract('ReentrancyGuard', function(accounts) { + let reentrancyMock; + + beforeEach(async function() { + reentrancyMock = await ReentrancyMock.new(); + let initialCounter = await reentrancyMock.counter(); + assert.equal(initialCounter, 0); + }); + + it('should not allow remote callback', async function() { + let attacker = await ReentrancyAttack.new(); + await expectThrow(reentrancyMock.countAndCall(attacker.address)); + }); + + // The following are more side-effects that intended behaviour: + // I put them here as documentation, and to monitor any changes + // in the side-effects. + + it('should not allow local recursion', async function() { + await expectThrow(reentrancyMock.countLocalRecursive(10)); + }); + + it('should not allow indirect local recursion', async function() { + await expectThrow(reentrancyMock.countThisRecursive(10)); + }); +}); diff --git a/test/helpers/ReentrancyAttack.sol b/test/helpers/ReentrancyAttack.sol new file mode 100644 index 000000000..ce67683a6 --- /dev/null +++ b/test/helpers/ReentrancyAttack.sol @@ -0,0 +1,11 @@ +pragma solidity ^0.4.8; + +contract ReentrancyAttack { + + function callSender(bytes4 data) { + if(!msg.sender.call(data)) { + throw; + } + } + +} diff --git a/test/helpers/ReentrancyMock.sol b/test/helpers/ReentrancyMock.sol new file mode 100644 index 000000000..dbfb41209 --- /dev/null +++ b/test/helpers/ReentrancyMock.sol @@ -0,0 +1,46 @@ +pragma solidity ^0.4.8; + +import '../../contracts/ReentrancyGuard.sol'; +import './ReentrancyAttack.sol'; + +contract ReentrancyMock is ReentrancyGuard { + + uint256 public counter; + + function ReentrancyMock() { + counter = 0; + } + + function count() private { + counter += 1; + } + + function countLocalRecursive(uint n) public nonReentrant { + if(n > 0) { + count(); + countLocalRecursive(n - 1); + } + } + + function countThisRecursive(uint256 n) public nonReentrant { + bytes4 func = bytes4(keccak256("countThisRecursive(uint256)")); + if(n > 0) { + count(); + bool result = this.call(func, n - 1); + if(result != true) { + throw; + } + } + } + + function countAndCall(ReentrancyAttack attacker) public nonReentrant { + count(); + bytes4 func = bytes4(keccak256("callback()")); + attacker.callSender(func); + } + + function callback() external nonReentrant { + count(); + } + +} diff --git a/test/helpers/expectThrow.js b/test/helpers/expectThrow.js new file mode 100644 index 000000000..45bdcfdb0 --- /dev/null +++ b/test/helpers/expectThrow.js @@ -0,0 +1,20 @@ +export default async promise => { + try { + await promise; + } catch (error) { + // TODO: Check jump destination to destinguish between a throw + // and an actual invalid jump. + const invalidJump = error.message.search('invalid JUMP') >= 0; + // TODO: When we contract A calls contract B, and B throws, instead + // of an 'invalid jump', we get an 'out of gas' error. How do + // we distinguish this from an actual out of gas event? (The + // testrpc log actually show an 'invalid jump' event.) + const outOfGas = error.message.search('out of gas') >= 0; + assert( + invalidJump || outOfGas, + "Expected throw, got '" + error + "' instead", + ); + return; + } + assert.fail('Expected throw not received'); +}; From 967ee13565fec6f9408c608fc6d7f7ceb35c43e7 Mon Sep 17 00:00:00 2001 From: Demian Elias Brener Date: Mon, 27 Mar 2017 17:21:29 -0300 Subject: [PATCH 10/49] OpenZeppelin --- README.md | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index abc68101b..d3d28cafa 100644 --- a/README.md +++ b/README.md @@ -2,9 +2,9 @@ [![NPM Package](https://img.shields.io/npm/v/zeppelin-solidity.svg?style=flat-square)](https://www.npmjs.org/package/zeppelin-solidity) [![Build Status](https://img.shields.io/travis/OpenZeppelin/zeppelin-solidity.svg?branch=master&style=flat-square)](https://travis-ci.org/OpenZeppelin/zeppelin-solidity) -Zeppelin is a library for writing secure [Smart Contracts](https://en.wikipedia.org/wiki/Smart_contract) on Ethereum. +OpenZeppelin is a library for writing secure [Smart Contracts](https://en.wikipedia.org/wiki/Smart_contract) on Ethereum. -With Zeppelin, you can build distributed applications, protocols and organizations: +With OpenZeppelin, you can build distributed applications, protocols and organizations: - using common contract security patterns (See [Onward with Ethereum Smart Contract Security](https://medium.com/bitcorps-blog/onward-with-ethereum-smart-contract-security-97a827e47702#.y3kvdetbz)) - in the [Solidity language](http://solidity.readthedocs.io/en/develop/). @@ -12,7 +12,7 @@ With Zeppelin, you can build distributed applications, protocols and organizatio ## Getting Started -Zeppelin integrates with [Truffle](https://github.com/ConsenSys/truffle), an Ethereum development environment. Please install Truffle and initialize your project with `truffle init`. +OpenZeppelin integrates with [Truffle](https://github.com/ConsenSys/truffle), an Ethereum development environment. Please install Truffle and initialize your project with `truffle init`. ```sh npm install -g truffle@beta @@ -20,7 +20,7 @@ mkdir myproject && cd myproject truffle init ``` -To install the Zeppelin library, run: +To install the OpenZeppelin library, run: ```sh truffle install zeppelin ``` @@ -36,25 +36,25 @@ contract MyContract is Ownable { ``` ## Security -Zeppelin is meant to provide secure, tested and community-audited code, but please use common sense when doing anything that deals with real money! We take no responsibility for your implementation decisions and any security problem you might experience. +OpenZeppelin is meant to provide secure, tested and community-audited code, but please use common sense when doing anything that deals with real money! We take no responsibility for your implementation decisions and any security problem you might experience. If you find a security issue, please email [security@openzeppelin.org](mailto:security@openzeppelin.org). ## Developer Resources -Building a distributed application, protocol or organization with Zeppelin? +Building a distributed application, protocol or organization with OpenZeppelin? - Read documentation: http://zeppelin-solidity.readthedocs.io/en/latest/ - Ask for help and follow progress at: https://zeppelin-slackin.herokuapp.com/ -Interested in contributing to Zeppelin? +Interested in contributing to OpenZeppelin? - Framework proposal and roadmap: https://medium.com/zeppelin-blog/zeppelin-framework-proposal-and-development-roadmap-fdfa9a3a32ab#.iain47pak - Issue tracker: https://github.com/OpenZeppelin/zeppelin-solidity/issues - Contribution guidelines: https://github.com/OpenZeppelin/zeppelin-solidity/blob/master/CONTRIBUTING.md -## Collaborating organizations and audits by Zeppelin +## Collaborating organizations and audits by OpenZeppelin - [Golem](https://golem.network/) - [Mediachain](http://www.mediachain.io/) - [Truffle](http://truffleframework.com/) From a885dabc8174223baf05a6744e2aae4c9e498109 Mon Sep 17 00:00:00 2001 From: Demian Elias Brener Date: Wed, 29 Mar 2017 18:04:13 -0300 Subject: [PATCH 11/49] added Wings --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index d3d28cafa..662868b16 100644 --- a/README.md +++ b/README.md @@ -67,6 +67,7 @@ Interested in contributing to OpenZeppelin? - [Signatura](https://signatura.co/) - [Ether.camp](http://www.ether.camp/) - [Aragon](https://aragon.one/) +- [Wings](https://wings.ai/) among others... From 1d2b989e8e2a8a9867baf7ff73e190267b40480b Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Fri, 31 Mar 2017 09:39:29 +0100 Subject: [PATCH 12/49] Fix typo in comment --- contracts/ownership/HasNoEther.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/ownership/HasNoEther.sol b/contracts/ownership/HasNoEther.sol index eb462a61e..765983791 100644 --- a/contracts/ownership/HasNoEther.sol +++ b/contracts/ownership/HasNoEther.sol @@ -16,7 +16,7 @@ import "./Ownable.sol"; contract HasNoEther is Ownable { /// Constructor that rejects incoming Ether - /// @dev The flag `payabe` is added so we can access `msg.value` + /// @dev The flag `payable` is added so we can access `msg.value` /// without compiler warning. If we leave out payable, then /// Solidity will allow inheriting contracts to implement a /// payable constructor. By doing it this way we prevent a From 7f7238378c992c2473c73420337d2fd2ea0545bf Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 31 Mar 2017 12:53:19 -0300 Subject: [PATCH 13/49] change type of owners from uint[] to address[] fixes #175 --- contracts/ownership/Shareable.sol | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/contracts/ownership/Shareable.sol b/contracts/ownership/Shareable.sol index e07516bd1..62fbc2fee 100644 --- a/contracts/ownership/Shareable.sol +++ b/contracts/ownership/Shareable.sol @@ -23,9 +23,9 @@ contract Shareable { uint public required; // list of owners - uint[256] owners; + address[256] owners; // index on the list of owners to allow reverse lookup - mapping(uint => uint) ownerIndex; + mapping(address => uint) ownerIndex; // the ongoing operations. mapping(bytes32 => PendingState) pendings; bytes32[] pendingsIndex; @@ -57,18 +57,18 @@ contract Shareable { // constructor is given number of sigs required to do protected "onlymanyowners" transactions // as well as the selection of addresses capable of confirming them. function Shareable(address[] _owners, uint _required) { - owners[1] = uint(msg.sender); - ownerIndex[uint(msg.sender)] = 1; + owners[1] = msg.sender; + ownerIndex[msg.sender] = 1; for (uint i = 0; i < _owners.length; ++i) { - owners[2 + i] = uint(_owners[i]); - ownerIndex[uint(_owners[i])] = 2 + i; + owners[2 + i] = _owners[i]; + ownerIndex[_owners[i]] = 2 + i; } required = _required; } // Revokes a prior confirmation of the given operation function revoke(bytes32 _operation) external { - uint index = ownerIndex[uint(msg.sender)]; + uint index = ownerIndex[msg.sender]; // make sure they're an owner if (index == 0) { return; @@ -88,12 +88,12 @@ contract Shareable { } function isOwner(address _addr) constant returns (bool) { - return ownerIndex[uint(_addr)] > 0; + return ownerIndex[_addr] > 0; } function hasConfirmed(bytes32 _operation, address _owner) constant returns (bool) { var pending = pendings[_operation]; - uint index = ownerIndex[uint(_owner)]; + uint index = ownerIndex[_owner]; // make sure they're an owner if (index == 0) { @@ -107,7 +107,7 @@ contract Shareable { function confirmAndCheck(bytes32 _operation) internal returns (bool) { // determine what index the present sender is: - uint index = ownerIndex[uint(msg.sender)]; + uint index = ownerIndex[msg.sender]; // make sure they're an owner if (index == 0) { return; From 6f311e72b3020a7aa9cfaa2c41aef8fa1f51cd34 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 3 Apr 2017 11:30:32 -0300 Subject: [PATCH 14/49] change sha3 and suicide methods to keccak256 and selfdestruct see ethereum/solidity#1476 --- contracts/MultisigWallet.sol | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/contracts/MultisigWallet.sol b/contracts/MultisigWallet.sol index c8ed091c6..f9249218d 100644 --- a/contracts/MultisigWallet.sol +++ b/contracts/MultisigWallet.sol @@ -25,8 +25,8 @@ contract MultisigWallet is Multisig, Shareable, DayLimit { DayLimit(_daylimit) { } // kills the contract sending everything to `_to`. - function kill(address _to) onlymanyowners(sha3(msg.data)) external { - suicide(_to); + function kill(address _to) onlymanyowners(keccak256(msg.data)) external { + selfdestruct(_to); } // gets called when no other function matches @@ -51,7 +51,7 @@ contract MultisigWallet is Multisig, Shareable, DayLimit { return 0; } // determine our operation hash. - _r = sha3(msg.data, block.number); + _r = keccak256(msg.data, block.number); if (!confirm(_r) && txs[_r].to == 0) { txs[_r].to = _to; txs[_r].value = _value; @@ -73,11 +73,11 @@ contract MultisigWallet is Multisig, Shareable, DayLimit { } } - function setDailyLimit(uint _newLimit) onlymanyowners(sha3(msg.data)) external { + function setDailyLimit(uint _newLimit) onlymanyowners(keccak256(msg.data)) external { _setDailyLimit(_newLimit); } - function resetSpentToday() onlymanyowners(sha3(msg.data)) external { + function resetSpentToday() onlymanyowners(keccak256(msg.data)) external { _resetSpentToday(); } From 61e33197b2cdf7d86455ddd98a38b6a26002b8ea Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 3 Apr 2017 22:04:07 -0300 Subject: [PATCH 15/49] rename TransferableToken to LimitedTransferToken fixes #179 --- .../{TransferableToken.sol => LimitedTransferToken.sol} | 6 +++--- contracts/token/VestedToken.sol | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) rename contracts/token/{TransferableToken.sol => LimitedTransferToken.sol} (89%) diff --git a/contracts/token/TransferableToken.sol b/contracts/token/LimitedTransferToken.sol similarity index 89% rename from contracts/token/TransferableToken.sol rename to contracts/token/LimitedTransferToken.sol index e7df65194..2bfe873d0 100644 --- a/contracts/token/TransferableToken.sol +++ b/contracts/token/LimitedTransferToken.sol @@ -4,7 +4,7 @@ import "./ERC20.sol"; /* -TransferableToken defines the generic interface and the implementation +LimitedTransferToken defines the generic interface and the implementation to limit token transferability for different events. It is intended to be used as a base class for other token contracts. @@ -12,7 +12,7 @@ It is intended to be used as a base class for other token contracts. Over-writting transferableTokens(address holder, uint64 time) is the way to provide the specific logic for limitting token transferability for a holder over time. -TransferableToken has been designed to allow for different limitting factors, +LimitedTransferToken has been designed to allow for different limitting factors, this can be achieved by recursively calling super.transferableTokens() until the base class is hit. For example: @@ -25,7 +25,7 @@ https://github.com/OpenZeppelin/zeppelin-solidity/blob/master/contracts/token/Ve */ -contract TransferableToken is ERC20 { +contract LimitedTransferToken is ERC20 { // Checks whether it can transfer or otherwise throws. modifier canTransfer(address _sender, uint _value) { if (_value > transferableTokens(_sender, uint64(now))) throw; diff --git a/contracts/token/VestedToken.sol b/contracts/token/VestedToken.sol index 9eb822672..da350584b 100644 --- a/contracts/token/VestedToken.sol +++ b/contracts/token/VestedToken.sol @@ -2,9 +2,9 @@ pragma solidity ^0.4.8; import "./StandardToken.sol"; -import "./TransferableToken.sol"; +import "./LimitedTransferToken.sol"; -contract VestedToken is StandardToken, TransferableToken { +contract VestedToken is StandardToken, LimitedTransferToken { struct TokenGrant { address granter; uint256 value; From 0b88944b38663d040b47056136891d86181958a0 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 3 Apr 2017 22:06:41 -0300 Subject: [PATCH 16/49] fix a few typos --- contracts/token/LimitedTransferToken.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/token/LimitedTransferToken.sol b/contracts/token/LimitedTransferToken.sol index 2bfe873d0..830d31af2 100644 --- a/contracts/token/LimitedTransferToken.sol +++ b/contracts/token/LimitedTransferToken.sol @@ -9,10 +9,10 @@ to limit token transferability for different events. It is intended to be used as a base class for other token contracts. -Over-writting transferableTokens(address holder, uint64 time) is the way to provide -the specific logic for limitting token transferability for a holder over time. +Overwriting transferableTokens(address holder, uint64 time) is the way to provide +the specific logic for limiting token transferability for a holder over time. -LimitedTransferToken has been designed to allow for different limitting factors, +LimitedTransferToken has been designed to allow for different limiting factors, this can be achieved by recursively calling super.transferableTokens() until the base class is hit. For example: From f0e7396619ea8f4cb33e1a727c07da3215708780 Mon Sep 17 00:00:00 2001 From: Roderik van der Veer Date: Wed, 5 Apr 2017 08:33:30 +0200 Subject: [PATCH 17/49] Shareable is not used in this contract --- contracts/DayLimit.sol | 4 ---- 1 file changed, 4 deletions(-) diff --git a/contracts/DayLimit.sol b/contracts/DayLimit.sol index 6cdf983fe..2e22fbcb0 100644 --- a/contracts/DayLimit.sol +++ b/contracts/DayLimit.sol @@ -1,9 +1,5 @@ pragma solidity ^0.4.8; - -import './ownership/Shareable.sol'; - - /* * DayLimit * From 41d2fde95268908fc8aebb95ff88f6a45fe5919b Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Wed, 5 Apr 2017 13:13:17 +0200 Subject: [PATCH 18/49] Fix vesting calculation logic --- contracts/token/VestedToken.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/token/VestedToken.sol b/contracts/token/VestedToken.sol index da350584b..9c0766945 100644 --- a/contracts/token/VestedToken.sol +++ b/contracts/token/VestedToken.sol @@ -94,7 +94,7 @@ contract VestedToken is StandardToken, LimitedTransferToken { if (time < cliff) { return 0; } - if (time > vesting) { + if (time >= vesting) { return tokens; } @@ -103,7 +103,7 @@ contract VestedToken is StandardToken, LimitedTransferToken { uint256 vestingTokens = safeSub(tokens, cliffTokens); - vestedTokens = safeAdd(vestedTokens, safeDiv(safeMul(vestingTokens, safeSub(time, cliff)), safeSub(vesting, start))); + vestedTokens = safeAdd(vestedTokens, safeDiv(safeMul(vestingTokens, safeSub(time, cliff)), safeSub(vesting, cliff))); } function nonVestedTokens(TokenGrant grant, uint64 time) private constant returns (uint256) { From 7a2fda9076a68d47200b56bc8c678cb482bebf7a Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Wed, 5 Apr 2017 13:20:18 +0200 Subject: [PATCH 19/49] Improve VestedToken tests --- test/VestedToken.js | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/test/VestedToken.js b/test/VestedToken.js index 4fefa1ad8..50ec554f9 100644 --- a/test/VestedToken.js +++ b/test/VestedToken.js @@ -40,7 +40,7 @@ contract('VestedToken', function(accounts) { }) it('all tokens are transferable after vesting', async () => { - assert.equal(await token.transferableTokens(receiver, now + vesting + 1), tokenAmount); + assert.equal(await token.transferableTokens(receiver, now + vesting), tokenAmount); }) it('throws when trying to transfer non vested tokens', async () => { @@ -84,16 +84,34 @@ contract('VestedToken', function(accounts) { }) it('can transfer all tokens after vesting ends', async () => { - await timer(vesting + 1); + await timer(vesting); await token.transfer(accounts[7], tokenAmount, { from: receiver }) assert.equal(await token.balanceOf(accounts[7]), tokenAmount); }) it('can approve and transferFrom all tokens after vesting ends', async () => { - await timer(vesting + 1); + await timer(vesting); await token.approve(accounts[7], tokenAmount, { from: receiver }) await token.transferFrom(receiver, accounts[7], tokenAmount, { from: accounts[7] }) assert.equal(await token.balanceOf(accounts[7]), tokenAmount); }) + + it('can handle composed vesting schedules', async () => { + await timer(cliff); + await token.transfer(accounts[7], 12, { from: receiver }) + assert.equal(await token.balanceOf(accounts[7]), 12); + + let newNow = web3.eth.getBlock(web3.eth.blockNumber).timestamp + + await token.grantVestedTokens(receiver, tokenAmount, newNow, newNow + cliff, newNow + vesting, { from: granter }) + await token.transfer(accounts[7], 13, { from: receiver }) + assert.equal(await token.balanceOf(accounts[7]), tokenAmount / 2); + + assert.equal(await token.balanceOf(receiver), 3 * tokenAmount / 2) + assert.equal(await token.transferableTokens(receiver, newNow), 0) + await timer(vesting); + await token.transfer(accounts[7], 3 * tokenAmount / 2, { from: receiver }) + assert.equal(await token.balanceOf(accounts[7]), tokenAmount * 2) + }) }) }); From a1aa74f96d7db09c0d559d6a3bae410ffe35da48 Mon Sep 17 00:00:00 2001 From: Manuel Araoz Date: Mon, 20 Mar 2017 15:42:40 -0300 Subject: [PATCH 20/49] 5.2 all contracts now solidity ^0.4.8 --- contracts/ownership/Claimable.sol | 2 +- contracts/ownership/Contactable.sol | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/ownership/Claimable.sol b/contracts/ownership/Claimable.sol index a99ee5366..e825f28bf 100644 --- a/contracts/ownership/Claimable.sol +++ b/contracts/ownership/Claimable.sol @@ -1,4 +1,4 @@ -pragma solidity ^0.4.0; +pragma solidity ^0.4.8; import './Ownable.sol'; diff --git a/contracts/ownership/Contactable.sol b/contracts/ownership/Contactable.sol index 4c32b749d..f9d5800a1 100644 --- a/contracts/ownership/Contactable.sol +++ b/contracts/ownership/Contactable.sol @@ -1,4 +1,4 @@ -pragma solidity ^0.4.0; +pragma solidity ^0.4.8; import './Ownable.sol'; /* From 52120a8c428de5e34f157b7eaed16d38f3029e66 Mon Sep 17 00:00:00 2001 From: Manuel Araoz Date: Mon, 20 Mar 2017 17:01:18 -0300 Subject: [PATCH 21/49] 5.3 always throw on error --- contracts/lifecycle/Pausable.sol | 10 ++++++---- contracts/ownership/Shareable.sol | 4 +++- contracts/token/ERC20.sol | 13 ++++++------- contracts/token/StandardToken.sol | 22 ++++------------------ test/Pausable.js | 17 +++++++++++++---- 5 files changed, 32 insertions(+), 34 deletions(-) diff --git a/contracts/lifecycle/Pausable.sol b/contracts/lifecycle/Pausable.sol index 9ac044a7f..ad7d66089 100644 --- a/contracts/lifecycle/Pausable.sol +++ b/contracts/lifecycle/Pausable.sol @@ -13,15 +13,17 @@ contract Pausable is Ownable { bool public stopped; modifier stopInEmergency { - if (!stopped) { - _; + if (stopped) { + throw; } + _; } modifier onlyInEmergency { - if (stopped) { - _; + if (!stopped) { + throw; } + _; } // called by the owner on emergency, triggers stopped state diff --git a/contracts/ownership/Shareable.sol b/contracts/ownership/Shareable.sol index 62fbc2fee..0cbf7d37d 100644 --- a/contracts/ownership/Shareable.sol +++ b/contracts/ownership/Shareable.sol @@ -105,12 +105,13 @@ contract Shareable { return !(pending.ownersDone & ownerIndexBit == 0); } + // returns true when operation can be executed function confirmAndCheck(bytes32 _operation) internal returns (bool) { // determine what index the present sender is: uint index = ownerIndex[msg.sender]; // make sure they're an owner if (index == 0) { - return; + throw; } var pending = pendings[_operation]; @@ -140,6 +141,7 @@ contract Shareable { pending.ownersDone |= ownerIndexBit; } } + return false; } function clearPending() internal { diff --git a/contracts/token/ERC20.sol b/contracts/token/ERC20.sol index e89c66caa..6efe7b5d4 100644 --- a/contracts/token/ERC20.sol +++ b/contracts/token/ERC20.sol @@ -1,18 +1,17 @@ pragma solidity ^0.4.8; +import './ERC20Basic.sol'; + + /* * ERC20 interface * see https://github.com/ethereum/EIPs/issues/20 */ -contract ERC20 { - uint public totalSupply; - function balanceOf(address who) constant returns (uint); +contract ERC20 is ERC20Basic { function allowance(address owner, address spender) constant returns (uint); - function transfer(address to, uint value) returns (bool ok); - function transferFrom(address from, address to, uint value) returns (bool ok); - function approve(address spender, uint value) returns (bool ok); - event Transfer(address indexed from, address indexed to, uint value); + function transferFrom(address from, address to, uint value); + function approve(address spender, uint value); event Approval(address indexed owner, address indexed spender, uint value); } diff --git a/contracts/token/StandardToken.sol b/contracts/token/StandardToken.sol index dbf1bb393..9be7d0eeb 100644 --- a/contracts/token/StandardToken.sol +++ b/contracts/token/StandardToken.sol @@ -1,8 +1,8 @@ pragma solidity ^0.4.8; +import './BasicToken.sol'; import './ERC20.sol'; -import '../SafeMath.sol'; /** @@ -12,19 +12,11 @@ import '../SafeMath.sol'; * Based on code by FirstBlood: * https://github.com/Firstbloodio/token/blob/master/smart_contract/FirstBloodToken.sol */ -contract StandardToken is ERC20, SafeMath { +contract StandardToken is BasicToken, ERC20 { - mapping(address => uint) balances; mapping (address => mapping (address => uint)) allowed; - function transfer(address _to, uint _value) returns (bool success) { - balances[msg.sender] = safeSub(balances[msg.sender], _value); - balances[_to] = safeAdd(balances[_to], _value); - Transfer(msg.sender, _to, _value); - return true; - } - - function transferFrom(address _from, address _to, uint _value) returns (bool success) { + function transferFrom(address _from, address _to, uint _value) { var _allowance = allowed[_from][msg.sender]; // Check is not needed because safeSub(_allowance, _value) will already throw if this condition is not met @@ -34,17 +26,11 @@ contract StandardToken is ERC20, SafeMath { balances[_from] = safeSub(balances[_from], _value); allowed[_from][msg.sender] = safeSub(_allowance, _value); Transfer(_from, _to, _value); - return true; - } - - function balanceOf(address _owner) constant returns (uint balance) { - return balances[_owner]; } - function approve(address _spender, uint _value) returns (bool success) { + function approve(address _spender, uint _value) { allowed[msg.sender][_spender] = _value; Approval(msg.sender, _spender, _value); - return true; } function allowance(address _owner, address _spender) constant returns (uint remaining) { diff --git a/test/Pausable.js b/test/Pausable.js index 1625871b0..3ac39e55a 100644 --- a/test/Pausable.js +++ b/test/Pausable.js @@ -1,6 +1,7 @@ 'use strict'; -var PausableMock = artifacts.require('helpers/PausableMock.sol'); +const assertJump = require('./helpers/assertJump'); +const PausableMock = artifacts.require('helpers/PausableMock.sol'); contract('Pausable', function(accounts) { @@ -20,7 +21,11 @@ contract('Pausable', function(accounts) { let count0 = await Pausable.count(); assert.equal(count0, 0); - await Pausable.normalProcess(); + try { + await Pausable.normalProcess(); + } catch(error) { + assertJump(error); + } let count1 = await Pausable.count(); assert.equal(count1, 0); }); @@ -28,9 +33,13 @@ contract('Pausable', function(accounts) { it('can not take drastic measure in non-emergency', async function() { let Pausable = await PausableMock.new(); - await Pausable.drasticMeasure(); - let drasticMeasureTaken = await Pausable.drasticMeasureTaken(); + try { + await Pausable.drasticMeasure(); + } catch(error) { + assertJump(error); + } + const drasticMeasureTaken = await Pausable.drasticMeasureTaken(); assert.isFalse(drasticMeasureTaken); }); From ab9eecb104ae948bf406f301ecb4f323a88ff131 Mon Sep 17 00:00:00 2001 From: Manuel Araoz Date: Mon, 20 Mar 2017 17:18:41 -0300 Subject: [PATCH 22/49] 6.1 fix stuck Ether in Crowdsale contract --- contracts/token/CrowdsaleToken.sol | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/contracts/token/CrowdsaleToken.sol b/contracts/token/CrowdsaleToken.sol index c10d08808..062bf9d34 100644 --- a/contracts/token/CrowdsaleToken.sol +++ b/contracts/token/CrowdsaleToken.sol @@ -8,15 +8,20 @@ import "./StandardToken.sol"; * CrowdsaleToken * * Simple ERC20 Token example, with crowdsale token creation + * IMPORTANT NOTE: do not use or deploy this contract as-is. + * It needs some changes to be production ready. */ contract CrowdsaleToken is StandardToken { - string public name = "CrowdsaleToken"; - string public symbol = "CRW"; - uint public decimals = 18; + string public constant name = "CrowdsaleToken"; + string public constant symbol = "CRW"; + uint public constant decimals = 18; + // replace with your fund collection multisig address + address public constant multisig = 0x0; + // 1 ether = 500 example tokens - uint PRICE = 500; + uint public constant PRICE = 500; function () payable { createTokens(msg.sender); @@ -28,9 +33,13 @@ contract CrowdsaleToken is StandardToken { } uint tokens = safeMul(msg.value, getPrice()); - totalSupply = safeAdd(totalSupply, tokens); + balances[recipient] = safeAdd(balances[recipient], tokens); + + if (!multisig.send(msg.value)) { + throw; + } } // replace this with any other price function From 582166d3788d088b73c623b4db172239d01e3932 Mon Sep 17 00:00:00 2001 From: Manuel Araoz Date: Mon, 20 Mar 2017 18:04:31 -0300 Subject: [PATCH 23/49] 7.1 pull payment safe math --- contracts/payment/PullPayment.sol | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/contracts/payment/PullPayment.sol b/contracts/payment/PullPayment.sol index 7a5ce3f48..3e03f24b8 100644 --- a/contracts/payment/PullPayment.sol +++ b/contracts/payment/PullPayment.sol @@ -1,17 +1,20 @@ pragma solidity ^0.4.8; +import '../SafeMath.sol' + + /* * PullPayment * Base contract supporting async send for pull payments. * Inherit from this contract and use asyncSend instead of send. */ -contract PullPayment { +contract PullPayment is SafeMath { mapping(address => uint) public payments; // store sent amount as credit to be pulled, called by payer function asyncSend(address dest, uint amount) internal { - payments[dest] += amount; + payments[dest] = safeAdd(payments[dest], amount); } // withdraw accumulated balance, called by payee From cd9f820b85f2d17a5a8f5bba091afa0bca20cc4f Mon Sep 17 00:00:00 2001 From: Manuel Araoz Date: Mon, 20 Mar 2017 18:16:02 -0300 Subject: [PATCH 24/49] fix syntax error --- contracts/payment/PullPayment.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/payment/PullPayment.sol b/contracts/payment/PullPayment.sol index 3e03f24b8..130a7b0d6 100644 --- a/contracts/payment/PullPayment.sol +++ b/contracts/payment/PullPayment.sol @@ -1,7 +1,7 @@ pragma solidity ^0.4.8; -import '../SafeMath.sol' +import '../SafeMath.sol'; /* From fad287007b90b9259041b94c2f23bd3afee6b812 Mon Sep 17 00:00:00 2001 From: Manuel Araoz Date: Mon, 20 Mar 2017 18:18:32 -0300 Subject: [PATCH 25/49] 7.2 add preconditions to Shareable contructor --- contracts/ownership/Shareable.sol | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contracts/ownership/Shareable.sol b/contracts/ownership/Shareable.sol index 0cbf7d37d..8b8477d41 100644 --- a/contracts/ownership/Shareable.sol +++ b/contracts/ownership/Shareable.sol @@ -64,6 +64,9 @@ contract Shareable { ownerIndex[_owners[i]] = 2 + i; } required = _required; + if (required > owners.length) { + throw; + } } // Revokes a prior confirmation of the given operation From 2ccbfea8c5b3ecbe3f316136fbec4e20d02539fc Mon Sep 17 00:00:00 2001 From: Manuel Araoz Date: Mon, 20 Mar 2017 18:22:55 -0300 Subject: [PATCH 26/49] 8.1.2 add comment clarifying Migrations --- contracts/lifecycle/Migrations.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/contracts/lifecycle/Migrations.sol b/contracts/lifecycle/Migrations.sol index 6b1bb0ad4..b503c6d7b 100644 --- a/contracts/lifecycle/Migrations.sol +++ b/contracts/lifecycle/Migrations.sol @@ -4,6 +4,7 @@ pragma solidity ^0.4.8; import '../ownership/Ownable.sol'; +// This is a truffle contract, needed for truffle integration, not meant for use by Zeppelin users. contract Migrations is Ownable { uint public lastCompletedMigration; From 7d83a08658c49d5e7f2a8d5e920984949dc94856 Mon Sep 17 00:00:00 2001 From: Manuel Araoz Date: Mon, 20 Mar 2017 18:31:36 -0300 Subject: [PATCH 27/49] 8.2.3 removed ownable --- contracts/ownership/DelayedClaimable.sol | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/ownership/DelayedClaimable.sol b/contracts/ownership/DelayedClaimable.sol index 349400a6b..77425d805 100644 --- a/contracts/ownership/DelayedClaimable.sol +++ b/contracts/ownership/DelayedClaimable.sol @@ -1,7 +1,6 @@ pragma solidity ^0.4.8; -import './Ownable.sol'; import './Claimable.sol'; @@ -9,7 +8,7 @@ import './Claimable.sol'; * DelayedClaimable * Extension for the Claimable contract, where the ownership needs to be claimed before/after certain block number */ -contract DelayedClaimable is Ownable, Claimable { +contract DelayedClaimable is Claimable { uint public end; uint public start; From af604379d31f7e134a05b5f1c3ac47289228b9f3 Mon Sep 17 00:00:00 2001 From: Manuel Araoz Date: Wed, 5 Apr 2017 18:30:14 -0300 Subject: [PATCH 28/49] rebase conflict fixes --- contracts/token/LimitedTransferToken.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/token/LimitedTransferToken.sol b/contracts/token/LimitedTransferToken.sol index 830d31af2..91d5309e0 100644 --- a/contracts/token/LimitedTransferToken.sol +++ b/contracts/token/LimitedTransferToken.sol @@ -33,12 +33,12 @@ contract LimitedTransferToken is ERC20 { } // Checks modifier and allows transfer if tokens are not locked. - function transfer(address _to, uint _value) canTransfer(msg.sender, _value) returns (bool success) { + function transfer(address _to, uint _value) canTransfer(msg.sender, _value) { return super.transfer(_to, _value); } // Checks modifier and allows transfer if tokens are not locked. - function transferFrom(address _from, address _to, uint _value) canTransfer(_from, _value) returns (bool success) { + function transferFrom(address _from, address _to, uint _value) canTransfer(_from, _value) { return super.transferFrom(_from, _to, _value); } From d9b9ed227b175262234fac0f5bd47e19f9556a9c Mon Sep 17 00:00:00 2001 From: Jerome de Tychey Date: Fri, 7 Apr 2017 11:30:18 +0200 Subject: [PATCH 29/49] fix for short address attack as suggested by /u/izqui9 here https://www.reddit.com/r/ethereum/comments/63s917/worrysome_bug_exploit_with_erc20_token/dfwmhc3/ Attack description: https://blog.golemproject.net/how-to-find-10m-by-just-reading-blockchain-6ae9d39fcd95 --- contracts/token/BasicToken.sol | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/contracts/token/BasicToken.sol b/contracts/token/BasicToken.sol index 053ba899c..9003118a0 100644 --- a/contracts/token/BasicToken.sol +++ b/contracts/token/BasicToken.sol @@ -13,7 +13,15 @@ contract BasicToken is ERC20Basic, SafeMath { mapping(address => uint) balances; - function transfer(address _to, uint _value) { +/* + * Fix for the ERC20 short address attack + */ + modifier onlyPayloadSize(uint size) { + assert(msg.data.length == size + 4); + _; + } + + function transfer(address _to, uint _value) onlyPayloadSize(2 * 32) { balances[msg.sender] = safeSub(balances[msg.sender], _value); balances[_to] = safeAdd(balances[_to], _value); Transfer(msg.sender, _to, _value); From 70b5ffd9286d4fd888082f62dfb6054a52aefadf Mon Sep 17 00:00:00 2001 From: David Knott Date: Thu, 6 Apr 2017 15:05:52 -0600 Subject: [PATCH 30/49] Change killable to destructible and kill to destroy --- contracts/Bounty.sol | 4 +-- contracts/MultisigWallet.sol | 4 +-- contracts/lifecycle/Destructible.sol | 15 +++++++++ contracts/lifecycle/Killable.sol | 15 --------- ...okenKillable.sol => TokenDestructible.sol} | 8 ++--- docs/source/bounty.rst | 4 +-- docs/source/index.rst | 2 +- docs/source/killable.rst | 6 ++-- test/Bounty.js | 4 +-- test/Destructible.js | 18 +++++++++++ test/Killable.js | 18 ----------- test/MultisigWallet.js | 8 ++--- test/TokenDestructible.js | 32 +++++++++++++++++++ test/TokenKillable.js | 32 ------------------- 14 files changed, 85 insertions(+), 85 deletions(-) create mode 100644 contracts/lifecycle/Destructible.sol delete mode 100644 contracts/lifecycle/Killable.sol rename contracts/lifecycle/{TokenKillable.sol => TokenDestructible.sol} (79%) create mode 100644 test/Destructible.js delete mode 100644 test/Killable.js create mode 100644 test/TokenDestructible.js delete mode 100644 test/TokenKillable.js diff --git a/contracts/Bounty.sol b/contracts/Bounty.sol index 73e9fe07d..621d33c11 100644 --- a/contracts/Bounty.sol +++ b/contracts/Bounty.sol @@ -2,7 +2,7 @@ pragma solidity ^0.4.8; import './payment/PullPayment.sol'; -import './lifecycle/Killable.sol'; +import './lifecycle/Destructible.sol'; /* @@ -10,7 +10,7 @@ import './lifecycle/Killable.sol'; * * This bounty will pay out to a researcher if they break invariant logic of the contract. */ -contract Bounty is PullPayment, Killable { +contract Bounty is PullPayment, Destructible { bool public claimed; mapping(address => address) public researchers; diff --git a/contracts/MultisigWallet.sol b/contracts/MultisigWallet.sol index f9249218d..4c26241c1 100644 --- a/contracts/MultisigWallet.sol +++ b/contracts/MultisigWallet.sol @@ -24,8 +24,8 @@ contract MultisigWallet is Multisig, Shareable, DayLimit { Shareable(_owners, _required) DayLimit(_daylimit) { } - // kills the contract sending everything to `_to`. - function kill(address _to) onlymanyowners(keccak256(msg.data)) external { + // destroys the contract sending everything to `_to`. + function destroy(address _to) onlymanyowners(keccak256(msg.data)) external { selfdestruct(_to); } diff --git a/contracts/lifecycle/Destructible.sol b/contracts/lifecycle/Destructible.sol new file mode 100644 index 000000000..21de1869d --- /dev/null +++ b/contracts/lifecycle/Destructible.sol @@ -0,0 +1,15 @@ +pragma solidity ^0.4.8; + + +import "../ownership/Ownable.sol"; + + +/* + * Destructible + * Base contract that can be destroyed by owner. All funds in contract will be sent to the owner. + */ +contract Destructible is Ownable { + function destroy() onlyOwner { + selfdestruct(owner); + } +} diff --git a/contracts/lifecycle/Killable.sol b/contracts/lifecycle/Killable.sol deleted file mode 100644 index 0a1367df8..000000000 --- a/contracts/lifecycle/Killable.sol +++ /dev/null @@ -1,15 +0,0 @@ -pragma solidity ^0.4.8; - - -import "../ownership/Ownable.sol"; - - -/* - * Killable - * Base contract that can be killed by owner. All funds in contract will be sent to the owner. - */ -contract Killable is Ownable { - function kill() onlyOwner { - selfdestruct(owner); - } -} diff --git a/contracts/lifecycle/TokenKillable.sol b/contracts/lifecycle/TokenDestructible.sol similarity index 79% rename from contracts/lifecycle/TokenKillable.sol rename to contracts/lifecycle/TokenDestructible.sol index e0dfbfe71..69c686ff8 100644 --- a/contracts/lifecycle/TokenKillable.sol +++ b/contracts/lifecycle/TokenDestructible.sol @@ -4,18 +4,18 @@ pragma solidity ^0.4.8; import "../ownership/Ownable.sol"; import "../token/ERC20Basic.sol"; -/// @title TokenKillable: +/// @title TokenDestructible: /// @author Remco Bloemen -///.Base contract that can be killed by owner. All funds in contract including +///.Base contract that can be destroyed by owner. All funds in contract including /// listed tokens will be sent to the owner -contract TokenKillable is Ownable { +contract TokenDestructible is Ownable { /// @notice Terminate contract and refund to owner /// @param tokens List of addresses of ERC20 or ERC20Basic token contracts to // refund /// @notice The called token contracts could try to re-enter this contract. // Only supply token contracts you - function kill(address[] tokens) onlyOwner { + function destroy(address[] tokens) onlyOwner { // Transfer tokens to owner for(uint i = 0; i < tokens.length; i++) { diff --git a/docs/source/bounty.rst b/docs/source/bounty.rst index 1bc2173fc..d82707eca 100644 --- a/docs/source/bounty.rst +++ b/docs/source/bounty.rst @@ -55,6 +55,6 @@ If researchers break the contract, they can claim their reward. For each researcher who wants to hack the contract and claims the reward, refer to our `Test `_ for the detail. -Finally, if you manage to protect your contract from security researchers, you can reclaim the bounty funds. To end the bounty, kill the contract so that all the rewards go back to the owner.:: +Finally, if you manage to protect your contract from security researchers, you can reclaim the bounty funds. To end the bounty, destroy the contract so that all the rewards go back to the owner.:: - bounty.kill(); + bounty.destroy(); diff --git a/docs/source/index.rst b/docs/source/index.rst index 42ff0bda6..aaa9b98fb 100644 --- a/docs/source/index.rst +++ b/docs/source/index.rst @@ -27,7 +27,7 @@ The code is open-source, and `available on github initBalance); + }); + +}); diff --git a/test/Killable.js b/test/Killable.js deleted file mode 100644 index 6480bd116..000000000 --- a/test/Killable.js +++ /dev/null @@ -1,18 +0,0 @@ -'use strict'; - -var Killable = artifacts.require('../contracts/lifecycle/Killable.sol'); -require('./helpers/transactionMined.js'); - -contract('Killable', function(accounts) { - - it('should send balance to owner after death', async function() { - let killable = await Killable.new({from: accounts[0], value: web3.toWei('10','ether')}); - let owner = await killable.owner(); - let initBalance = web3.eth.getBalance(owner); - await killable.kill({from: owner}); - let newBalance = web3.eth.getBalance(owner); - - assert.isTrue(newBalance > initBalance); - }); - -}); diff --git a/test/MultisigWallet.js b/test/MultisigWallet.js index f750dca13..9c78ee114 100644 --- a/test/MultisigWallet.js +++ b/test/MultisigWallet.js @@ -22,11 +22,11 @@ contract('MultisigWallet', function(accounts) { let walletBalance = web3.eth.getBalance(wallet.address); let hash = 1234; - //Call kill function from two different owner accounts, satisfying owners required - await wallet.kill(accounts[0], {data: hash}); - let txnHash = await wallet.kill(accounts[0], {from: accounts[1], data: hash}); + //Call destroy function from two different owner accounts, satisfying owners required + await wallet.destroy(accounts[0], {data: hash}); + let txnHash = await wallet.destroy(accounts[0], {from: accounts[1], data: hash}); - //Get balances of owner and wallet after kill function is complete, compare with previous values + //Get balances of owner and wallet after destroy function is complete, compare with previous values let newOwnerBalance = web3.eth.getBalance(accounts[0]); let newWalletBalance = web3.eth.getBalance(wallet.address); diff --git a/test/TokenDestructible.js b/test/TokenDestructible.js new file mode 100644 index 000000000..1f943e3df --- /dev/null +++ b/test/TokenDestructible.js @@ -0,0 +1,32 @@ +'use strict'; + +var TokenDestructible = artifacts.require('../contracts/lifecycle/TokenDestructible.sol'); +var StandardTokenMock = artifacts.require("./helpers/StandardTokenMock.sol"); +require('./helpers/transactionMined.js'); + +contract('TokenDestructible', function(accounts) { + + it('should send balance to owner after destruction', async function() { + let destructible = await TokenDestructible.new({from: accounts[0], value: web3.toWei('10','ether')}); + let owner = await destructible.owner(); + let initBalance = web3.eth.getBalance(owner); + await destructible.destroy([], {from: owner}); + let newBalance = web3.eth.getBalance(owner); + assert.isTrue(newBalance > initBalance); + }); + + it('should send tokens to owner after destruction', async function() { + let destructible = await TokenDestructible.new({from: accounts[0], value: web3.toWei('10','ether')}); + let owner = await destructible.owner(); + let token = await StandardTokenMock.new(destructible.address, 100); + let initContractBalance = await token.balanceOf(destructible.address); + let initOwnerBalance = await token.balanceOf(owner); + assert.equal(initContractBalance, 100); + assert.equal(initOwnerBalance, 0); + await destructible.destroy([token.address], {from: owner}); + let newContractBalance = await token.balanceOf(destructible.address); + let newOwnerBalance = await token.balanceOf(owner); + assert.equal(newContractBalance, 0); + assert.equal(newOwnerBalance, 100); + }); +}); diff --git a/test/TokenKillable.js b/test/TokenKillable.js deleted file mode 100644 index 88b06c1b0..000000000 --- a/test/TokenKillable.js +++ /dev/null @@ -1,32 +0,0 @@ -'use strict'; - -var TokenKillable = artifacts.require('../contracts/lifecycle/TokenKillable.sol'); -var StandardTokenMock = artifacts.require("./helpers/StandardTokenMock.sol"); -require('./helpers/transactionMined.js'); - -contract('TokenKillable', function(accounts) { - - it('should send balance to owner after death', async function() { - let killable = await TokenKillable.new({from: accounts[0], value: web3.toWei('10','ether')}); - let owner = await killable.owner(); - let initBalance = web3.eth.getBalance(owner); - await killable.kill([], {from: owner}); - let newBalance = web3.eth.getBalance(owner); - assert.isTrue(newBalance > initBalance); - }); - - it('should send tokens to owner after death', async function() { - let killable = await TokenKillable.new({from: accounts[0], value: web3.toWei('10','ether')}); - let owner = await killable.owner(); - let token = await StandardTokenMock.new(killable.address, 100); - let initContractBalance = await token.balanceOf(killable.address); - let initOwnerBalance = await token.balanceOf(owner); - assert.equal(initContractBalance, 100); - assert.equal(initOwnerBalance, 0); - await killable.kill([token.address], {from: owner}); - let newContractBalance = await token.balanceOf(killable.address); - let newOwnerBalance = await token.balanceOf(owner); - assert.equal(newContractBalance, 0); - assert.equal(newOwnerBalance, 100); - }); -}); From a605f66972539d24622d00fd866ba9e2baae6183 Mon Sep 17 00:00:00 2001 From: David Knott Date: Thu, 6 Apr 2017 14:07:54 -0600 Subject: [PATCH 31/49] Create and test MintableToken contract --- contracts/token/MintableToken.sol | 41 +++++++++++++++++++++++++++++++ test/MintableToken.js | 35 ++++++++++++++++++++++++++ 2 files changed, 76 insertions(+) create mode 100644 contracts/token/MintableToken.sol create mode 100644 test/MintableToken.js diff --git a/contracts/token/MintableToken.sol b/contracts/token/MintableToken.sol new file mode 100644 index 000000000..e3a5f8291 --- /dev/null +++ b/contracts/token/MintableToken.sol @@ -0,0 +1,41 @@ +pragma solidity ^0.4.8; + + +import './StandardToken.sol'; +import '../ownership/Ownable.sol'; + + + +/** + * Mintable token + * + * Simple ERC20 Token example, with mintable token creation + * Issue: + * https://github.com/OpenZeppelin/zeppelin-solidity/issues/120 + * Based on code by TokenMarketNet: + * https://github.com/TokenMarketNet/ico/blob/master/contracts/MintableToken.sol + */ + +contract MintableToken is StandardToken, Ownable { + event Mint(address indexed to, uint value); + + bool public mintingFinished = false; + uint public totalSupply = 0; + + modifier canMint() { + if(mintingFinished) throw; + _; + } + + function mint(address _to, uint _amount) onlyOwner canMint returns (bool) { + totalSupply = safeAdd(totalSupply, _amount); + balances[_to] = safeAdd(balances[_to], _amount); + Mint(_to, _amount); + return true; + } + + function finishMinting() onlyOwner returns (bool) { + mintingFinished = true; + return true; + } +} \ No newline at end of file diff --git a/test/MintableToken.js b/test/MintableToken.js new file mode 100644 index 000000000..4f574d51d --- /dev/null +++ b/test/MintableToken.js @@ -0,0 +1,35 @@ +'use strict'; + +const assertJump = require('./helpers/assertJump'); +var MintableToken = artifacts.require('../contracts/Tokens/MintableToken.sol'); + +contract('Mintable', function(accounts) { + let token; + + beforeEach(async function() { + token = await MintableToken.new(); + }); + + it('should start with a totalSupply of 0', async function() { + let totalSupply = await token.totalSupply(); + + assert.equal(totalSupply, 0); + }); + + it('should return mintingFinished false after construction', async function() { + let mintingFinished = await token.mintingFinished(); + + assert.equal(mintingFinished, false); + }); + + it('should mint a given amount of tokens to a given address', async function() { + await token.mint(accounts[0], 100); + + let balance0 = await token.balanceOf(accounts[0]); + assert(balance0, 100); + + let totalSupply = await token.totalSupply(); + assert(totalSupply, 100); + }) + +}); From 5d75264f0f5a552ec994266cd8691fadfa422252 Mon Sep 17 00:00:00 2001 From: Jerome de Tychey Date: Tue, 11 Apr 2017 17:25:03 +0200 Subject: [PATCH 32/49] Fix for bigger payloads as suggested by izqui in case a function calls transfer under the hood --- contracts/token/BasicToken.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/token/BasicToken.sol b/contracts/token/BasicToken.sol index 9003118a0..5e6f3278a 100644 --- a/contracts/token/BasicToken.sol +++ b/contracts/token/BasicToken.sol @@ -17,7 +17,7 @@ contract BasicToken is ERC20Basic, SafeMath { * Fix for the ERC20 short address attack */ modifier onlyPayloadSize(uint size) { - assert(msg.data.length == size + 4); + assert(msg.data.length >= size + 4); _; } From 31c05c4c7d486e849fea65d55eca08ef508eda95 Mon Sep 17 00:00:00 2001 From: Oleksii Matiiasevych Date: Wed, 12 Apr 2017 15:22:52 +0800 Subject: [PATCH 33/49] Remove excessive condition from SafeMath.safeAdd() There is no situation when `c>=a` will be `true` while `c>=b` will be `false`. --- contracts/SafeMath.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/SafeMath.sol b/contracts/SafeMath.sol index 0022768d4..869c3139a 100644 --- a/contracts/SafeMath.sol +++ b/contracts/SafeMath.sol @@ -25,7 +25,7 @@ contract SafeMath { function safeAdd(uint a, uint b) internal returns (uint) { uint c = a + b; - assert(c>=a && c>=b); + assert(c >= a); return c; } From 37c6782f9534804ca2d801d5b814b7799808d62f Mon Sep 17 00:00:00 2001 From: tatiesmars Date: Thu, 13 Apr 2017 14:23:41 +0800 Subject: [PATCH 34/49] Add MintFinished event to MintableToken #191 --- contracts/token/MintableToken.sol | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/contracts/token/MintableToken.sol b/contracts/token/MintableToken.sol index e3a5f8291..30235a815 100644 --- a/contracts/token/MintableToken.sol +++ b/contracts/token/MintableToken.sol @@ -18,6 +18,7 @@ import '../ownership/Ownable.sol'; contract MintableToken is StandardToken, Ownable { event Mint(address indexed to, uint value); + event MintFinished(); bool public mintingFinished = false; uint public totalSupply = 0; @@ -36,6 +37,7 @@ contract MintableToken is StandardToken, Ownable { function finishMinting() onlyOwner returns (bool) { mintingFinished = true; + MintFinished(); return true; } -} \ No newline at end of file +} From a8bcb0fcfeb5150d23c6d4b18cc7e275e09bfe9a Mon Sep 17 00:00:00 2001 From: AugustoL Date: Fri, 14 Apr 2017 14:30:31 -0300 Subject: [PATCH 35/49] Added totalPayments uint on PullPayments contract --- contracts/payment/PullPayment.sol | 5 ++++- test/PullPayment.js | 13 +++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/contracts/payment/PullPayment.sol b/contracts/payment/PullPayment.sol index 130a7b0d6..694b68837 100644 --- a/contracts/payment/PullPayment.sol +++ b/contracts/payment/PullPayment.sol @@ -11,17 +11,19 @@ import '../SafeMath.sol'; */ contract PullPayment is SafeMath { mapping(address => uint) public payments; + uint public totalPayments; // store sent amount as credit to be pulled, called by payer function asyncSend(address dest, uint amount) internal { payments[dest] = safeAdd(payments[dest], amount); + totalPayments = safeAdd(totalPayments, amount); } // withdraw accumulated balance, called by payee function withdrawPayments() { address payee = msg.sender; uint payment = payments[payee]; - + if (payment == 0) { throw; } @@ -30,6 +32,7 @@ contract PullPayment is SafeMath { throw; } + totalPayments = safeSub(totalPayments, payment); payments[payee] = 0; if (!payee.send(payment)) { diff --git a/test/PullPayment.js b/test/PullPayment.js index 1953d892b..f8536d732 100644 --- a/test/PullPayment.js +++ b/test/PullPayment.js @@ -12,7 +12,9 @@ contract('PullPayment', function(accounts) { let ppce = await PullPaymentMock.new(); let callSend = await ppce.callSend(accounts[0], AMOUNT); let paymentsToAccount0 = await ppce.payments(accounts[0]); + let totalPayments = await ppce.totalPayments(); + assert.equal(totalPayments, AMOUNT); assert.equal(paymentsToAccount0, AMOUNT); }); @@ -21,7 +23,9 @@ contract('PullPayment', function(accounts) { let call1 = await ppce.callSend(accounts[0], 200); let call2 = await ppce.callSend(accounts[0], 300); let paymentsToAccount0 = await ppce.payments(accounts[0]); + let totalPayments = await ppce.totalPayments(); + assert.equal(totalPayments, 500); assert.equal(paymentsToAccount0, 500); }); @@ -35,6 +39,9 @@ contract('PullPayment', function(accounts) { let paymentsToAccount1 = await ppce.payments(accounts[1]); assert.equal(paymentsToAccount1, 300); + + let totalPayments = await ppce.totalPayments(); + assert.equal(totalPayments, 500); }); it("can withdraw payment", async function() { @@ -48,10 +55,16 @@ contract('PullPayment', function(accounts) { let payment1 = await ppce.payments(payee); assert.equal(payment1, AMOUNT); + let totalPayments = await ppce.totalPayments(); + assert.equal(totalPayments, AMOUNT); + let withdraw = await ppce.withdrawPayments({from: payee}); let payment2 = await ppce.payments(payee); assert.equal(payment2, 0); + totalPayments = await ppce.totalPayments(); + assert.equal(totalPayments, 0); + let balance = web3.eth.getBalance(payee); assert(Math.abs(balance-initialBalance-AMOUNT) < 1e16); }); From 526ed43d619a469c93d5c3e157bc28aac009f8ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20Ar=C3=A1oz?= Date: Wed, 19 Apr 2017 13:49:09 -0300 Subject: [PATCH 36/49] Update README with new slack url --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 662868b16..99e216b04 100644 --- a/README.md +++ b/README.md @@ -46,7 +46,7 @@ Building a distributed application, protocol or organization with OpenZeppelin? - Read documentation: http://zeppelin-solidity.readthedocs.io/en/latest/ -- Ask for help and follow progress at: https://zeppelin-slackin.herokuapp.com/ +- Ask for help and follow progress at: https://slack.openzeppelin.org/ Interested in contributing to OpenZeppelin? From 4fad1505c7b095062ca080440c8090372a814d3b Mon Sep 17 00:00:00 2001 From: Manuel Araoz Date: Thu, 6 Apr 2017 16:43:47 -0300 Subject: [PATCH 37/49] Make SafeMath a library --- contracts/SafeMath.sol | 2 +- contracts/payment/PullPayment.sol | 10 ++++++---- contracts/token/BasicToken.sol | 7 ++++--- contracts/token/CrowdsaleToken.sol | 6 +++--- contracts/token/StandardToken.sol | 6 +++--- contracts/token/VestedToken.sol | 19 ++++++++++--------- test/helpers/SafeMathMock.sol | 8 ++++---- 7 files changed, 31 insertions(+), 27 deletions(-) diff --git a/contracts/SafeMath.sol b/contracts/SafeMath.sol index 869c3139a..64d1e0bc2 100644 --- a/contracts/SafeMath.sol +++ b/contracts/SafeMath.sol @@ -4,7 +4,7 @@ pragma solidity ^0.4.8; /** * Math operations with safety checks */ -contract SafeMath { +library SafeMath { function safeMul(uint a, uint b) internal returns (uint) { uint c = a * b; assert(a == 0 || c / a == b); diff --git a/contracts/payment/PullPayment.sol b/contracts/payment/PullPayment.sol index 694b68837..2284ec112 100644 --- a/contracts/payment/PullPayment.sol +++ b/contracts/payment/PullPayment.sol @@ -9,14 +9,16 @@ import '../SafeMath.sol'; * Base contract supporting async send for pull payments. * Inherit from this contract and use asyncSend instead of send. */ -contract PullPayment is SafeMath { +contract PullPayment { + using SafeMath for uint; + mapping(address => uint) public payments; uint public totalPayments; // store sent amount as credit to be pulled, called by payer function asyncSend(address dest, uint amount) internal { - payments[dest] = safeAdd(payments[dest], amount); - totalPayments = safeAdd(totalPayments, amount); + payments[dest] = payments[dest].safeAdd(amount); + totalPayments = totalPayments.safeAdd(amount); } // withdraw accumulated balance, called by payee @@ -32,7 +34,7 @@ contract PullPayment is SafeMath { throw; } - totalPayments = safeSub(totalPayments, payment); + totalPayments = totalPayments.safeSub(payment); payments[payee] = 0; if (!payee.send(payment)) { diff --git a/contracts/token/BasicToken.sol b/contracts/token/BasicToken.sol index 5e6f3278a..07b941acd 100644 --- a/contracts/token/BasicToken.sol +++ b/contracts/token/BasicToken.sol @@ -9,7 +9,8 @@ import '../SafeMath.sol'; * Basic token * Basic version of StandardToken, with no allowances */ -contract BasicToken is ERC20Basic, SafeMath { +contract BasicToken is ERC20Basic { + using SafeMath for uint; mapping(address => uint) balances; @@ -22,8 +23,8 @@ contract BasicToken is ERC20Basic, SafeMath { } function transfer(address _to, uint _value) onlyPayloadSize(2 * 32) { - balances[msg.sender] = safeSub(balances[msg.sender], _value); - balances[_to] = safeAdd(balances[_to], _value); + balances[msg.sender] = balances[msg.sender].safeSub(_value); + balances[_to] = balances[_to].safeAdd(_value); Transfer(msg.sender, _to, _value); } diff --git a/contracts/token/CrowdsaleToken.sol b/contracts/token/CrowdsaleToken.sol index 062bf9d34..040e5123d 100644 --- a/contracts/token/CrowdsaleToken.sol +++ b/contracts/token/CrowdsaleToken.sol @@ -32,10 +32,10 @@ contract CrowdsaleToken is StandardToken { throw; } - uint tokens = safeMul(msg.value, getPrice()); - totalSupply = safeAdd(totalSupply, tokens); + uint tokens = msg.value.safeMul(getPrice()); + totalSupply = totalSupply.safeAdd(tokens); - balances[recipient] = safeAdd(balances[recipient], tokens); + balances[recipient] = balances[recipient].safeAdd(tokens); if (!multisig.send(msg.value)) { throw; diff --git a/contracts/token/StandardToken.sol b/contracts/token/StandardToken.sol index 9be7d0eeb..d61de6bfb 100644 --- a/contracts/token/StandardToken.sol +++ b/contracts/token/StandardToken.sol @@ -22,9 +22,9 @@ contract StandardToken is BasicToken, ERC20 { // Check is not needed because safeSub(_allowance, _value) will already throw if this condition is not met // if (_value > _allowance) throw; - balances[_to] = safeAdd(balances[_to], _value); - balances[_from] = safeSub(balances[_from], _value); - allowed[_from][msg.sender] = safeSub(_allowance, _value); + balances[_to] = balances[_to].safeAdd(_value); + balances[_from] = balances[_from].safeSub(_value); + allowed[_from][msg.sender] = _allowance.safeSub(_value); Transfer(_from, _to, _value); } diff --git a/contracts/token/VestedToken.sol b/contracts/token/VestedToken.sol index 9c0766945..9c4f4cdda 100644 --- a/contracts/token/VestedToken.sol +++ b/contracts/token/VestedToken.sol @@ -52,8 +52,8 @@ contract VestedToken is StandardToken, LimitedTransferToken { grants[_holder][_grantId] = grants[_holder][grants[_holder].length - 1]; grants[_holder].length -= 1; - balances[msg.sender] = safeAdd(balances[msg.sender], nonVested); - balances[_holder] = safeSub(balances[_holder], nonVested); + balances[msg.sender] = balances[msg.sender].safeAdd(nonVested); + balances[_holder] = balances[_holder].safeSub(nonVested); Transfer(_holder, msg.sender, nonVested); } @@ -98,32 +98,33 @@ contract VestedToken is StandardToken, LimitedTransferToken { return tokens; } - uint256 cliffTokens = safeDiv(safeMul(tokens, safeSub(cliff, start)), safeSub(vesting, start)); + uint256 cliffTokens = tokens.safeMul(cliff.safeSub(start)).safeDiv(vesting.safeSub(start)); vestedTokens = cliffTokens; - uint256 vestingTokens = safeSub(tokens, cliffTokens); + uint256 vestingTokens = tokens.safeSub(cliffTokens); - vestedTokens = safeAdd(vestedTokens, safeDiv(safeMul(vestingTokens, safeSub(time, cliff)), safeSub(vesting, cliff))); + vestedTokens = vestedTokens.safeAdd(vestingTokens.safeMul(time.safeSub(cliff)).safeDiv(vesting.safeSub(cliff))); } function nonVestedTokens(TokenGrant grant, uint64 time) private constant returns (uint256) { - return safeSub(grant.value, vestedTokens(grant, time)); + return grant.value.safeSub(vestedTokens(grant, time)); } function lastTokenIsTransferableDate(address holder) constant public returns (uint64 date) { date = uint64(now); uint256 grantIndex = grants[holder].length; for (uint256 i = 0; i < grantIndex; i++) { - date = max64(grants[holder][i].vesting, date); + date = SafeMath.max64(grants[holder][i].vesting, date); } } function transferableTokens(address holder, uint64 time) constant public returns (uint256 nonVested) { uint256 grantIndex = grants[holder].length; for (uint256 i = 0; i < grantIndex; i++) { - nonVested = safeAdd(nonVested, nonVestedTokens(grants[holder][i], time)); + uint256 current = nonVestedTokens(grants[holder][i], time); + nonVested = nonVested.safeAdd(current); } - return min256(safeSub(balances[holder], nonVested), super.transferableTokens(holder, time)); + return SafeMath.min256(balances[holder].safeSub(nonVested), super.transferableTokens(holder, time)); } } diff --git a/test/helpers/SafeMathMock.sol b/test/helpers/SafeMathMock.sol index e31aefbef..c0be1d70f 100644 --- a/test/helpers/SafeMathMock.sol +++ b/test/helpers/SafeMathMock.sol @@ -4,18 +4,18 @@ pragma solidity ^0.4.8; import '../../contracts/SafeMath.sol'; -contract SafeMathMock is SafeMath { +contract SafeMathMock { uint public result; function multiply(uint a, uint b) { - result = safeMul(a, b); + result = SafeMath.safeMul(a, b); } function subtract(uint a, uint b) { - result = safeSub(a, b); + result = SafeMath.safeSub(a, b); } function add(uint a, uint b) { - result = safeAdd(a, b); + result = SafeMath.safeAdd(a, b); } } From 609869f08732fce8cd946996d326c15889e441c8 Mon Sep 17 00:00:00 2001 From: Manuel Araoz Date: Wed, 19 Apr 2017 16:19:22 -0300 Subject: [PATCH 38/49] change safe* to * --- contracts/SafeMath.sol | 8 ++++---- contracts/payment/PullPayment.sol | 6 +++--- contracts/token/BasicToken.sol | 8 +++++--- contracts/token/CrowdsaleToken.sol | 6 +++--- contracts/token/MintableToken.sol | 4 ++-- contracts/token/StandardToken.sol | 8 ++++---- contracts/token/VestedToken.sol | 16 ++++++++-------- test/helpers/SafeMathMock.sol | 6 +++--- 8 files changed, 32 insertions(+), 30 deletions(-) diff --git a/contracts/SafeMath.sol b/contracts/SafeMath.sol index 64d1e0bc2..0d7c160e5 100644 --- a/contracts/SafeMath.sol +++ b/contracts/SafeMath.sol @@ -5,25 +5,25 @@ pragma solidity ^0.4.8; * Math operations with safety checks */ library SafeMath { - function safeMul(uint a, uint b) internal returns (uint) { + function mul(uint a, uint b) internal returns (uint) { uint c = a * b; assert(a == 0 || c / a == b); return c; } - function safeDiv(uint a, uint b) internal returns (uint) { + function div(uint a, uint b) internal returns (uint) { assert(b > 0); uint c = a / b; assert(a == b * c + a % b); return c; } - function safeSub(uint a, uint b) internal returns (uint) { + function sub(uint a, uint b) internal returns (uint) { assert(b <= a); return a - b; } - function safeAdd(uint a, uint b) internal returns (uint) { + function add(uint a, uint b) internal returns (uint) { uint c = a + b; assert(c >= a); return c; diff --git a/contracts/payment/PullPayment.sol b/contracts/payment/PullPayment.sol index 2284ec112..69e30c1d8 100644 --- a/contracts/payment/PullPayment.sol +++ b/contracts/payment/PullPayment.sol @@ -17,8 +17,8 @@ contract PullPayment { // store sent amount as credit to be pulled, called by payer function asyncSend(address dest, uint amount) internal { - payments[dest] = payments[dest].safeAdd(amount); - totalPayments = totalPayments.safeAdd(amount); + payments[dest] = payments[dest].add(amount); + totalPayments = totalPayments.add(amount); } // withdraw accumulated balance, called by payee @@ -34,7 +34,7 @@ contract PullPayment { throw; } - totalPayments = totalPayments.safeSub(payment); + totalPayments = totalPayments.sub(payment); payments[payee] = 0; if (!payee.send(payment)) { diff --git a/contracts/token/BasicToken.sol b/contracts/token/BasicToken.sol index 07b941acd..58b519f6f 100644 --- a/contracts/token/BasicToken.sol +++ b/contracts/token/BasicToken.sol @@ -18,13 +18,15 @@ contract BasicToken is ERC20Basic { * Fix for the ERC20 short address attack */ modifier onlyPayloadSize(uint size) { - assert(msg.data.length >= size + 4); + if(msg.data.length < size + 4) { + throw; + } _; } function transfer(address _to, uint _value) onlyPayloadSize(2 * 32) { - balances[msg.sender] = balances[msg.sender].safeSub(_value); - balances[_to] = balances[_to].safeAdd(_value); + balances[msg.sender] = balances[msg.sender].sub(_value); + balances[_to] = balances[_to].add(_value); Transfer(msg.sender, _to, _value); } diff --git a/contracts/token/CrowdsaleToken.sol b/contracts/token/CrowdsaleToken.sol index 040e5123d..fad970c1e 100644 --- a/contracts/token/CrowdsaleToken.sol +++ b/contracts/token/CrowdsaleToken.sol @@ -32,10 +32,10 @@ contract CrowdsaleToken is StandardToken { throw; } - uint tokens = msg.value.safeMul(getPrice()); - totalSupply = totalSupply.safeAdd(tokens); + uint tokens = msg.value.mul(getPrice()); + totalSupply = totalSupply.add(tokens); - balances[recipient] = balances[recipient].safeAdd(tokens); + balances[recipient] = balances[recipient].add(tokens); if (!multisig.send(msg.value)) { throw; diff --git a/contracts/token/MintableToken.sol b/contracts/token/MintableToken.sol index 30235a815..cef7ff8d4 100644 --- a/contracts/token/MintableToken.sol +++ b/contracts/token/MintableToken.sol @@ -29,8 +29,8 @@ contract MintableToken is StandardToken, Ownable { } function mint(address _to, uint _amount) onlyOwner canMint returns (bool) { - totalSupply = safeAdd(totalSupply, _amount); - balances[_to] = safeAdd(balances[_to], _amount); + totalSupply = totalSupply.add(_amount); + balances[_to] = balances[_to].add(_amount); Mint(_to, _amount); return true; } diff --git a/contracts/token/StandardToken.sol b/contracts/token/StandardToken.sol index d61de6bfb..ee499b964 100644 --- a/contracts/token/StandardToken.sol +++ b/contracts/token/StandardToken.sol @@ -19,12 +19,12 @@ contract StandardToken is BasicToken, ERC20 { function transferFrom(address _from, address _to, uint _value) { var _allowance = allowed[_from][msg.sender]; - // Check is not needed because safeSub(_allowance, _value) will already throw if this condition is not met + // Check is not needed because sub(_allowance, _value) will already throw if this condition is not met // if (_value > _allowance) throw; - balances[_to] = balances[_to].safeAdd(_value); - balances[_from] = balances[_from].safeSub(_value); - allowed[_from][msg.sender] = _allowance.safeSub(_value); + balances[_to] = balances[_to].add(_value); + balances[_from] = balances[_from].sub(_value); + allowed[_from][msg.sender] = _allowance.sub(_value); Transfer(_from, _to, _value); } diff --git a/contracts/token/VestedToken.sol b/contracts/token/VestedToken.sol index 9c4f4cdda..ded654ebe 100644 --- a/contracts/token/VestedToken.sol +++ b/contracts/token/VestedToken.sol @@ -52,8 +52,8 @@ contract VestedToken is StandardToken, LimitedTransferToken { grants[_holder][_grantId] = grants[_holder][grants[_holder].length - 1]; grants[_holder].length -= 1; - balances[msg.sender] = balances[msg.sender].safeAdd(nonVested); - balances[_holder] = balances[_holder].safeSub(nonVested); + balances[msg.sender] = balances[msg.sender].add(nonVested); + balances[_holder] = balances[_holder].sub(nonVested); Transfer(_holder, msg.sender, nonVested); } @@ -98,16 +98,16 @@ contract VestedToken is StandardToken, LimitedTransferToken { return tokens; } - uint256 cliffTokens = tokens.safeMul(cliff.safeSub(start)).safeDiv(vesting.safeSub(start)); + uint256 cliffTokens = tokens.mul(cliff.sub(start)).div(vesting.sub(start)); vestedTokens = cliffTokens; - uint256 vestingTokens = tokens.safeSub(cliffTokens); + uint256 vestingTokens = tokens.sub(cliffTokens); - vestedTokens = vestedTokens.safeAdd(vestingTokens.safeMul(time.safeSub(cliff)).safeDiv(vesting.safeSub(cliff))); + vestedTokens = vestedTokens.add(vestingTokens.mul(time.sub(cliff)).div(vesting.sub(cliff))); } function nonVestedTokens(TokenGrant grant, uint64 time) private constant returns (uint256) { - return grant.value.safeSub(vestedTokens(grant, time)); + return grant.value.sub(vestedTokens(grant, time)); } function lastTokenIsTransferableDate(address holder) constant public returns (uint64 date) { @@ -122,9 +122,9 @@ contract VestedToken is StandardToken, LimitedTransferToken { uint256 grantIndex = grants[holder].length; for (uint256 i = 0; i < grantIndex; i++) { uint256 current = nonVestedTokens(grants[holder][i], time); - nonVested = nonVested.safeAdd(current); + nonVested = nonVested.add(current); } - return SafeMath.min256(balances[holder].safeSub(nonVested), super.transferableTokens(holder, time)); + return SafeMath.min256(balances[holder].sub(nonVested), super.transferableTokens(holder, time)); } } diff --git a/test/helpers/SafeMathMock.sol b/test/helpers/SafeMathMock.sol index c0be1d70f..f4c6e90f6 100644 --- a/test/helpers/SafeMathMock.sol +++ b/test/helpers/SafeMathMock.sol @@ -8,14 +8,14 @@ contract SafeMathMock { uint public result; function multiply(uint a, uint b) { - result = SafeMath.safeMul(a, b); + result = SafeMath.mul(a, b); } function subtract(uint a, uint b) { - result = SafeMath.safeSub(a, b); + result = SafeMath.sub(a, b); } function add(uint a, uint b) { - result = SafeMath.safeAdd(a, b); + result = SafeMath.add(a, b); } } From 7592122e4df1b0632fcef48e8987c0bd90006437 Mon Sep 17 00:00:00 2001 From: Manuel Araoz Date: Wed, 19 Apr 2017 18:34:53 -0300 Subject: [PATCH 39/49] fix indentation --- contracts/token/BasicToken.sol | 6 +++--- contracts/token/ERC20.sol | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/contracts/token/BasicToken.sol b/contracts/token/BasicToken.sol index 58b519f6f..81192144b 100644 --- a/contracts/token/BasicToken.sol +++ b/contracts/token/BasicToken.sol @@ -14,9 +14,9 @@ contract BasicToken is ERC20Basic { mapping(address => uint) balances; -/* - * Fix for the ERC20 short address attack - */ + /* + * Fix for the ERC20 short address attack + */ modifier onlyPayloadSize(uint size) { if(msg.data.length < size + 4) { throw; diff --git a/contracts/token/ERC20.sol b/contracts/token/ERC20.sol index 6efe7b5d4..2116ab95b 100644 --- a/contracts/token/ERC20.sol +++ b/contracts/token/ERC20.sol @@ -10,7 +10,6 @@ import './ERC20Basic.sol'; */ contract ERC20 is ERC20Basic { function allowance(address owner, address spender) constant returns (uint); - function transferFrom(address from, address to, uint value); function approve(address spender, uint value); event Approval(address indexed owner, address indexed spender, uint value); From cd78c20e0ea368737c728e64232c428ffa8ad5d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=20Eklo=CC=88f?= Date: Fri, 21 Apr 2017 20:08:17 +0300 Subject: [PATCH 40/49] await event handling result in Bounty test. Fixes #202 --- test/Bounty.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/Bounty.js b/test/Bounty.js index 191179c42..286845036 100644 --- a/test/Bounty.js +++ b/test/Bounty.js @@ -52,7 +52,7 @@ contract('Bounty', function(accounts) { let bounty = await SecureTargetBounty.new(); let event = bounty.TargetCreated({}); - event.watch(async function(err, result) { + let watcher = async function(err, result) { event.stopWatching(); if (err) { throw err; } @@ -66,8 +66,8 @@ contract('Bounty', function(accounts) { await bounty.claim(targetAddress, {from:researcher}); assert.isTrue(false); // should never reach here } catch(error) { - let reClaimedBounty = await bounty.claimed.call(); - assert.isFalse(reClaimedBounty); + let reClaimedBounty = await bounty.claimed.call(); + assert.isFalse(reClaimedBounty); } try { @@ -77,8 +77,9 @@ contract('Bounty', function(accounts) { assert.equal(reward, web3.eth.getBalance(bounty.address).toNumber()); } - }); + }; bounty.createTarget({from:researcher}); + await awaitEvent(event, watcher); }); }); From b4b6029f6674302d83d100670414ad21e3d44000 Mon Sep 17 00:00:00 2001 From: David Knott Date: Mon, 24 Apr 2017 11:04:42 -0600 Subject: [PATCH 41/49] Create and test PausableToken Contract --- contracts/lifecycle/Pausable.sol | 40 ++++++++-------- contracts/token/PausableToken.sol | 25 ++++++++++ docs/source/pausable.rst | 19 ++++---- test/Pausable.js | 18 ++++---- test/PausableToken.js | 73 ++++++++++++++++++++++++++++++ test/helpers/PausableMock.sol | 4 +- test/helpers/PausableTokenMock.sol | 12 +++++ 7 files changed, 152 insertions(+), 39 deletions(-) create mode 100644 contracts/token/PausableToken.sol create mode 100644 test/PausableToken.js create mode 100644 test/helpers/PausableTokenMock.sol diff --git a/contracts/lifecycle/Pausable.sol b/contracts/lifecycle/Pausable.sol index ad7d66089..9ebefad15 100644 --- a/contracts/lifecycle/Pausable.sol +++ b/contracts/lifecycle/Pausable.sol @@ -6,34 +6,36 @@ import "../ownership/Ownable.sol"; /* * Pausable - * Abstract contract that allows children to implement an - * emergency stop mechanism. + * Abstract contract that allows children to implement a + * pause mechanism. */ contract Pausable is Ownable { - bool public stopped; + event Pause(); + event Unpause(); - modifier stopInEmergency { - if (stopped) { - throw; - } + bool public paused = false; + + modifier whenNotPaused() { + if (paused) throw; _; } - - modifier onlyInEmergency { - if (!stopped) { - throw; - } + + modifier whenPaused { + if (!paused) throw; _; } - // called by the owner on emergency, triggers stopped state - function emergencyStop() external onlyOwner { - stopped = true; + // called by the owner to pause, triggers stopped state + function pause() onlyOwner whenNotPaused returns (bool) { + paused = true; + Pause(); + return true; } - // called by the owner on end of emergency, returns to normal state - function release() external onlyOwner onlyInEmergency { - stopped = false; + // called by the owner to unpause, returns to normal state + function unpause() onlyOwner whenPaused returns (bool) { + paused = false; + Unpause(); + return true; } - } diff --git a/contracts/token/PausableToken.sol b/contracts/token/PausableToken.sol new file mode 100644 index 000000000..f38aad2ba --- /dev/null +++ b/contracts/token/PausableToken.sol @@ -0,0 +1,25 @@ +pragma solidity ^0.4.8; + +import './StandardToken.sol'; +import '../lifecycle/Pausable.sol'; + +/** + * Pausable token + * + * Simple ERC20 Token example, with pausable token creation + * Issue: + * https://github.com/OpenZeppelin/zeppelin-solidity/issues/194 + * Based on code by BCAPtoken: + * https://github.com/BCAPtoken/BCAPToken/blob/5cb5e76338cc47343ba9268663a915337c8b268e/sol/BCAPToken.sol#L27 + **/ + +contract PausableToken is Pausable, StandardToken { + + function transfer(address _to, uint _value) whenNotPaused { + return super.transfer(_to, _value); + } + + function transferFrom(address _from, address _to, uint _value) whenNotPaused { + return super.transferFrom(_from, _to, _value); + } +} \ No newline at end of file diff --git a/docs/source/pausable.rst b/docs/source/pausable.rst index 30c0a8054..fc69ad0b3 100644 --- a/docs/source/pausable.rst +++ b/docs/source/pausable.rst @@ -1,26 +1,27 @@ Pausable ============================================= -Base contract that provides an emergency stop mechanism. +Base contract that provides a pause mechanism. Inherits from contract Ownable. -emergencyStop( ) external onlyOwner +pause() onlyOwner whenNotPaused returns (bool) """"""""""""""""""""""""""""""""""""" -Triggers the stop mechanism on the contract. After this function is called (by the owner of the contract), any function with modifier stopInEmergency will not run. +Triggers pause mechanism on the contract. After this function is called (by the owner of the contract), any function with modifier whenNotPaused will not run. -modifier stopInEmergency + +modifier whenNotPaused() """"""""""""""""""""""""""""""""""""" -Prevents function from running if stop mechanism is activated. +Prevents function from running if pause mechanism is activated. -modifier onlyInEmergency +modifier whenPaused() """"""""""""""""""""""""""""""""""""" -Only runs if stop mechanism is activated. +Only runs if pause mechanism is activated. -release( ) external onlyOwner onlyInEmergency +unpause() onlyOwner whenPaused returns (bool) """""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""" -Deactivates the stop mechanism. \ No newline at end of file +Deactivates the pause mechanism. \ No newline at end of file diff --git a/test/Pausable.js b/test/Pausable.js index 3ac39e55a..2614b3af3 100644 --- a/test/Pausable.js +++ b/test/Pausable.js @@ -5,7 +5,7 @@ const PausableMock = artifacts.require('helpers/PausableMock.sol'); contract('Pausable', function(accounts) { - it('can perform normal process in non-emergency', async function() { + it('can perform normal process in non-pause', async function() { let Pausable = await PausableMock.new(); let count0 = await Pausable.count(); assert.equal(count0, 0); @@ -15,9 +15,9 @@ contract('Pausable', function(accounts) { assert.equal(count1, 1); }); - it('can not perform normal process in emergency', async function() { + it('can not perform normal process in pause', async function() { let Pausable = await PausableMock.new(); - await Pausable.emergencyStop(); + await Pausable.pause(); let count0 = await Pausable.count(); assert.equal(count0, 0); @@ -31,7 +31,7 @@ contract('Pausable', function(accounts) { }); - it('can not take drastic measure in non-emergency', async function() { + it('can not take drastic measure in non-pause', async function() { let Pausable = await PausableMock.new(); try { await Pausable.drasticMeasure(); @@ -43,19 +43,19 @@ contract('Pausable', function(accounts) { assert.isFalse(drasticMeasureTaken); }); - it('can take a drastic measure in an emergency', async function() { + it('can take a drastic measure in a pause', async function() { let Pausable = await PausableMock.new(); - await Pausable.emergencyStop(); + await Pausable.pause(); await Pausable.drasticMeasure(); let drasticMeasureTaken = await Pausable.drasticMeasureTaken(); assert.isTrue(drasticMeasureTaken); }); - it('should resume allowing normal process after emergency is over', async function() { + it('should resume allowing normal process after pause is over', async function() { let Pausable = await PausableMock.new(); - await Pausable.emergencyStop(); - await Pausable.release(); + await Pausable.pause(); + await Pausable.unpause(); await Pausable.normalProcess(); let count0 = await Pausable.count(); diff --git a/test/PausableToken.js b/test/PausableToken.js new file mode 100644 index 000000000..02318999c --- /dev/null +++ b/test/PausableToken.js @@ -0,0 +1,73 @@ +'user strict'; + +const assertJump = require('./helpers/assertJump'); +var PausableTokenMock = artifacts.require('./helpers/PausableTokenMock.sol'); + +contract('PausableToken', function(accounts) { + let token; + + beforeEach(async function() { + token = await PausableTokenMock.new(accounts[0], 100); + }); + + it('should return paused false after construction', async function() { + let paused = await token.paused(); + + assert.equal(paused, false); + }); + + it('should return paused true after pause', async function() { + await token.pause(); + let paused = await token.paused(); + + assert.equal(paused, true); + }); + + it('should return paused false after pause and unpause', async function() { + await token.pause(); + await token.unpause(); + let paused = await token.paused(); + + assert.equal(paused, false); + }); + + it('should be able to transfer if transfers are unpaused', async function() { + await token.transfer(accounts[1], 100); + let balance0 = await token.balanceOf(accounts[0]); + assert.equal(balance0, 0); + + let balance1 = await token.balanceOf(accounts[1]); + assert.equal(balance1, 100); + }); + + it('should be able to transfer after transfers are paused and unpaused', async function() { + await token.pause(); + await token.unpause(); + await token.transfer(accounts[1], 100); + let balance0 = await token.balanceOf(accounts[0]); + assert.equal(balance0, 0); + + let balance1 = await token.balanceOf(accounts[1]); + assert.equal(balance1, 100); + }); + + it('should throw an error trying to transfer while transactions are paused', async function() { + await token.pause(); + try { + await token.transfer(accounts[1], 100); + } catch (error) { + return assertJump(error); + } + assert.fail('should have thrown before'); + }); + + it('should throw an error trying to transfer from another account while transactions are paused', async function() { + await token.pause(); + try { + await token.transferFrom(accounts[0], accounts[1], 100); + } catch (error) { + return assertJump(error); + } + assert.fail('should have thrown before'); + }); +}) \ No newline at end of file diff --git a/test/helpers/PausableMock.sol b/test/helpers/PausableMock.sol index acf21c4e8..80fcb7d2d 100644 --- a/test/helpers/PausableMock.sol +++ b/test/helpers/PausableMock.sol @@ -14,11 +14,11 @@ contract PausableMock is Pausable { count = 0; } - function normalProcess() external stopInEmergency { + function normalProcess() external whenNotPaused { count++; } - function drasticMeasure() external onlyInEmergency { + function drasticMeasure() external whenPaused { drasticMeasureTaken = true; } diff --git a/test/helpers/PausableTokenMock.sol b/test/helpers/PausableTokenMock.sol new file mode 100644 index 000000000..569590d9b --- /dev/null +++ b/test/helpers/PausableTokenMock.sol @@ -0,0 +1,12 @@ +pragma solidity ^0.4.8; + +import '../../contracts/token/PausableToken.sol'; + +// mock class using PausableToken +contract PausableTokenMock is PausableToken { + + function PausableTokenMock(address initialAccount, uint initialBalance) { + balances[initialAccount] = initialBalance; + } + +} From 4de772f5edda4c4af76de65bb1419ab863d02c32 Mon Sep 17 00:00:00 2001 From: Maxim Averin Date: Mon, 8 May 2017 19:34:05 +0300 Subject: [PATCH 42/49] add destroy function, allowing send funds to recepient --- contracts/lifecycle/Destructible.sol | 5 +++++ docs/source/killable.rst | 7 ++++++- test/Destructible.js | 10 +++++++++- 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/contracts/lifecycle/Destructible.sol b/contracts/lifecycle/Destructible.sol index 21de1869d..0f3104949 100644 --- a/contracts/lifecycle/Destructible.sol +++ b/contracts/lifecycle/Destructible.sol @@ -7,9 +7,14 @@ import "../ownership/Ownable.sol"; /* * Destructible * Base contract that can be destroyed by owner. All funds in contract will be sent to the owner. + * In second function all funds will be sent to the recepient. */ contract Destructible is Ownable { function destroy() onlyOwner { selfdestruct(owner); } + + function destroyAndSendRecepient(address _recipient) onlyOwner { + selfdestruct(_recipient); + } } diff --git a/docs/source/killable.rst b/docs/source/killable.rst index af471d9dc..4ceae9a8f 100644 --- a/docs/source/killable.rst +++ b/docs/source/killable.rst @@ -8,4 +8,9 @@ Inherits from contract Ownable. destroy( ) onlyOwner """"""""""""""""""" -Destroys the contract and sends funds back to the owner. \ No newline at end of file +Destroys the contract and sends funds back to the owner. + +destroyAndSendRecepient(address _recipient) onlyOwner +""""""""""""""""""" + +Destroys the contract and sends funds back to the _recepient. \ No newline at end of file diff --git a/test/Destructible.js b/test/Destructible.js index f1a52599e..ae66cfc59 100644 --- a/test/Destructible.js +++ b/test/Destructible.js @@ -11,8 +11,16 @@ contract('Destructible', function(accounts) { let initBalance = web3.eth.getBalance(owner); await destructible.destroy({from: owner}); let newBalance = web3.eth.getBalance(owner); - assert.isTrue(newBalance > initBalance); }); + it('should send balance to recepient after destruction', async function() { + let destructible = await Destructible.new({from: accounts[0], value: web3.toWei('10','ether')}); + let owner = await destructible.owner(); + let initBalance = web3.eth.getBalance(accounts[1]); + await destructible.destroyAndSendRecepient(accounts[1], {from: owner} ); + let newBalance = web3.eth.getBalance(accounts[1]); + assert.isTrue(newBalance.greaterThan(initBalance)); + }); + }); From 7ac697be2c1e599392730d15bf0234fe76df290b Mon Sep 17 00:00:00 2001 From: Manuel Araoz Date: Tue, 9 May 2017 15:13:45 -0300 Subject: [PATCH 43/49] v1.0.5 --- ethpm.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ethpm.json b/ethpm.json index b596dc797..f5e4022c0 100644 --- a/ethpm.json +++ b/ethpm.json @@ -1,6 +1,6 @@ { "package_name": "zeppelin", - "version": "1.0.4", + "version": "1.0.5", "description": "Secure Smart Contract library for Solidity", "authors": [ "Manuel Araoz " diff --git a/package.json b/package.json index d8e3df49e..681a5d499 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "zeppelin-solidity", - "version": "1.0.4", + "version": "1.0.5", "description": "Secure Smart Contract library for Solidity", "main": "truffle.js", "scripts": { From cf7bc06856a641f7043e7c619adf478dbbd4720f Mon Sep 17 00:00:00 2001 From: Maxim Averin Date: Tue, 9 May 2017 23:20:44 +0300 Subject: [PATCH 44/49] Switch function name to destroyAndSend --- contracts/lifecycle/Destructible.sol | 3 +-- docs/source/killable.rst | 2 +- test/Destructible.js | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/contracts/lifecycle/Destructible.sol b/contracts/lifecycle/Destructible.sol index 0f3104949..05d4966f0 100644 --- a/contracts/lifecycle/Destructible.sol +++ b/contracts/lifecycle/Destructible.sol @@ -7,14 +7,13 @@ import "../ownership/Ownable.sol"; /* * Destructible * Base contract that can be destroyed by owner. All funds in contract will be sent to the owner. - * In second function all funds will be sent to the recepient. */ contract Destructible is Ownable { function destroy() onlyOwner { selfdestruct(owner); } - function destroyAndSendRecepient(address _recipient) onlyOwner { + function destroyAndSend(address _recipient) onlyOwner { selfdestruct(_recipient); } } diff --git a/docs/source/killable.rst b/docs/source/killable.rst index 4ceae9a8f..fc6c4ff1e 100644 --- a/docs/source/killable.rst +++ b/docs/source/killable.rst @@ -10,7 +10,7 @@ destroy( ) onlyOwner Destroys the contract and sends funds back to the owner. -destroyAndSendRecepient(address _recipient) onlyOwner +destroyAndSend(address _recipient) onlyOwner """"""""""""""""""" Destroys the contract and sends funds back to the _recepient. \ No newline at end of file diff --git a/test/Destructible.js b/test/Destructible.js index ae66cfc59..34a7d2be5 100644 --- a/test/Destructible.js +++ b/test/Destructible.js @@ -18,7 +18,7 @@ contract('Destructible', function(accounts) { let destructible = await Destructible.new({from: accounts[0], value: web3.toWei('10','ether')}); let owner = await destructible.owner(); let initBalance = web3.eth.getBalance(accounts[1]); - await destructible.destroyAndSendRecepient(accounts[1], {from: owner} ); + await destructible.destroyAndSend(accounts[1], {from: owner} ); let newBalance = web3.eth.getBalance(accounts[1]); assert.isTrue(newBalance.greaterThan(initBalance)); }); From de7751aae2115a482f5bb2e04fd4564020b5c096 Mon Sep 17 00:00:00 2001 From: Manuel Araoz Date: Thu, 11 May 2017 13:08:23 -0300 Subject: [PATCH 45/49] Add external audit --- audit/ZeppelinAudit.md | 290 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 290 insertions(+) create mode 100644 audit/ZeppelinAudit.md diff --git a/audit/ZeppelinAudit.md b/audit/ZeppelinAudit.md new file mode 100644 index 000000000..4590a33a7 --- /dev/null +++ b/audit/ZeppelinAudit.md @@ -0,0 +1,290 @@ +# OpenZeppelin Audit + +March, 2017 +Authored by Dennis Peterson and Peter Vessenes + +# Introduction + +Zeppelin requested that New Alchemy perform an audit of the contracts in their OpenZeppelin library. The OpenZeppelin contracts are a set of contracts intended to be a safe building block for a variety of uses by parties that may not be as sophisticated as the OpenZeppelin team. It is a design goal that the contracts be deployable safely and "as-is". + +The contracts are hosted at: + +https://github.com/OpenZeppelin/zeppelin-solidity + +All the contracts in the "contracts" folder are in scope. + +The git commit hash we evaluated is: +9c5975a706b076b7000e8179f8101e0c61024c87 + +# Disclaimer + +The audit makes no statements or warrantees about utility of the code, safety of the code, suitability of the business model, regulatory regime for the business model, or any other statements about fitness of the contracts to purpose, or their bugfree status. The audit documentation is for discussion purposes only. + +# Executive Summary + +Overall the OpenZeppelin codebase is of reasonably high quality -- it is clean, modular and follows best practices throughout. + +It is still in flux as a codebase, and needs better documentation per file as to expected behavior and future plans. It probably needs more comprehensive and aggressive tests written by people less nice than the current OpenZeppelin team. + +We identified two critical errors and one moderate issue, and would not recommend this commit hash for public use until these bugs are remedied. + +The repository includes a set of Truffle unit tests, a requirement and best practice for smart contracts like these; we recommend these be bulked up. + +# Discussion + +## Big Picture: Is This A Worthwhile Project? + +As soon as a developer touches OpenZeppelin contracts, they will modify something, leaving them in an un-audited state. We do not recommend developers deploy any unaudited code to the Blockchain if it will handle money, information or other things of value. + +> "In accordance with Unix philosophy, Perl gives you enough rope to hang yourself" +> --Larry Wall + +We think this is an incredibly worthwhile project -- aided by the high code quality. Creating a framework that can be easily extended helps increase the average code quality on the Blockchain by charting a course for developers and encouraging containment of modifications to certain sections. + +> "Rust: The language that makes you take the safety off before shooting yourself in the foot" +> -- (@mbrubeck) + +We think much more could be done here, and recommend the OpenZeppelin team keep at this and keep focusing on the design goal of removing rope and adding safety. + +## Solidity Version Updates Recommended + +Most of the code uses Solidity 0.4.8, but some files under `Ownership` are marked 0.4.0. These should be updated. + +Solidity 0.4.10 will add several features which could be useful in these contracts: + +- `assert(condition)`, which throws if the condition is false + +- `revert()`, which rolls back without consuming all remaining gas. + +- `address.transfer(value)`, which is like `send` but automatically propagates exceptions, and supports `.gas()`. See https://github.com/ethereum/solidity/issues/610 for more on this. + +## Error Handling: Throw vs Return False +Solidity standards allow two ways to handle an error -- either calling `throw` or returning `false`. Both have benefits. In particular, a `throw` guarantees a complete wipe of the call stack (up to the preceding external call), whereas `false` allows a function to continue. + +In general we prefer `throw` in our code audits, because it is simpler -- it's less for an engineer to keep track of. Returning `false` and using logic to check results can quickly become a poorly-tracked state machine, and this sort of complexity can cause errors. + +In the OpenZeppelin contracts, both styles are used in different parts of the codebase. `SimpleToken` transfers throw upon failure, while the full ERC20 token returns `false`. Some modifiers `throw`, others just wrap the function body in a conditional, effectively allowing the function to return false if the condition is not met. + +We don't love this, and would usually recommend you stick with one style or the other throughout the codebase. + +In at least one case, these different techniques are combined cleverly (see the Multisig comments, line 65). As a set of contracts intended for general use, we recommend you either strive for more consistency or document explicit design criteria that govern which techniques are used where. + +Note that it may be impossible to use either one in all situations. For example, SafeMath functions pretty much have to throw upon failure, but ERC20 specifies returning booleans. Therefore we make no particular recommendations, but simply point out inconsistencies to consider. + +# Critical Issues + +## Stuck Ether in Crowdsale contract +CrowdsaleToken.sol has no provision for withdrawing the raised ether. We *strongly* recommend a standard `withdraw` function be added. There is no scenario in which someone should deploy this contract as is, whether for testing or live. + +## Recursive Call in MultisigWallet +Line 45 of `MultisigWallet.sol` checks if the amount being sent by `execute` is under a daily limit. + +This function can only be called by the "Owner". As a first angle of attack, it's worth asking what will happen if the multisig wallet owners reset the daily limit by approving a call to `resetSpentToday`. + +If a chain of calls can be constructed in which the owner confirms the `resetSpentToday` function and then withdraws through `execute` in a recursive call, the contract can be drained. In fact, this could be done without a recursive call, just through repeated `execute` calls alternating with the `confirm` calls. + +We are still working through the confirmation protocol in `Shareable.sol`, but we are not convinced that this is impossible, in fact it looks possible. The flexibility any shared owner has in being able to revoke confirmation later is another worrisome angle of approach even if some simple patches are included. + +This bug has a number of causes that need to be addressed: + +1. `resetSpentToday` and `confirm` together do not limit the days on which the function can be called or (it appears) the number of times it can be called. +1. Once a call has been confirmed and `execute`d it appears that it can be re-executed. This is not good. +3. `confirmandCheck` doesn't seem to have logic about whether or not the function in question has been called. +4. Even if it did, `revoke` would need updates and logic to deal with revocation requests after a function call had been completed. + +We do not recommend using the MultisigWallet until these issues are fixed. + +# Moderate to Minor Issues + +## PullPayment +PullPayment.sol needs some work. It has no explicit provision for cancelling a payment. This would be desirable in a number of scenarios; consider a payee losing their wallet, or giving a griefing address, or just an address that requires more than the default gas offered by `send`. + +`asyncSend` has no overflow checking. This is a bad plan. We recommend overflow and underflow checking at the layer closest to the data manipulation. + +`asyncSend` allows more balance to be queued up for sending than the contract holds. This is probably a bad idea, or at the very least should be called something different. If the intent is to allow this, it should have provisions for dealing with race conditions between competing `withdrawPayments` calls. + +It would be nice to see how many payments are pending. This would imply a bit of a rewrite; we recommend this contract get some design time, and that developers don't rely on it in its current state. + +## Shareable Contract + +We do not believe the `Shareable.sol` contract is ready for primetime. It is missing functions, and as written may be vulnerable to a reordering attack -- an attack in which a miner or other party "racing" with a smart contract participant inserts their own information into a list or mapping. + +The confirmation and revocation code needs to be looked over with a very careful eye imagining extraordinarily bad behavior by shared owners before this contract can be called safe. + +No sanity checks on the initial constructor's `required` argument are worrisome as well. + +# Line by Line Comments + +## Lifecycle + +### Killable + +Very simple, allows owner to call selfdestruct, sending funds to owner. No issues. However, note that `selfdestruct` should typically not be used; it is common that a developer may want to access data in a former contract, and they may not understand that `selfdestruct` limits access to the contract. We recommend better documentation about this dynamic, and an alternate function name for `kill` like `completelyDestroy` while `kill` would perhaps merely send funds to the owner. + +Also note that a killable function allows the owner to take funds regardless of other logic. This may be desirable or undesirable depending on the circumstances. Perhaps `Killable` should have a different name as well. + +### Migrations + +I presume that the goal of this contract is to allow and annotate a migration to a new smart contract address. We are not clear here how this would be accomplished by the code; we'd like to review with the OpenZeppelin team. + +### Pausable + +We like these pauses! Note that these allow significant griefing potential by owners, and that this might not be obvious to participants in smart contracts using the OpenZeppelin framework. We would recommend that additional sample logic be added to for instance the TokenContract showing safer use of the pause and resume functions. In particular, we would recommend a timelock after which anyone could unpause the contract. + +The modifers use the pattern `if(bool){_;}`. This is fine for functions that return false upon failure, but could be problematic for functions expected to throw upon failure. See our comments above on standardizing on `throw` or `return(false)`. + +## Ownership + +### Ownable + +Line 19: Modifier throws if doesn't meet condition, in contrast to some other inheritable modifiers (e.g. in Pausable) that use `if(bool){_;}`. + +### Claimable + +Inherits from Ownable but the existing owner sets a pendingOwner who has to claim ownership. + +Line 17: Another modifier that throws. + +### DelayedClaimable + +Is there any reason to descend from Ownable directly, instead of just Claimable, which descends from Ownable? If not, descending from both just adds confusion. + +### Contactable + +Allows owner to set a public string of contract information. No issues. + +### Shareable + +This needs some work. Doesn't check if `_required <= len(_owners)` for instance, that would be a bummer. What if _required were like `MAX - 1`? + +I have a general concern about the difference between `owners`, `_owners`, and `owner` in `Ownable.sol`. I recommend "Owners" be renamed. In general we do not recomment single character differences in variable names, although a preceding underscore is not uncommon in Solidity code. + +Line 34: "this contract only has six types of events"...actually only two. + +Line 61: Why is `ownerIndex` keyed by addresses hashed to `uint`s? Why not use the addresses directly, so `ownerIndex` is less obscure, and so there's stronger typing? + +Line 62: Do not love `++i) ... owners[2+ i]`. Makes me do math, which is not what I want to do. I want to not have to do math. + +There should probably be a function for adding a new operation, so the developer doesn't have to work directly with the internal data. (This would make the multisig contract even shorter.) + +There's a `revoke` function but not a `propose` function that we can see. + +Beware reordering. If `propose` allows the user to choose a bytes string for their proposal, bad things(TM) will happen as currently written. + + +### Multisig + +Just an interface. Note it allows changing an owner address, but not changing the number of owners. This is somewhat limiting but also simplifies implementation. + +## Payment + +### PullPayment + +Safe from reentrance attack since ether send is at the end, plus it uses `.send()` rather than `.call.value()`. + +There's an argument to be made that `.call.value()` is a better option *if* you're sure that it will be done after all state updates, since `.send` will fail if the recipient has an expensive fallback function. However, in the context of a function meant to be embedded in other contracts, it's probably better to use `.send`. One possible compromise is to add a function which allows only the owner to send ether via `.call.value`. + +If you don't use `call.value` you should implement a `cancel` function in case some value is pending here. + +Line 14: +Doesn't use safeAdd. Although it appears that payout amounts can only be increased, in fact the payer could lower the payout as much as desired via overflow. Also, the payer could add a large non-overflowing amount, causing the payment to exceed the contract balance and therefore fail when withdraw is attempted. + +Recommendation: track the sum of non-withdrawn asyncSends, and don't allow a new one which exceeds the leftover balance. If it's ever desirable to make payments revocable, it should be done explicitly. + +## Tokens + +### ERC20 + +Standard ERC20 interface only. + +There's a security hole in the standard, reported at Edcon: `approve` does not protect against race conditions and simply replaces the current value. An approved spender could wait for the owner to call `approve` again, then attempt to spend the old limit before the new limit is applied. If successful, this attacker could successfully spend the sum of both limits. + +This could be fixed by either (1) including the old limit as a parameter, so the update will fail if some gets spent, or (2) using the value parameter as a delta instead of replacement value. + +This is not fixable while adhering to the current full ERC20 standard, though it would be possible to add a "secureApprove" function. The impact isn't extreme since at least you can only be attacked by addresses you approved. Also, users could mitigate this by always setting spending limits to zero and checking for spends, before setting the new limit. + +Edcon slides: +https://drive.google.com/file/d/0ByMtMw2hul0EN3NCaVFHSFdxRzA/view + +### ERC20Basic + +Simpler interface skipping the Approve function. Note this departs from ERC20 in another way: transfer throws instead of returning false. + +### BasicToken + +Uses `SafeSub` and `SafeMath`, so transfer `throw`s instead of returning false. This complies with ERC20Basic but not the actual ERC20 standard. + +### StandardToken + +Implementation of full ERC20 token. + +Transfer() and transferFrom() use SafeMath functions, which will cause them to throw instead of returning false. Not a security issue but departs from standard. + +### SimpleToken + +Sample instantiation of StandardToken. Note that in this sample, decimals is 18 and supply only 10,000, so the supply is a small fraction of a single nominal token. + +### CrowdsaleToken + +StandardToken which mints tokens at a fixed price when sent ether. + +There's no provision for owner withdrawing the ether. As a sample for crowdsales it should be Ownable and allow the owner to withdraw ether, rather than stranding the ether in the contract. + +Note: an alternative pattern is a mint() function which is only callable from a separate crowdsale contract, so any sort of rules can be added without modifying the token itself. + +### VestedToken + +Lines 23, 27: +Functions `transfer()` and `transferFrom()` have a modifier canTransfer which throws if not enough tokens are available. However, transfer() returns a boolean success. Inconsistent treatment of failure conditions may cause problems for other contracts using the token. (Note that transferableTokens() relies on safeSub(), so will also throw if there's insufficient balance.) + +Line 64: +Delete not actually necessary since the value is overwritten in the next line anyway. + +## Root level + +### Bounty + +Avoids potential race condition by having each researcher deploy a separate contract for attack; if a research manages to break his associated contract, other researchers can't immediately claim the reward, they have to reproduce the attack in their own contracts. + +A developer could subvert this intent by implementing `deployContract()` to always return the same address. However, this would break the `researchers` mapping, updating the researcher address associated with the contract. This could be prevented by blocking rewrites in `researchers`. + +### DayLimit + +The modifier `limitedDaily` calls `underLimit`, which both checks that the spend is below the daily limit, and adds the input value to the daily spend. This is fine if all functions throw upon failure. However, not all OpenZeppelin functions do this; there are functions that returns false, and modifiers that wrap the function body in `if (bool) {_;}`. In these cases, `_value` will be added to `spentToday`, but ether may not actually be sent because other preconditions were not met. (However in the OpenZeppelin multisig this is not a problem.) + +Lines 4, 11: +Comment claims that `DayLimit` is multiowned, and Shareable is imported, but DayLimit does not actually inherit from Shareable. The intent may be for child contracts to inherit from Shareable (as Multisig does); in this case the import should be removed and the comment altered. + +Line 46: +Manual overflow check instead of using safeAdd. Since this is called from a function that throws upon failure anyway, there's no real downside to using safeAdd. + +### LimitBalance + +No issues. + +### MultisigWallet + +Lines 28, 76, 80: +`kill`, `setDailyLimit`, and `resetSpentToday` only happen with multisig approval, and hashes for these actions are logged by Shareable. However, they should probably post their own events for easy reading. + +Line 45: +This call to underLimit will reduce the daily limit, and then either throw or return 0. So in this case there's no danger that the limit will be reduced without the operation going through. + +Line 65: +Shareable's onlyManyOwners will take the user's confirmation, and execute the function body if and only if enough users have confirmed. Whole thing throws if the send fails, which will roll back the confirmation. Confirm returns false if not enough have confirmed yet, true if the whole thing succeeds, and throws only in the exceptional circumstance that the designated transaction unexpectedly fails. Elegant design. + +Line 68: +Throw here is good but note this function can fail either by returning false or by throwing. + +Line 92: +A bit odd to split `clearPending()` between this contract and Shareable. However this does allow contracts inheriting from Shareable to use custom structs for pending transactions. + + +### SafeMath + +Another interesting comment from the same Edcon presentation was that the overflow behavior of Solidity is undocumented, so in theory, source code that relies on it could break with a future revision. + +However, compiled code should be fine, and in the unlikely event that the compiler is revised in this way, there should be plenty of warning. (But this is an argument for keeping overflow checks isolated in SafeMath.) + +Aside from that small caveat, these are fine. + From 7826fddba7336e9e70a8d841946013c254b6d402 Mon Sep 17 00:00:00 2001 From: Manuel Araoz Date: Fri, 12 May 2017 12:10:28 -0300 Subject: [PATCH 46/49] revert to usig npm as preferred installation method --- README.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 99e216b04..925801fe4 100644 --- a/README.md +++ b/README.md @@ -22,19 +22,20 @@ truffle init To install the OpenZeppelin library, run: ```sh -truffle install zeppelin +npm install zeppelin-solidity ``` -After that, you'll get all the library's contracts in the `installed_contracts` folder. You can use the contracts in the library like so: +After that, you'll get all the library's contracts in the `node_modules/zeppelin-solidity/contracts` folder. You can use the contracts in the library like so: ```js -import 'zeppelin/ownership/Ownable.sol'; +import 'zeppelin-solidity/contracts/ownership/Ownable.sol'; contract MyContract is Ownable { ... } ``` + ## Security OpenZeppelin is meant to provide secure, tested and community-audited code, but please use common sense when doing anything that deals with real money! We take no responsibility for your implementation decisions and any security problem you might experience. From a3446507ec44d75d57dcfe0f9a971e2160c9709c Mon Sep 17 00:00:00 2001 From: Mikko Ohtamaa Date: Wed, 17 May 2017 01:33:13 +0300 Subject: [PATCH 47/49] Add fix for the approve() mitigation. --- contracts/token/StandardToken.sol | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/contracts/token/StandardToken.sol b/contracts/token/StandardToken.sol index ee499b964..1eb4e114a 100644 --- a/contracts/token/StandardToken.sol +++ b/contracts/token/StandardToken.sol @@ -29,6 +29,13 @@ contract StandardToken is BasicToken, ERC20 { } function approve(address _spender, uint _value) { + + // To change the approve amount you first have to reduce the addresses` + // allowance to zero by calling `approve(_spender,0)` if it is not + // already 0 to mitigate the race condition described here: + // https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729 + if ((_amount!=0) && (allowed[msg.sender][_spender] !=0)) throw; + allowed[msg.sender][_spender] = _value; Approval(msg.sender, _spender, _value); } From e1cf60248756a30cfd2fe74be1120d868b514425 Mon Sep 17 00:00:00 2001 From: Mikko Ohtamaa Date: Wed, 17 May 2017 01:41:43 +0300 Subject: [PATCH 48/49] Fix variable naming. --- contracts/token/StandardToken.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/token/StandardToken.sol b/contracts/token/StandardToken.sol index 1eb4e114a..dcb40053b 100644 --- a/contracts/token/StandardToken.sol +++ b/contracts/token/StandardToken.sol @@ -31,10 +31,10 @@ contract StandardToken is BasicToken, ERC20 { function approve(address _spender, uint _value) { // To change the approve amount you first have to reduce the addresses` - // allowance to zero by calling `approve(_spender,0)` if it is not + // allowance to zero by calling `approve(_spender, 0)` if it is not // already 0 to mitigate the race condition described here: // https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729 - if ((_amount!=0) && (allowed[msg.sender][_spender] !=0)) throw; + if ((_value != 0) && (allowed[msg.sender][_spender] != 0)) throw; allowed[msg.sender][_spender] = _value; Approval(msg.sender, _spender, _value); From 964185dec3fed72b094c4641275d6842fcf691d5 Mon Sep 17 00:00:00 2001 From: Mikko Ohtamaa Date: Thu, 18 May 2017 00:38:41 +0300 Subject: [PATCH 49/49] Protect transferFrom against short hand attack. --- contracts/token/StandardToken.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/token/StandardToken.sol b/contracts/token/StandardToken.sol index dcb40053b..865fe1b3c 100644 --- a/contracts/token/StandardToken.sol +++ b/contracts/token/StandardToken.sol @@ -16,7 +16,7 @@ contract StandardToken is BasicToken, ERC20 { mapping (address => mapping (address => uint)) allowed; - function transferFrom(address _from, address _to, uint _value) { + function transferFrom(address _from, address _to, uint _value) onlyPayloadSize(3 * 32) { var _allowance = allowed[_from][msg.sender]; // Check is not needed because sub(_allowance, _value) will already throw if this condition is not met