From d1af3ef1b3a746ddf3cd0773830ff1887e8f1693 Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Thu, 23 Mar 2017 12:23:27 +0000 Subject: [PATCH 1/6] 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 2/6] 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 3/6] 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 4/6] 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 5/6] 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 1d2b989e8e2a8a9867baf7ff73e190267b40480b Mon Sep 17 00:00:00 2001 From: Remco Bloemen Date: Fri, 31 Mar 2017 09:39:29 +0100 Subject: [PATCH 6/6] 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