Merge pull request #169 from maraoz/audit

Follow security audit recommendations

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;
 

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

contracts/ownership/Claimable.sol

@@ -1,4 +1,4 @@
-pragma solidity ^0.4.0;
+pragma solidity ^0.4.8;
 
 
 import './Ownable.sol';

contracts/ownership/Contactable.sol

@@ -1,4 +1,4 @@
-pragma solidity ^0.4.0;
+pragma solidity ^0.4.8;
 
 import './Ownable.sol';
 /*

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;

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
@@ -105,12 +108,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 +144,7 @@ contract Shareable {
         pending.ownersDone |= ownerIndexBit;
       }
     }
+    return false;
   }
 
   function clearPending() internal {

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

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

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

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

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

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