Merge pull request #1050 from 0xProject/refactor/contracts/multisig

Update MultiSig and add more tests

6 files changed, 114 insertions, 97 deletions

diff --git a/packages/contracts/src/2.0.0/extensions/Forwarder/MixinExchangeWrapper.sol b/packages/contracts/src/2.0.0/extensions/Forwarder/MixinExchangeWrapper.sol
index 33326c2ab..fea9a53c2 100644
--- a/packages/contracts/src/2.0.0/extensions/Forwarder/MixinExchangeWrapper.sol
+++ b/packages/contracts/src/2.0.0/extensions/Forwarder/MixinExchangeWrapper.sol
@@ -60,7 +60,7 @@ contract MixinExchangeWrapper is
         // Call `fillOrder` and handle any exceptions gracefully
         assembly {
             let success := call(
-                gas,                                // forward all gas, TODO: look into gas consumption of assert/throw
+                gas,                                // forward all gas
                 exchange,                           // call address of Exchange contract
                 0,                                  // transfer 0 wei
                 add(fillOrderCalldata, 32),         // pointer to start of input (skip array length in first 32 bytes)
diff --git a/packages/contracts/src/2.0.0/multisig/MultiSigWallet.sol b/packages/contracts/src/2.0.0/multisig/MultiSigWallet.sol
index eb54fe047..516e7391c 100644
--- a/packages/contracts/src/2.0.0/multisig/MultiSigWallet.sol
+++ b/packages/contracts/src/2.0.0/multisig/MultiSigWallet.sol
@@ -1,13 +1,14 @@
 // solhint-disable
-pragma solidity ^0.4.10;
+pragma solidity ^0.4.15;
 
 
 /// @title Multisignature wallet - Allows multiple parties to agree on transactions before execution.
 /// @author Stefan George - <stefan.george@consensys.net>
 contract MultiSigWallet {
 
-    uint constant public MAX_OWNER_COUNT = 50;
-
+    /*
+     *  Events
+     */
     event Confirmation(address indexed sender, uint indexed transactionId);
     event Revocation(address indexed sender, uint indexed transactionId);
     event Submission(uint indexed transactionId);
@@ -18,6 +19,14 @@ contract MultiSigWallet {
     event OwnerRemoval(address indexed owner);
     event RequirementChange(uint required);
 
+    /*
+     *  Constants
+     */
+    uint constant public MAX_OWNER_COUNT = 50;
+
+    /*
+     *  Storage
+     */
     mapping (uint => Transaction) public transactions;
     mapping (uint => mapping (address => bool)) public confirmations;
     mapping (address => bool) public isOwner;
@@ -32,60 +41,54 @@ contract MultiSigWallet {
         bool executed;
     }
 
+    /*
+     *  Modifiers
+     */
     modifier onlyWallet() {
-        if (msg.sender != address(this))
-            throw;
+        require(msg.sender == address(this));
         _;
     }
 
     modifier ownerDoesNotExist(address owner) {
-        if (isOwner[owner])
-            throw;
+        require(!isOwner[owner]);
         _;
     }
 
     modifier ownerExists(address owner) {
-        if (!isOwner[owner])
-            throw;
+        require(isOwner[owner]);
         _;
     }
 
     modifier transactionExists(uint transactionId) {
-        if (transactions[transactionId].destination == 0)
-            throw;
+        require(transactions[transactionId].destination != 0);
         _;
     }
 
     modifier confirmed(uint transactionId, address owner) {
-        if (!confirmations[transactionId][owner])
-            throw;
+        require(confirmations[transactionId][owner]);
         _;
     }
 
     modifier notConfirmed(uint transactionId, address owner) {
-        if (confirmations[transactionId][owner])
-            throw;
+        require(!confirmations[transactionId][owner]);
         _;
     }
 
     modifier notExecuted(uint transactionId) {
-        if (transactions[transactionId].executed)
-            throw;
+        require(!transactions[transactionId].executed);
         _;
     }
 
     modifier notNull(address _address) {
-        if (_address == 0)
-            throw;
+        require(_address != 0);
         _;
     }
 
     modifier validRequirement(uint ownerCount, uint _required) {
-        if (   ownerCount > MAX_OWNER_COUNT
-            || _required > ownerCount
-            || _required == 0
-            || ownerCount == 0)
-            throw;
+        require(ownerCount <= MAX_OWNER_COUNT
+            && _required <= ownerCount
+            && _required != 0
+            && ownerCount != 0);
         _;
     }
 
@@ -108,8 +111,7 @@ contract MultiSigWallet {
         validRequirement(_owners.length, _required)
     {
         for (uint i=0; i<_owners.length; i++) {
-            if (isOwner[_owners[i]] || _owners[i] == 0)
-                throw;
+            require(!isOwner[_owners[i]] && _owners[i] != 0);
             isOwner[_owners[i]] = true;
         }
         owners = _owners;
@@ -151,7 +153,7 @@ contract MultiSigWallet {
 
     /// @dev Allows to replace an owner with a new owner. Transaction has to be sent by wallet.
     /// @param owner Address of owner to be replaced.
-    /// @param owner Address of new owner.
+    /// @param newOwner Address of new owner.
     function replaceOwner(address owner, address newOwner)
         public
         onlyWallet
@@ -222,20 +224,44 @@ contract MultiSigWallet {
     /// @param transactionId Transaction ID.
     function executeTransaction(uint transactionId)
         public
+        ownerExists(msg.sender)
+        confirmed(transactionId, msg.sender)
         notExecuted(transactionId)
     {
         if (isConfirmed(transactionId)) {
-            Transaction tx = transactions[transactionId];
-            tx.executed = true;
-            if (tx.destination.call.value(tx.value)(tx.data))
+            Transaction storage txn = transactions[transactionId];
+            txn.executed = true;
+            if (external_call(txn.destination, txn.value, txn.data.length, txn.data))
                 Execution(transactionId);
             else {
                 ExecutionFailure(transactionId);
-                tx.executed = false;
+                txn.executed = false;
             }
         }
     }
 
+    // call has been separated into its own function in order to take advantage
+    // of the Solidity's code generator to produce a loop that copies tx.data into memory.
+    function external_call(address destination, uint value, uint dataLength, bytes data) internal returns (bool) {
+        bool result;
+        assembly {
+            let x := mload(0x40)   // "Allocate" memory for output (0x40 is where "free memory" pointer is stored by convention)
+            let d := add(data, 32) // First 32 bytes are the padded length of data, so exclude that
+            result := call(
+                sub(gas, 34710),   // 34710 is the value that solidity is currently emitting
+                                   // It includes callGas (700) + callVeryLow (3, to pay for SUB) + callValueTransferGas (9000) +
+                                   // callNewAccountGas (25000, in case the destination address does not exist and needs creating)
+                destination,
+                value,
+                d,
+                dataLength,        // Size of the input (in bytes) - this is what fixes the padding problem
+                x,
+                0                  // Output is ignored, therefore the output size is zero
+            )
+        }
+        return result;
+    }
+
     /// @dev Returns the confirmation status of a transaction.
     /// @param transactionId Transaction ID.
     /// @return Confirmation status.
@@ -364,4 +390,4 @@ contract MultiSigWallet {
         for (i=from; i<to; i++)
             _transactionIds[i - from] = transactionIdsTemp[i];
     }
-}
+}
\ No newline at end of file
diff --git a/packages/contracts/src/2.0.0/multisig/MultiSigWalletWithTimeLock.sol b/packages/contracts/src/2.0.0/multisig/MultiSigWalletWithTimeLock.sol
index 8c5e6e1e6..9513d3b30 100644
--- a/packages/contracts/src/2.0.0/multisig/MultiSigWalletWithTimeLock.sol
+++ b/packages/contracts/src/2.0.0/multisig/MultiSigWalletWithTimeLock.sol
@@ -16,47 +16,57 @@
 
 */
 
-// solhint-disable
-pragma solidity ^0.4.10;
+pragma solidity 0.4.24;
 
 import "./MultiSigWallet.sol";
 
 
 /// @title Multisignature wallet with time lock- Allows multiple parties to execute a transaction after a time lock has passed.
 /// @author Amir Bandeali - <amir@0xProject.com>
-contract MultiSigWalletWithTimeLock is MultiSigWallet {
-
-    event ConfirmationTimeSet(uint indexed transactionId, uint confirmationTime);
-    event TimeLockChange(uint secondsTimeLocked);
-
-    uint public secondsTimeLocked;
-
-    mapping (uint => uint) public confirmationTimes;
-
-    modifier notFullyConfirmed(uint transactionId) {
-        require(!isConfirmed(transactionId));
+// solhint-disable not-rely-on-time
+contract MultiSigWalletWithTimeLock is
+    MultiSigWallet
+{
+    event ConfirmationTimeSet(uint256 indexed transactionId, uint256 confirmationTime);
+    event TimeLockChange(uint256 secondsTimeLocked);
+
+    uint256 public secondsTimeLocked;
+
+    mapping (uint256 => uint256) public confirmationTimes;
+
+    modifier notFullyConfirmed(uint256 transactionId) {
+        require(
+            !isConfirmed(transactionId),
+            "TX_FULLY_CONFIRMED"
+        );
         _;
     }
 
-    modifier fullyConfirmed(uint transactionId) {
-        require(isConfirmed(transactionId));
+    modifier fullyConfirmed(uint256 transactionId) {
+        require(
+            isConfirmed(transactionId),
+            "TX_NOT_FULLY_CONFIRMED"
+        );
         _;
     }
 
-    modifier pastTimeLock(uint transactionId) {
-        require(block.timestamp >= confirmationTimes[transactionId] + secondsTimeLocked);
+    modifier pastTimeLock(uint256 transactionId) {
+        require(
+            block.timestamp >= confirmationTimes[transactionId] + secondsTimeLocked,
+            "TIME_LOCK_INCOMPLETE"
+        );
         _;
     }
 
-    /*
-     * Public functions
-     */
-
     /// @dev Contract constructor sets initial owners, required number of confirmations, and time lock.
     /// @param _owners List of initial owners.
     /// @param _required Number of required confirmations.
     /// @param _secondsTimeLocked Duration needed after a transaction is confirmed and before it becomes executable, in seconds.
-    function MultiSigWalletWithTimeLock(address[] _owners, uint _required, uint _secondsTimeLocked)
+    constructor (
+        address[] _owners,
+        uint256 _required,
+        uint256 _secondsTimeLocked
+    )
         public
         MultiSigWallet(_owners, _required)
     {
@@ -65,17 +75,17 @@ contract MultiSigWalletWithTimeLock is MultiSigWallet {
 
     /// @dev Changes the duration of the time lock for transactions.
     /// @param _secondsTimeLocked Duration needed after a transaction is confirmed and before it becomes executable, in seconds.
-    function changeTimeLock(uint _secondsTimeLocked)
+    function changeTimeLock(uint256 _secondsTimeLocked)
         public
         onlyWallet
     {
         secondsTimeLocked = _secondsTimeLocked;
-        TimeLockChange(_secondsTimeLocked);
+        emit TimeLockChange(_secondsTimeLocked);
     }
 
     /// @dev Allows an owner to confirm a transaction.
     /// @param transactionId Transaction ID.
-    function confirmTransaction(uint transactionId)
+    function confirmTransaction(uint256 transactionId)
         public
         ownerExists(msg.sender)
         transactionExists(transactionId)
@@ -83,52 +93,35 @@ contract MultiSigWalletWithTimeLock is MultiSigWallet {
         notFullyConfirmed(transactionId)
     {
         confirmations[transactionId][msg.sender] = true;
-        Confirmation(msg.sender, transactionId);
+        emit Confirmation(msg.sender, transactionId);
         if (isConfirmed(transactionId)) {
             setConfirmationTime(transactionId, block.timestamp);
         }
     }
 
-    /// @dev Allows an owner to revoke a confirmation for a transaction.
-    /// @param transactionId Transaction ID.
-    function revokeConfirmation(uint transactionId)
-        public
-        ownerExists(msg.sender)
-        confirmed(transactionId, msg.sender)
-        notExecuted(transactionId)
-        notFullyConfirmed(transactionId)
-    {
-        confirmations[transactionId][msg.sender] = false;
-        Revocation(msg.sender, transactionId);
-    }
-
     /// @dev Allows anyone to execute a confirmed transaction.
     /// @param transactionId Transaction ID.
-    function executeTransaction(uint transactionId)
+    function executeTransaction(uint256 transactionId)
         public
         notExecuted(transactionId)
         fullyConfirmed(transactionId)
         pastTimeLock(transactionId)
     {
-        Transaction storage tx = transactions[transactionId];
-        tx.executed = true;
-        if (tx.destination.call.value(tx.value)(tx.data))
-            Execution(transactionId);
-        else {
-            ExecutionFailure(transactionId);
-            tx.executed = false;
+        Transaction storage txn = transactions[transactionId];
+        txn.executed = true;
+        if (external_call(txn.destination, txn.value, txn.data.length, txn.data)) {
+            emit Execution(transactionId);
+        } else {
+            emit ExecutionFailure(transactionId);
+            txn.executed = false;
         }
     }
 
-    /*
-     * Internal functions
-     */
-
     /// @dev Sets the time of when a submission first passed.
-    function setConfirmationTime(uint transactionId, uint confirmationTime)
+    function setConfirmationTime(uint256 transactionId, uint256 confirmationTime)
         internal
     {
         confirmationTimes[transactionId] = confirmationTime;
-        ConfirmationTimeSet(transactionId, confirmationTime);
+        emit ConfirmationTimeSet(transactionId, confirmationTime);
     }
 }
diff --git a/packages/contracts/src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol b/packages/contracts/src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol
index 5f69198e9..edb788fab 100644
--- a/packages/contracts/src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol
+++ b/packages/contracts/src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol
@@ -38,13 +38,13 @@ contract AssetProxyOwner is
     /// @dev Function will revert if the transaction does not call `removeAuthorizedAddressAtIndex`
     ///      on an approved AssetProxy contract.
     modifier validRemoveAuthorizedAddressAtIndexTx(uint256 transactionId) {
-        Transaction storage tx = transactions[transactionId];
+        Transaction storage txn = transactions[transactionId];
         require(
-            isAssetProxyRegistered[tx.destination],
+            isAssetProxyRegistered[txn.destination],
             "UNREGISTERED_ASSET_PROXY"
         );
         require(
-            tx.data.readBytes4(0) == REMOVE_AUTHORIZED_ADDRESS_AT_INDEX_SELECTOR,
+            txn.data.readBytes4(0) == REMOVE_AUTHORIZED_ADDRESS_AT_INDEX_SELECTOR,
             "INVALID_FUNCTION_SELECTOR"
         );
         _;
@@ -96,14 +96,13 @@ contract AssetProxyOwner is
         fullyConfirmed(transactionId)
         validRemoveAuthorizedAddressAtIndexTx(transactionId)
     {
-        Transaction storage tx = transactions[transactionId];
-        tx.executed = true;
-        // solhint-disable-next-line avoid-call-value
-        if (tx.destination.call.value(tx.value)(tx.data))
+        Transaction storage txn = transactions[transactionId];
+        txn.executed = true;
+        if (external_call(txn.destination, txn.value, txn.data.length, txn.data)) {
             emit Execution(transactionId);
-        else {
+        } else {
             emit ExecutionFailure(transactionId);
-            tx.executed = false;
+            txn.executed = false;
         }
     }
 }
diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol
index fc3210d90..cddff0e5f 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol
@@ -81,7 +81,7 @@ contract MixinWrapperFunctions is
         // Delegate to `fillOrder` and handle any exceptions gracefully
         assembly {
             let success := delegatecall(
-                gas,                                // forward all gas, TODO: look into gas consumption of assert/throw
+                gas,                                // forward all gas
                 address,                            // call address of this contract
                 add(fillOrderCalldata, 32),         // pointer to start of input (skip array length in first 32 bytes)
                 mload(fillOrderCalldata),           // length of input
diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibMath.sol b/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibMath.sol
index d8fed176c..c0b85ea10 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibMath.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibMath.sol
@@ -246,7 +246,6 @@ contract LibMath is
             numerator,
             denominator
         );
-        // TODO: safeMod
         remainder = safeSub(denominator, remainder) % denominator;
         isError = safeMul(1000, remainder) >= safeMul(numerator, target);
         return isError;