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)));