Merge pull request #170 from Recmo/feature/no-owner

Prevent contracts from owning Ether, Tokens and other contracts.

contracts/ownership/HasNoContracts.sol

+pragma solidity ^0.4.8;
+
+import "./Ownable.sol";
+
+/// @title Contracts that should not own Contracts
+/// @author Remco Bloemen <remco@2π.com>
+///
+/// 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);
+  }
+}

contracts/ownership/HasNoEther.sol

+pragma solidity ^0.4.8;
+
+import "./Ownable.sol";
+
+/// @title Contracts that should not own Ether
+/// @author Remco Bloemen <remco@2π.com>
+///
+/// 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 `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
+  ///      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;
+    }
+  }
+}

contracts/ownership/HasNoTokens.sol

+pragma solidity ^0.4.8;
+
+import "./Ownable.sol";
+import "../token/ERC20Basic.sol";
+
+/// @title Contracts that should not own Tokens
+/// @author Remco Bloemen <remco@2π.com>
+///
+/// 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);
+  }
+}

contracts/ownership/NoOwner.sol

+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 <remco@2π.com>
+///
+/// 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 {
+}

test/HasNoContracts.js

+'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]}),
+    );
+  });
+});

test/HasNoEther.js

+'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]}));
+  });
+});

test/HasNoTokens.js

+'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]}),
+    );
+  });
+});

test/helpers/ERC23TokenMock.sol

+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;
+  }
+}

test/helpers/ForceEther.sol

+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 <remco@neufund.org>
+contract ForceEther  {
+  function ForceEther(address target) payable {
+    // Selfdestruct transfers all Ether to the arget address
+    selfdestruct(target);
+  }
+}

test/helpers/HasNoEtherTest.sol

+pragma solidity ^0.4.8;
+
+import "../../contracts/ownership/HasNoEther.sol";
+
+contract HasNoEtherTest is HasNoEther {
+
+  // Constructor with explicit payable — should still fail
+  function HasNoEtherTest() payable {
+  }
+
+}

test/helpers/toPromise.js

+export default func =>
+  (...args) =>
+    new Promise((accept, reject) =>
+      func(...args, (error, data) => error ? reject(error) : accept(data)));