From 92497d7df4b61ee62acfdd58bfb98569ff67214e Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Thu, 23 Aug 2018 11:01:52 -0700
Subject: Fix isRoundingError

---
 .../src/2.0.0/protocol/Exchange/libs/LibMath.sol   | 27 ++++++++++++++--------
 1 file changed, 17 insertions(+), 10 deletions(-)

(limited to 'packages/contracts')

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 fa09da6ac..146bb9943 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
@@ -46,7 +46,7 @@ contract LibMath is
         return partialAmount;
     }
 
-    /// @dev Checks if rounding error > 0.1%.
+    /// @dev Checks if rounding error >= 0.1%.
     /// @param numerator Numerator.
     /// @param denominator Denominator.
     /// @param target Value to multiply with numerator/denominator.
@@ -60,16 +60,23 @@ contract LibMath is
         pure
         returns (bool isError)
     {
+        // The absolute rounding error is the difference between the rounded
+        // value and the ideal value. The relative rounding error is the
+        // absolute rounding error divided by the absolute value of the
+        // ideal value. We want the relative rounding error to be strictly less
+        // than 0.1%.
+        // Let's call `numerator * target % denominator` the remainder.
+        // The ideal value is `numerator * target / denominator`.
+        // The absolute error is `remainder / denominator`.
+        // The relative error is `remainder / numerator * target`.
+        // We want the relative error less than 1 / 1000:
+        //        remainder / numerator * denominator  <  1 / 1000
+        // or equivalently:
+        //        1000 * remainder  <  numerator * target
+        // so we have a rounding error iff:
+        //        1000 * remainder  >=  numerator * target
         uint256 remainder = mulmod(target, numerator, denominator);
-        if (remainder == 0) {
-            return false; // No rounding error.
-        }
-
-        uint256 errPercentageTimes1000000 = safeDiv(
-            safeMul(remainder, 1000000),
-            safeMul(numerator, target)
-        );
-        isError = errPercentageTimes1000000 > 1000;
+        isError = safeMul(1000, remainder) >= safeMul(numerator, target);
         return isError;
     }
 }
-- 
cgit v1.2.3


From 4159e45aff14c04f2d799ec8a7b6e7f1a4894064 Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Thu, 23 Aug 2018 12:54:13 -0700
Subject: Update tests

---
 packages/contracts/test/exchange/internal.ts | 26 +++++++++++++++-----------
 packages/contracts/test/exchange/libs.ts     |  4 ++--
 2 files changed, 17 insertions(+), 13 deletions(-)

(limited to 'packages/contracts')

diff --git a/packages/contracts/test/exchange/internal.ts b/packages/contracts/test/exchange/internal.ts
index 67d1d2d2c..0231cc3f1 100644
--- a/packages/contracts/test/exchange/internal.ts
+++ b/packages/contracts/test/exchange/internal.ts
@@ -63,6 +63,7 @@ describe('Exchange core internal functions', () => {
     let testExchange: TestExchangeInternalsContract;
     let invalidOpcodeErrorForCall: Error | undefined;
     let overflowErrorForSendTransaction: Error | undefined;
+    let divisionByZeroErrorForCall: Error | undefined;
 
     before(async () => {
         await blockchainLifecycle.startAsync();
@@ -79,6 +80,9 @@ describe('Exchange core internal functions', () => {
         overflowErrorForSendTransaction = new Error(
             await getRevertReasonOrErrorMessageForSendTransactionAsync(RevertReason.Uint256Overflow),
         );
+        divisionByZeroErrorForCall = new Error(
+            await getRevertReasonOrErrorMessageForSendTransactionAsync(RevertReason.DivisionByZero),
+        );
         invalidOpcodeErrorForCall = new Error(await getInvalidOpcodeErrorMessageForCallAsync());
     });
     // Note(albrow): Don't forget to add beforeEach and afterEach calls to reset
@@ -215,26 +219,26 @@ describe('Exchange core internal functions', () => {
             denominator: BigNumber,
             target: BigNumber,
         ): Promise<boolean> {
-            const product = numerator.mul(target);
             if (denominator.eq(0)) {
-                throw invalidOpcodeErrorForCall;
+                throw divisionByZeroErrorForCall;
             }
-            const remainder = product.mod(denominator);
-            if (remainder.eq(0)) {
+            if (numerator.eq(0)) {
+                return false;
+            }
+            if (target.eq(0)) {
                 return false;
             }
+            const product = numerator.mul(target);
+            const remainder = product.mod(denominator);
+            const remainderTimes1000 = remainder.mul('1000');
+            const isError = remainderTimes1000.gt(product);
             if (product.greaterThan(MAX_UINT256)) {
                 throw overflowErrorForCall;
             }
-            if (product.eq(0)) {
-                throw invalidOpcodeErrorForCall;
-            }
-            const remainderTimes1000000 = remainder.mul('1000000');
-            if (remainderTimes1000000.greaterThan(MAX_UINT256)) {
+            if (remainderTimes1000.greaterThan(MAX_UINT256)) {
                 throw overflowErrorForCall;
             }
-            const errPercentageTimes1000000 = remainderTimes1000000.dividedToIntegerBy(product);
-            return errPercentageTimes1000000.greaterThan('1000');
+            return isError;
         }
         async function testIsRoundingErrorAsync(
             numerator: BigNumber,
diff --git a/packages/contracts/test/exchange/libs.ts b/packages/contracts/test/exchange/libs.ts
index 6c3305d1d..c08d62758 100644
--- a/packages/contracts/test/exchange/libs.ts
+++ b/packages/contracts/test/exchange/libs.ts
@@ -71,13 +71,13 @@ describe('Exchange libs', () => {
     // combinatorial tests in test/exchange/internal. They test specific edge
     // cases that are not covered by the combinatorial tests.
     describe('LibMath', () => {
-        it('should return false if there is a rounding error of 0.1%', async () => {
+        it('should return true if there is a rounding error of 0.1%', async () => {
             const numerator = new BigNumber(20);
             const denominator = new BigNumber(999);
             const target = new BigNumber(50);
             // rounding error = ((20*50/999) - floor(20*50/999)) / (20*50/999) = 0.1%
             const isRoundingError = await libs.publicIsRoundingError.callAsync(numerator, denominator, target);
-            expect(isRoundingError).to.be.false();
+            expect(isRoundingError).to.be.true();
         });
         it('should return false if there is a rounding of 0.09%', async () => {
             const numerator = new BigNumber(20);
-- 
cgit v1.2.3


From e68942ee78eb19c27a96fb0b6b8b05c83b647bcc Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Thu, 23 Aug 2018 12:54:39 -0700
Subject: Handle zero case

---
 .../src/2.0.0/protocol/Exchange/libs/LibMath.sol       | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

(limited to 'packages/contracts')

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 146bb9943..758b7ec90 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
@@ -60,14 +60,26 @@ contract LibMath is
         pure
         returns (bool isError)
     {
+        require(denominator > 0, "DIVISION_BY_ZERO");
+        
         // The absolute rounding error is the difference between the rounded
         // value and the ideal value. The relative rounding error is the
         // absolute rounding error divided by the absolute value of the
-        // ideal value. We want the relative rounding error to be strictly less
-        // than 0.1%.
-        // Let's call `numerator * target % denominator` the remainder.
+        // ideal value. This is undefined when the ideal value is zero.
+        //
         // The ideal value is `numerator * target / denominator`.
+        // Let's call `numerator * target % denominator` the remainder.
         // The absolute error is `remainder / denominator`.
+        //
+        // When the ideal value is zero, we require the absolute error to
+        // be zero. Fortunately, this is always the case. The ideal value is
+        // zero iff `numerator == 0` and/or `target == 0`. In this case the
+        // remainder and absolute error are also zero. 
+        if (target == 0 || numerator == 0) {
+            return false;
+        }
+        // Otherwise, we want the relative rounding error to be strictly
+        // less than 0.1%.
         // The relative error is `remainder / numerator * target`.
         // We want the relative error less than 1 / 1000:
         //        remainder / numerator * denominator  <  1 / 1000
-- 
cgit v1.2.3


From ab5df342e1dc4add20223fab7128f9323a114b8e Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Wed, 22 Aug 2018 12:22:49 -0700
Subject: Add getPartialAmountCeil

---
 .../src/2.0.0/protocol/Exchange/libs/LibMath.sol   | 35 ++++++++++++++++++++--
 1 file changed, 32 insertions(+), 3 deletions(-)

(limited to 'packages/contracts')

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 758b7ec90..3e70d1b60 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
@@ -29,7 +29,7 @@ contract LibMath is
     /// @param numerator Numerator.
     /// @param denominator Denominator.
     /// @param target Value to calculate partial of.
-    /// @return Partial value of target.
+    /// @return Partial value of target rounded down.
     function getPartialAmount(
         uint256 numerator,
         uint256 denominator,
@@ -45,8 +45,37 @@ contract LibMath is
         );
         return partialAmount;
     }
-
-    /// @dev Checks if rounding error >= 0.1%.
+    
+    /// @dev Calculates partial value given a numerator and denominator.
+    /// @param numerator Numerator.
+    /// @param denominator Denominator.
+    /// @param target Value to calculate partial of.
+    /// @return Partial value of target rounded up.
+    function getPartialAmountCeil(
+        uint256 numerator,
+        uint256 denominator,
+        uint256 target
+    )
+        internal
+        pure
+        returns (uint256 partialAmount)
+    {
+        require(
+            denominator > 0,
+            "DIVISION_BY_ZERO"
+        );
+        
+        // SafeDiv rounds down. To use it to round up we need to add
+        // denominator - 1. This causes anything less than an exact multiple
+        // of denominator to be rounded up.
+        partialAmount = safeDiv(
+            safeAdd(safeMul(numerator, target), safeSub(denominator, 1)),
+            denominator
+        );
+        return partialAmount;
+    }
+    
+    /// @dev Checks if rounding error > 0.1%.
     /// @param numerator Numerator.
     /// @param denominator Denominator.
     /// @param target Value to multiply with numerator/denominator.
-- 
cgit v1.2.3


From 5d70df771b151800b8a04b5569529843701c9fbd Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Wed, 22 Aug 2018 12:22:58 -0700
Subject: Add isRoundingErrorCeil

---
 .../src/2.0.0/protocol/Exchange/libs/LibMath.sol   | 29 ++++++++++++++++++++++
 1 file changed, 29 insertions(+)

(limited to 'packages/contracts')

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 3e70d1b60..c4aa2abb5 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
@@ -107,6 +107,7 @@ contract LibMath is
         if (target == 0 || numerator == 0) {
             return false;
         }
+        
         // Otherwise, we want the relative rounding error to be strictly
         // less than 0.1%.
         // The relative error is `remainder / numerator * target`.
@@ -120,4 +121,32 @@ contract LibMath is
         isError = safeMul(1000, remainder) >= safeMul(numerator, target);
         return isError;
     }
+    
+    /// @dev Checks if rounding error > 0.1%.
+    /// @param numerator Numerator.
+    /// @param denominator Denominator.
+    /// @param target Value to multiply with numerator/denominator.
+    /// @return Rounding error is present.
+    function isRoundingErrorCeil(
+        uint256 numerator,
+        uint256 denominator,
+        uint256 target
+    )
+        internal
+        pure
+        returns (bool isError)
+    {
+        require(denominator > 0, "DIVISION_BY_ZERO");
+        
+        if (target == 0 || numerator == 0) {
+            return false;
+        }
+        uint256 remainder = mulmod(target, numerator, denominator);
+        if (remainder == 0) {
+            return false;
+        }
+        remainder = safeSub(denominator, remainder);
+        isError = safeMul(1000, remainder) >= safeMul(numerator, target);
+        return isError;
+    }
 }
-- 
cgit v1.2.3


From 3dad6ee55e9f51daa66cfe3c83dd17abc31f23f5 Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Wed, 22 Aug 2018 12:23:32 -0700
Subject: Add tests for getPartialAmountCeil

---
 .../TestExchangeInternals.sol                      | 17 +++++++++
 packages/contracts/test/exchange/internal.ts       | 40 ++++++++++++++++++++++
 2 files changed, 57 insertions(+)

(limited to 'packages/contracts')

diff --git a/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol b/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol
index d9cec9edc..239dd10a8 100644
--- a/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol
+++ b/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol
@@ -79,6 +79,23 @@ contract TestExchangeInternals is
         return getPartialAmount(numerator, denominator, target);
     }
 
+    /// @dev Calculates partial value given a numerator and denominator.
+    /// @param numerator Numerator.
+    /// @param denominator Denominator.
+    /// @param target Value to calculate partial of.
+    /// @return Partial value of target.
+    function publicGetPartialAmountCeil(
+        uint256 numerator,
+        uint256 denominator,
+        uint256 target
+    )
+        public
+        pure
+        returns (uint256 partialAmount)
+    {
+        return getPartialAmountCeil(numerator, denominator, target);
+    }
+
     /// @dev Checks if rounding error > 0.1%.
     /// @param numerator Numerator.
     /// @param denominator Denominator.
diff --git a/packages/contracts/test/exchange/internal.ts b/packages/contracts/test/exchange/internal.ts
index 0231cc3f1..69625ee1d 100644
--- a/packages/contracts/test/exchange/internal.ts
+++ b/packages/contracts/test/exchange/internal.ts
@@ -1,6 +1,7 @@
 import { BlockchainLifecycle } from '@0xproject/dev-utils';
 import { Order, RevertReason, SignedOrder } from '@0xproject/types';
 import { BigNumber } from '@0xproject/utils';
+import * as chai from 'chai';
 import * as _ from 'lodash';
 
 import { TestExchangeInternalsContract } from '../../generated_contract_wrappers/test_exchange_internals';
@@ -16,6 +17,8 @@ import { FillResults } from '../utils/types';
 import { provider, txDefaults, web3Wrapper } from '../utils/web3_wrapper';
 
 chaiSetup.configure();
+const expect = chai.expect;
+
 const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper);
 
 const MAX_UINT256 = new BigNumber(2).pow(256).minus(1);
@@ -213,6 +216,43 @@ describe('Exchange core internal functions', () => {
         );
     });
 
+    describe.only('getPartialAmountCeil', async () => {
+        async function referenceGetPartialAmountCeilAsync(
+            numerator: BigNumber,
+            denominator: BigNumber,
+            target: BigNumber,
+        ) {
+            if (denominator.eq(0)) {
+                throw new Error('revert DIVISION_BY_ZERO');
+            }
+            const product = numerator.mul(target);
+            const offset = product.add(denominator.sub(1));
+            if (offset.greaterThan(MAX_UINT256)) {
+                throw overflowErrorForCall;
+            }
+            const result = offset.dividedToIntegerBy(denominator);
+            if (product.mod(denominator).eq(0)) {
+                expect(result.mul(denominator)).to.be.bignumber.eq(product);
+            } else {
+                expect(result.mul(denominator)).to.be.bignumber.gt(product);
+            }
+            return result;
+        }
+        async function testGetPartialAmountCeilAsync(
+            numerator: BigNumber,
+            denominator: BigNumber,
+            target: BigNumber,
+        ): Promise<BigNumber> {
+            return testExchange.publicGetPartialAmountCeil.callAsync(numerator, denominator, target);
+        }
+        await testCombinatoriallyWithReferenceFuncAsync(
+            'getPartialAmountCeil',
+            referenceGetPartialAmountCeilAsync,
+            testGetPartialAmountCeilAsync,
+            [uint256Values, uint256Values, uint256Values],
+        );
+    });
+
     describe('isRoundingError', async () => {
         async function referenceIsRoundingErrorAsync(
             numerator: BigNumber,
-- 
cgit v1.2.3


From 50fab9feb3b80b7d5ac1e94df9af68cad4aaabe0 Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Wed, 22 Aug 2018 12:22:58 -0700
Subject: Improve getPartialAmountCeil docs

---
 packages/contracts/src/2.0.0/protocol/Exchange/libs/LibMath.sol | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

(limited to 'packages/contracts')

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 c4aa2abb5..d123c55a1 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
@@ -65,9 +65,9 @@ contract LibMath is
             "DIVISION_BY_ZERO"
         );
         
-        // SafeDiv rounds down. To use it to round up we need to add
-        // denominator - 1. This causes anything less than an exact multiple
-        // of denominator to be rounded up.
+        // safeDiv computes `floor(a / b)`. We use the identity (a, b integer):
+        //       ceil(a / b) = floor((a + b - 1) / b)
+        // To implement `ceil(a / b)` using safeDiv.
         partialAmount = safeDiv(
             safeAdd(safeMul(numerator, target), safeSub(denominator, 1)),
             denominator
-- 
cgit v1.2.3


From c109d1f5451c43afd92dd0fb4bebb48cba65c661 Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Thu, 23 Aug 2018 13:33:05 -0700
Subject: Remove .only

---
 packages/contracts/test/exchange/internal.ts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(limited to 'packages/contracts')

diff --git a/packages/contracts/test/exchange/internal.ts b/packages/contracts/test/exchange/internal.ts
index 69625ee1d..2f25b1708 100644
--- a/packages/contracts/test/exchange/internal.ts
+++ b/packages/contracts/test/exchange/internal.ts
@@ -216,7 +216,7 @@ describe('Exchange core internal functions', () => {
         );
     });
 
-    describe.only('getPartialAmountCeil', async () => {
+    describe('getPartialAmountCeil', async () => {
         async function referenceGetPartialAmountCeilAsync(
             numerator: BigNumber,
             denominator: BigNumber,
-- 
cgit v1.2.3


From 4219af1430f1cfc105d3521616941b7947fde4e3 Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Thu, 23 Aug 2018 14:22:59 -0700
Subject: Add DIVISION_BY_ZERO to getPartialAmount for consistency

---
 .../src/2.0.0/protocol/Exchange/libs/LibMath.sol   |  7 +++--
 packages/contracts/test/exchange/internal.ts       | 31 +++++++++++-----------
 2 files changed, 18 insertions(+), 20 deletions(-)

(limited to 'packages/contracts')

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 d123c55a1..f4e2f1958 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
@@ -39,6 +39,8 @@ contract LibMath is
         pure
         returns (uint256 partialAmount)
     {
+        require(denominator > 0, "DIVISION_BY_ZERO");
+        
         partialAmount = safeDiv(
             safeMul(numerator, target),
             denominator
@@ -60,10 +62,7 @@ contract LibMath is
         pure
         returns (uint256 partialAmount)
     {
-        require(
-            denominator > 0,
-            "DIVISION_BY_ZERO"
-        );
+        require(denominator > 0, "DIVISION_BY_ZERO");
         
         // safeDiv computes `floor(a / b)`. We use the identity (a, b integer):
         //       ceil(a / b) = floor((a + b - 1) / b)
diff --git a/packages/contracts/test/exchange/internal.ts b/packages/contracts/test/exchange/internal.ts
index 2f25b1708..48e1e620c 100644
--- a/packages/contracts/test/exchange/internal.ts
+++ b/packages/contracts/test/exchange/internal.ts
@@ -46,22 +46,6 @@ const emptySignedOrder: SignedOrder = {
 
 const overflowErrorForCall = new Error(RevertReason.Uint256Overflow);
 
-async function referenceGetPartialAmountAsync(
-    numerator: BigNumber,
-    denominator: BigNumber,
-    target: BigNumber,
-): Promise<BigNumber> {
-    const invalidOpcodeErrorForCall = new Error(await getInvalidOpcodeErrorMessageForCallAsync());
-    const product = numerator.mul(target);
-    if (product.greaterThan(MAX_UINT256)) {
-        throw overflowErrorForCall;
-    }
-    if (denominator.eq(0)) {
-        throw invalidOpcodeErrorForCall;
-    }
-    return product.dividedToIntegerBy(denominator);
-}
-
 describe('Exchange core internal functions', () => {
     let testExchange: TestExchangeInternalsContract;
     let invalidOpcodeErrorForCall: Error | undefined;
@@ -91,6 +75,21 @@ describe('Exchange core internal functions', () => {
     // Note(albrow): Don't forget to add beforeEach and afterEach calls to reset
     // the blockchain state for any tests which modify it!
 
+    async function referenceGetPartialAmountAsync(
+        numerator: BigNumber,
+        denominator: BigNumber,
+        target: BigNumber,
+    ): Promise<BigNumber> {
+        if (denominator.eq(0)) {
+            throw divisionByZeroErrorForCall;
+        }
+        const product = numerator.mul(target);
+        if (product.greaterThan(MAX_UINT256)) {
+            throw overflowErrorForCall;
+        }
+        return product.dividedToIntegerBy(denominator);
+    }
+
     describe('addFillResults', async () => {
         function makeFillResults(value: BigNumber): FillResults {
             return {
-- 
cgit v1.2.3


From 0fb7617a785b765415cb7f43448c9b8ea905963f Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Thu, 23 Aug 2018 15:18:45 -0700
Subject: Fix incorect modulus

---
 packages/contracts/src/2.0.0/protocol/Exchange/libs/LibMath.sol | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

(limited to 'packages/contracts')

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 f4e2f1958..1c14dbcae 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
@@ -141,10 +141,8 @@ contract LibMath is
             return false;
         }
         uint256 remainder = mulmod(target, numerator, denominator);
-        if (remainder == 0) {
-            return false;
-        }
-        remainder = safeSub(denominator, remainder);
+        // TODO: safeMod
+        remainder = safeSub(denominator, remainder) % denominator;
         isError = safeMul(1000, remainder) >= safeMul(numerator, target);
         return isError;
     }
-- 
cgit v1.2.3


From 6734f2f1bcdab8f0d50524a26195707da00bd8ed Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Thu, 23 Aug 2018 15:18:55 -0700
Subject: Add docs

---
 packages/contracts/src/2.0.0/protocol/Exchange/libs/LibMath.sol | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

(limited to 'packages/contracts')

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 1c14dbcae..8a1444af1 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
@@ -74,7 +74,7 @@ contract LibMath is
         return partialAmount;
     }
     
-    /// @dev Checks if rounding error > 0.1%.
+    /// @dev Checks if rounding error >= 0.1% when rounding down.
     /// @param numerator Numerator.
     /// @param denominator Denominator.
     /// @param target Value to multiply with numerator/denominator.
@@ -121,7 +121,7 @@ contract LibMath is
         return isError;
     }
     
-    /// @dev Checks if rounding error > 0.1%.
+    /// @dev Checks if rounding error >= 0.1% when rounding up.
     /// @param numerator Numerator.
     /// @param denominator Denominator.
     /// @param target Value to multiply with numerator/denominator.
@@ -137,9 +137,14 @@ contract LibMath is
     {
         require(denominator > 0, "DIVISION_BY_ZERO");
         
+        // See the comments in `isRoundingError`.
         if (target == 0 || numerator == 0) {
+            // When either is zero, the ideal value and rounded value are zero
+            // and there is no rounding error. (Although the relative error
+            // is undefined.)
             return false;
         }
+        // Compute remainder as before
         uint256 remainder = mulmod(target, numerator, denominator);
         // TODO: safeMod
         remainder = safeSub(denominator, remainder) % denominator;
-- 
cgit v1.2.3


From 7f78d7da9dd13d1f0068a292bcd1ee3c5439d5a8 Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Thu, 23 Aug 2018 15:19:29 -0700
Subject: Add tests

---
 .../TestExchangeInternals.sol                      | 19 +++++-
 .../contracts/src/2.0.0/test/TestLibs/TestLibs.sol | 34 ++++++++++
 packages/contracts/test/exchange/internal.ts       | 43 +++++++++++++
 packages/contracts/test/exchange/libs.ts           | 72 +++++++++++++++-------
 4 files changed, 145 insertions(+), 23 deletions(-)

(limited to 'packages/contracts')

diff --git a/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol b/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol
index 239dd10a8..5e2ae2f0e 100644
--- a/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol
+++ b/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol
@@ -96,7 +96,7 @@ contract TestExchangeInternals is
         return getPartialAmountCeil(numerator, denominator, target);
     }
 
-    /// @dev Checks if rounding error > 0.1%.
+    /// @dev Checks if rounding error >= 0.1%.
     /// @param numerator Numerator.
     /// @param denominator Denominator.
     /// @param target Value to multiply with numerator/denominator.
@@ -112,6 +112,23 @@ contract TestExchangeInternals is
     {
         return isRoundingError(numerator, denominator, target);
     }
+
+    /// @dev Checks if rounding error >= 0.1%.
+    /// @param numerator Numerator.
+    /// @param denominator Denominator.
+    /// @param target Value to multiply with numerator/denominator.
+    /// @return Rounding error is present.
+    function publicIsRoundingErrorCeil(
+        uint256 numerator,
+        uint256 denominator,
+        uint256 target
+    )
+        public
+        pure
+        returns (bool isError)
+    {
+        return isRoundingErrorCeil(numerator, denominator, target);
+    }
  
     /// @dev Updates state with results of a fill order.
     /// @param order that was filled.
diff --git a/packages/contracts/src/2.0.0/test/TestLibs/TestLibs.sol b/packages/contracts/src/2.0.0/test/TestLibs/TestLibs.sol
index 4a99dd9c1..8c3d12226 100644
--- a/packages/contracts/src/2.0.0/test/TestLibs/TestLibs.sol
+++ b/packages/contracts/src/2.0.0/test/TestLibs/TestLibs.sol
@@ -66,6 +66,23 @@ contract TestLibs is
         return partialAmount;
     }
 
+    function publicGetPartialAmountCeil(
+        uint256 numerator,
+        uint256 denominator,
+        uint256 target
+    )
+        public
+        pure
+        returns (uint256 partialAmount)
+    {
+        partialAmount = getPartialAmountCeil(
+            numerator,
+            denominator,
+            target
+        );
+        return partialAmount;
+    }
+
     function publicIsRoundingError(
         uint256 numerator,
         uint256 denominator,
@@ -83,6 +100,23 @@ contract TestLibs is
         return isError;
     }
 
+    function publicIsRoundingErrorCeil(
+        uint256 numerator,
+        uint256 denominator,
+        uint256 target
+    )
+        public
+        pure
+        returns (bool isError)
+    {
+        isError = isRoundingErrorCeil(
+            numerator,
+            denominator,
+            target
+        );
+        return isError;
+    }
+
     function publicGetOrderHash(Order memory order)
         public
         view
diff --git a/packages/contracts/test/exchange/internal.ts b/packages/contracts/test/exchange/internal.ts
index 48e1e620c..0c6ab3707 100644
--- a/packages/contracts/test/exchange/internal.ts
+++ b/packages/contracts/test/exchange/internal.ts
@@ -294,6 +294,49 @@ describe('Exchange core internal functions', () => {
         );
     });
 
+    describe('isRoundingErrorCeil', async () => {
+        async function referenceIsRoundingErrorAsync(
+            numerator: BigNumber,
+            denominator: BigNumber,
+            target: BigNumber,
+        ): Promise<boolean> {
+            if (denominator.eq(0)) {
+                throw divisionByZeroErrorForCall;
+            }
+            if (numerator.eq(0)) {
+                return false;
+            }
+            if (target.eq(0)) {
+                return false;
+            }
+            const product = numerator.mul(target);
+            const remainder = product.mod(denominator);
+            const error = denominator.sub(remainder).mod(denominator);
+            const errorTimes1000 = error.mul('1000');
+            const isError = errorTimes1000.gt(product);
+            if (product.greaterThan(MAX_UINT256)) {
+                throw overflowErrorForCall;
+            }
+            if (errorTimes1000.greaterThan(MAX_UINT256)) {
+                throw overflowErrorForCall;
+            }
+            return isError;
+        }
+        async function testIsRoundingErrorCeilAsync(
+            numerator: BigNumber,
+            denominator: BigNumber,
+            target: BigNumber,
+        ): Promise<boolean> {
+            return testExchange.publicIsRoundingErrorCeil.callAsync(numerator, denominator, target);
+        }
+        await testCombinatoriallyWithReferenceFuncAsync(
+            'isRoundingErrorCeil',
+            referenceIsRoundingErrorAsync,
+            testIsRoundingErrorCeilAsync,
+            [uint256Values, uint256Values, uint256Values],
+        );
+    });
+
     describe('updateFilledState', async () => {
         // Note(albrow): Since updateFilledState modifies the state by calling
         // sendTransaction, we must reset the state after each test.
diff --git a/packages/contracts/test/exchange/libs.ts b/packages/contracts/test/exchange/libs.ts
index c08d62758..a5f31a498 100644
--- a/packages/contracts/test/exchange/libs.ts
+++ b/packages/contracts/test/exchange/libs.ts
@@ -71,29 +71,57 @@ describe('Exchange libs', () => {
     // combinatorial tests in test/exchange/internal. They test specific edge
     // cases that are not covered by the combinatorial tests.
     describe('LibMath', () => {
-        it('should return true if there is a rounding error of 0.1%', async () => {
-            const numerator = new BigNumber(20);
-            const denominator = new BigNumber(999);
-            const target = new BigNumber(50);
-            // rounding error = ((20*50/999) - floor(20*50/999)) / (20*50/999) = 0.1%
-            const isRoundingError = await libs.publicIsRoundingError.callAsync(numerator, denominator, target);
-            expect(isRoundingError).to.be.true();
-        });
-        it('should return false if there is a rounding of 0.09%', async () => {
-            const numerator = new BigNumber(20);
-            const denominator = new BigNumber(9991);
-            const target = new BigNumber(500);
-            // rounding error = ((20*500/9991) - floor(20*500/9991)) / (20*500/9991) = 0.09%
-            const isRoundingError = await libs.publicIsRoundingError.callAsync(numerator, denominator, target);
-            expect(isRoundingError).to.be.false();
+        describe('isRoundingError', () => {
+            it('should return true if there is a rounding error of 0.1%', async () => {
+                const numerator = new BigNumber(20);
+                const denominator = new BigNumber(999);
+                const target = new BigNumber(50);
+                // rounding error = ((20*50/999) - floor(20*50/999)) / (20*50/999) = 0.1%
+                const isRoundingError = await libs.publicIsRoundingError.callAsync(numerator, denominator, target);
+                expect(isRoundingError).to.be.true();
+            });
+            it('should return false if there is a rounding of 0.09%', async () => {
+                const numerator = new BigNumber(20);
+                const denominator = new BigNumber(9991);
+                const target = new BigNumber(500);
+                // rounding error = ((20*500/9991) - floor(20*500/9991)) / (20*500/9991) = 0.09%
+                const isRoundingError = await libs.publicIsRoundingError.callAsync(numerator, denominator, target);
+                expect(isRoundingError).to.be.false();
+            });
+            it('should return true if there is a rounding error of 0.11%', async () => {
+                const numerator = new BigNumber(20);
+                const denominator = new BigNumber(9989);
+                const target = new BigNumber(500);
+                // rounding error = ((20*500/9989) - floor(20*500/9989)) / (20*500/9989) = 0.011%
+                const isRoundingError = await libs.publicIsRoundingError.callAsync(numerator, denominator, target);
+                expect(isRoundingError).to.be.true();
+            });
         });
-        it('should return true if there is a rounding error of 0.11%', async () => {
-            const numerator = new BigNumber(20);
-            const denominator = new BigNumber(9989);
-            const target = new BigNumber(500);
-            // rounding error = ((20*500/9989) - floor(20*500/9989)) / (20*500/9989) = 0.011%
-            const isRoundingError = await libs.publicIsRoundingError.callAsync(numerator, denominator, target);
-            expect(isRoundingError).to.be.true();
+        describe('isRoundingErrorCeil', () => {
+            it('should return true if there is a rounding error of 0.1%', async () => {
+                const numerator = new BigNumber(20);
+                const denominator = new BigNumber(1001);
+                const target = new BigNumber(50);
+                // rounding error = (ceil(20*50/1001) - (20*50/1001)) / (20*50/1001) = 0.1%
+                const isRoundingError = await libs.publicIsRoundingErrorCeil.callAsync(numerator, denominator, target);
+                expect(isRoundingError).to.be.true();
+            });
+            it('should return false if there is a rounding of 0.09%', async () => {
+                const numerator = new BigNumber(20);
+                const denominator = new BigNumber(10009);
+                const target = new BigNumber(500);
+                // rounding error = (ceil(20*500/10009) - (20*500/10009)) / (20*500/10009) = 0.09%
+                const isRoundingError = await libs.publicIsRoundingErrorCeil.callAsync(numerator, denominator, target);
+                expect(isRoundingError).to.be.false();
+            });
+            it('should return true if there is a rounding error of 0.11%', async () => {
+                const numerator = new BigNumber(20);
+                const denominator = new BigNumber(10011);
+                const target = new BigNumber(500);
+                // rounding error = (ceil(20*500/10011) - (20*500/10011)) / (20*500/10011) = 0.11%
+                const isRoundingError = await libs.publicIsRoundingErrorCeil.callAsync(numerator, denominator, target);
+                expect(isRoundingError).to.be.true();
+            });
         });
     });
 
-- 
cgit v1.2.3


From f6080367fe3a90762afea76a653c5df1315c45fa Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Fri, 24 Aug 2018 14:11:45 -0700
Subject: Disambiguate the operator precedence

---
 packages/contracts/src/2.0.0/protocol/Exchange/libs/LibMath.sol | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

(limited to 'packages/contracts')

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 8a1444af1..90ec9e2b6 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
@@ -109,9 +109,9 @@ contract LibMath is
         
         // Otherwise, we want the relative rounding error to be strictly
         // less than 0.1%.
-        // The relative error is `remainder / numerator * target`.
+        // The relative error is `remainder / (numerator * target)`.
         // We want the relative error less than 1 / 1000:
-        //        remainder / numerator * denominator  <  1 / 1000
+        //        remainder / (numerator * denominator)  <  1 / 1000
         // or equivalently:
         //        1000 * remainder  <  numerator * target
         // so we have a rounding error iff:
-- 
cgit v1.2.3


From 80291caf7cef16f0b300484755960d92d6750a6a Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Fri, 24 Aug 2018 16:16:44 -0700
Subject: Append -Floor to getPartialAmount and isRoundingError

---
 .../extensions/Forwarder/MixinExchangeWrapper.sol    |  4 ++--
 .../extensions/Forwarder/MixinForwarderCore.sol      |  4 ++--
 .../src/2.0.0/extensions/Forwarder/MixinWeth.sol     |  2 +-
 .../2.0.0/protocol/Exchange/MixinExchangeCore.sol    |  8 ++++----
 .../src/2.0.0/protocol/Exchange/MixinMatchOrders.sol |  4 ++--
 .../protocol/Exchange/MixinWrapperFunctions.sol      |  4 ++--
 .../src/2.0.0/protocol/Exchange/libs/LibMath.sol     |  8 ++++----
 .../TestExchangeInternals/TestExchangeInternals.sol  |  8 ++++----
 .../contracts/src/2.0.0/test/TestLibs/TestLibs.sol   |  8 ++++----
 packages/contracts/test/exchange/internal.ts         | 20 ++++++++++----------
 packages/contracts/test/exchange/libs.ts             |  6 +++---
 .../test/utils/fill_order_combinatorial_utils.ts     | 12 ++++++------
 packages/contracts/test/utils/order_utils.ts         |  2 +-
 13 files changed, 45 insertions(+), 45 deletions(-)

(limited to 'packages/contracts')

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 218713d3c..a7ff400b9 100644
--- a/packages/contracts/src/2.0.0/extensions/Forwarder/MixinExchangeWrapper.sol
+++ b/packages/contracts/src/2.0.0/extensions/Forwarder/MixinExchangeWrapper.sol
@@ -163,7 +163,7 @@ contract MixinExchangeWrapper is
 
             // Convert the remaining amount of makerAsset to buy into remaining amount
             // of takerAsset to sell, assuming entire amount can be sold in the current order
-            uint256 remainingTakerAssetFillAmount = getPartialAmount(
+            uint256 remainingTakerAssetFillAmount = getPartialAmountFloor(
                 orders[i].takerAssetAmount,
                 orders[i].makerAssetAmount,
                 remainingMakerAssetFillAmount
@@ -231,7 +231,7 @@ contract MixinExchangeWrapper is
 
             // Convert the remaining amount of ZRX to buy into remaining amount
             // of WETH to sell, assuming entire amount can be sold in the current order.
-            uint256 remainingWethSellAmount = getPartialAmount(
+            uint256 remainingWethSellAmount = getPartialAmountFloor(
                 orders[i].takerAssetAmount,
                 safeSub(orders[i].makerAssetAmount, orders[i].takerFee),  // our exchange rate after fees 
                 remainingZrxBuyAmount
diff --git a/packages/contracts/src/2.0.0/extensions/Forwarder/MixinForwarderCore.sol b/packages/contracts/src/2.0.0/extensions/Forwarder/MixinForwarderCore.sol
index 42cec4d36..14f191879 100644
--- a/packages/contracts/src/2.0.0/extensions/Forwarder/MixinForwarderCore.sol
+++ b/packages/contracts/src/2.0.0/extensions/Forwarder/MixinForwarderCore.sol
@@ -87,7 +87,7 @@ contract MixinForwarderCore is
         uint256 makerAssetAmountPurchased;
         if (orders[0].makerAssetData.equals(ZRX_ASSET_DATA)) {
             // Calculate amount of WETH that won't be spent on ETH fees.
-            wethSellAmount = getPartialAmount(
+            wethSellAmount = getPartialAmountFloor(
                 PERCENTAGE_DENOMINATOR,
                 safeAdd(PERCENTAGE_DENOMINATOR, feePercentage),
                 msg.value
@@ -103,7 +103,7 @@ contract MixinForwarderCore is
             makerAssetAmountPurchased = safeSub(orderFillResults.makerAssetFilledAmount, orderFillResults.takerFeePaid);
         } else {
             // 5% of WETH is reserved for filling feeOrders and paying feeRecipient.
-            wethSellAmount = getPartialAmount(
+            wethSellAmount = getPartialAmountFloor(
                 MAX_WETH_FILL_PERCENTAGE,
                 PERCENTAGE_DENOMINATOR,
                 msg.value
diff --git a/packages/contracts/src/2.0.0/extensions/Forwarder/MixinWeth.sol b/packages/contracts/src/2.0.0/extensions/Forwarder/MixinWeth.sol
index 93e85e599..5863b522d 100644
--- a/packages/contracts/src/2.0.0/extensions/Forwarder/MixinWeth.sol
+++ b/packages/contracts/src/2.0.0/extensions/Forwarder/MixinWeth.sol
@@ -82,7 +82,7 @@ contract MixinWeth is
         uint256 wethRemaining = safeSub(msg.value, wethSold);
 
         // Calculate ETH fee to pay to feeRecipient.
-        uint256 ethFee = getPartialAmount(
+        uint256 ethFee = getPartialAmountFloor(
             feePercentage,
             PERCENTAGE_DENOMINATOR,
             wethSoldExcludingFeeOrders
diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
index ab5c6e507..f3a0591b2 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
@@ -320,7 +320,7 @@ contract MixinExchangeCore is
 
         // Validate fill order rounding
         require(
-            !isRoundingError(
+            !isRoundingErrorFloor(
                 takerAssetFilledAmount,
                 order.takerAssetAmount,
                 order.makerAssetAmount
@@ -376,17 +376,17 @@ contract MixinExchangeCore is
     {
         // Compute proportional transfer amounts
         fillResults.takerAssetFilledAmount = takerAssetFilledAmount;
-        fillResults.makerAssetFilledAmount = getPartialAmount(
+        fillResults.makerAssetFilledAmount = getPartialAmountFloor(
             takerAssetFilledAmount,
             order.takerAssetAmount,
             order.makerAssetAmount
         );
-        fillResults.makerFeePaid = getPartialAmount(
+        fillResults.makerFeePaid = getPartialAmountFloor(
             takerAssetFilledAmount,
             order.takerAssetAmount,
             order.makerFee
         );
-        fillResults.takerFeePaid = getPartialAmount(
+        fillResults.takerFeePaid = getPartialAmountFloor(
             takerAssetFilledAmount,
             order.takerAssetAmount,
             order.takerFee
diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
index 56b309a1b..20f4b1c33 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
@@ -183,7 +183,7 @@ contract MixinMatchOrders is
             leftTakerAssetFilledAmount = leftTakerAssetAmountRemaining;
 
             // The right order receives an amount proportional to how much was spent.
-            rightTakerAssetFilledAmount = getPartialAmount(
+            rightTakerAssetFilledAmount = getPartialAmountFloor(
                 rightOrder.takerAssetAmount,
                 rightOrder.makerAssetAmount,
                 leftTakerAssetFilledAmount
@@ -193,7 +193,7 @@ contract MixinMatchOrders is
             rightTakerAssetFilledAmount = rightTakerAssetAmountRemaining;
 
             // The left order receives an amount proportional to how much was spent.
-            leftTakerAssetFilledAmount = getPartialAmount(
+            leftTakerAssetFilledAmount = getPartialAmountFloor(
                 rightOrder.makerAssetAmount,
                 rightOrder.takerAssetAmount,
                 rightTakerAssetFilledAmount
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 86194f461..1b77cfbcb 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol
@@ -298,7 +298,7 @@ contract MixinWrapperFunctions is
 
             // Convert the remaining amount of makerAsset to buy into remaining amount
             // of takerAsset to sell, assuming entire amount can be sold in the current order
-            uint256 remainingTakerAssetFillAmount = getPartialAmount(
+            uint256 remainingTakerAssetFillAmount = getPartialAmountFloor(
                 orders[i].takerAssetAmount,
                 orders[i].makerAssetAmount,
                 remainingMakerAssetFillAmount
@@ -350,7 +350,7 @@ contract MixinWrapperFunctions is
 
             // Convert the remaining amount of makerAsset to buy into remaining amount
             // of takerAsset to sell, assuming entire amount can be sold in the current order
-            uint256 remainingTakerAssetFillAmount = getPartialAmount(
+            uint256 remainingTakerAssetFillAmount = getPartialAmountFloor(
                 orders[i].takerAssetAmount,
                 orders[i].makerAssetAmount,
                 remainingMakerAssetFillAmount
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 90ec9e2b6..7d58f70ce 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
@@ -25,12 +25,12 @@ contract LibMath is
     SafeMath
 {
 
-    /// @dev Calculates partial value given a numerator and denominator.
+    /// @dev Calculates partial value given a numerator and denominator rounded down.
     /// @param numerator Numerator.
     /// @param denominator Denominator.
     /// @param target Value to calculate partial of.
     /// @return Partial value of target rounded down.
-    function getPartialAmount(
+    function getPartialAmountFloor(
         uint256 numerator,
         uint256 denominator,
         uint256 target
@@ -48,7 +48,7 @@ contract LibMath is
         return partialAmount;
     }
     
-    /// @dev Calculates partial value given a numerator and denominator.
+    /// @dev Calculates partial value given a numerator and denominator rounded down.
     /// @param numerator Numerator.
     /// @param denominator Denominator.
     /// @param target Value to calculate partial of.
@@ -79,7 +79,7 @@ contract LibMath is
     /// @param denominator Denominator.
     /// @param target Value to multiply with numerator/denominator.
     /// @return Rounding error is present.
-    function isRoundingError(
+    function isRoundingErrorFloor(
         uint256 numerator,
         uint256 denominator,
         uint256 target
diff --git a/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol b/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol
index 5e2ae2f0e..da9313e02 100644
--- a/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol
+++ b/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol
@@ -67,7 +67,7 @@ contract TestExchangeInternals is
     /// @param denominator Denominator.
     /// @param target Value to calculate partial of.
     /// @return Partial value of target.
-    function publicGetPartialAmount(
+    function publicGetPartialAmountFloor(
         uint256 numerator,
         uint256 denominator,
         uint256 target
@@ -76,7 +76,7 @@ contract TestExchangeInternals is
         pure
         returns (uint256 partialAmount)
     {
-        return getPartialAmount(numerator, denominator, target);
+        return getPartialAmountFloor(numerator, denominator, target);
     }
 
     /// @dev Calculates partial value given a numerator and denominator.
@@ -101,7 +101,7 @@ contract TestExchangeInternals is
     /// @param denominator Denominator.
     /// @param target Value to multiply with numerator/denominator.
     /// @return Rounding error is present.
-    function publicIsRoundingError(
+    function publicIsRoundingErrorFloor(
         uint256 numerator,
         uint256 denominator,
         uint256 target
@@ -110,7 +110,7 @@ contract TestExchangeInternals is
         pure
         returns (bool isError)
     {
-        return isRoundingError(numerator, denominator, target);
+        return isRoundingErrorFloor(numerator, denominator, target);
     }
 
     /// @dev Checks if rounding error >= 0.1%.
diff --git a/packages/contracts/src/2.0.0/test/TestLibs/TestLibs.sol b/packages/contracts/src/2.0.0/test/TestLibs/TestLibs.sol
index 8c3d12226..c8c58545f 100644
--- a/packages/contracts/src/2.0.0/test/TestLibs/TestLibs.sol
+++ b/packages/contracts/src/2.0.0/test/TestLibs/TestLibs.sol
@@ -49,7 +49,7 @@ contract TestLibs is
         return fillOrderCalldata;
     }
 
-    function publicGetPartialAmount(
+    function publicGetPartialAmountFloor(
         uint256 numerator,
         uint256 denominator,
         uint256 target
@@ -58,7 +58,7 @@ contract TestLibs is
         pure
         returns (uint256 partialAmount)
     {
-        partialAmount = getPartialAmount(
+        partialAmount = getPartialAmountFloor(
             numerator,
             denominator,
             target
@@ -83,7 +83,7 @@ contract TestLibs is
         return partialAmount;
     }
 
-    function publicIsRoundingError(
+    function publicIsRoundingErrorFloor(
         uint256 numerator,
         uint256 denominator,
         uint256 target
@@ -92,7 +92,7 @@ contract TestLibs is
         pure
         returns (bool isError)
     {
-        isError = isRoundingError(
+        isError = isRoundingErrorFloor(
             numerator,
             denominator,
             target
diff --git a/packages/contracts/test/exchange/internal.ts b/packages/contracts/test/exchange/internal.ts
index 0c6ab3707..14c0ff4cb 100644
--- a/packages/contracts/test/exchange/internal.ts
+++ b/packages/contracts/test/exchange/internal.ts
@@ -75,7 +75,7 @@ describe('Exchange core internal functions', () => {
     // Note(albrow): Don't forget to add beforeEach and afterEach calls to reset
     // the blockchain state for any tests which modify it!
 
-    async function referenceGetPartialAmountAsync(
+    async function referenceGetPartialAmountFloorAsync(
         numerator: BigNumber,
         denominator: BigNumber,
         target: BigNumber,
@@ -165,18 +165,18 @@ describe('Exchange core internal functions', () => {
             // implementation or the Solidity implementation of
             // calculateFillResults.
             return {
-                makerAssetFilledAmount: await referenceGetPartialAmountAsync(
+                makerAssetFilledAmount: await referenceGetPartialAmountFloorAsync(
                     takerAssetFilledAmount,
                     orderTakerAssetAmount,
                     otherAmount,
                 ),
                 takerAssetFilledAmount,
-                makerFeePaid: await referenceGetPartialAmountAsync(
+                makerFeePaid: await referenceGetPartialAmountFloorAsync(
                     takerAssetFilledAmount,
                     orderTakerAssetAmount,
                     otherAmount,
                 ),
-                takerFeePaid: await referenceGetPartialAmountAsync(
+                takerFeePaid: await referenceGetPartialAmountFloorAsync(
                     takerAssetFilledAmount,
                     orderTakerAssetAmount,
                     otherAmount,
@@ -199,18 +199,18 @@ describe('Exchange core internal functions', () => {
         );
     });
 
-    describe('getPartialAmount', async () => {
-        async function testGetPartialAmountAsync(
+    describe('getPartialAmountFloor', async () => {
+        async function testGetPartialAmountFloorAsync(
             numerator: BigNumber,
             denominator: BigNumber,
             target: BigNumber,
         ): Promise<BigNumber> {
-            return testExchange.publicGetPartialAmount.callAsync(numerator, denominator, target);
+            return testExchange.publicGetPartialAmountFloor.callAsync(numerator, denominator, target);
         }
         await testCombinatoriallyWithReferenceFuncAsync(
             'getPartialAmount',
-            referenceGetPartialAmountAsync,
-            testGetPartialAmountAsync,
+            referenceGetPartialAmountFloorAsync,
+            testGetPartialAmountFloorAsync,
             [uint256Values, uint256Values, uint256Values],
         );
     });
@@ -284,7 +284,7 @@ describe('Exchange core internal functions', () => {
             denominator: BigNumber,
             target: BigNumber,
         ): Promise<boolean> {
-            return testExchange.publicIsRoundingError.callAsync(numerator, denominator, target);
+            return testExchange.publicIsRoundingErrorFloor.callAsync(numerator, denominator, target);
         }
         await testCombinatoriallyWithReferenceFuncAsync(
             'isRoundingError',
diff --git a/packages/contracts/test/exchange/libs.ts b/packages/contracts/test/exchange/libs.ts
index a5f31a498..37234489e 100644
--- a/packages/contracts/test/exchange/libs.ts
+++ b/packages/contracts/test/exchange/libs.ts
@@ -77,7 +77,7 @@ describe('Exchange libs', () => {
                 const denominator = new BigNumber(999);
                 const target = new BigNumber(50);
                 // rounding error = ((20*50/999) - floor(20*50/999)) / (20*50/999) = 0.1%
-                const isRoundingError = await libs.publicIsRoundingError.callAsync(numerator, denominator, target);
+                const isRoundingError = await libs.publicIsRoundingErrorFloor.callAsync(numerator, denominator, target);
                 expect(isRoundingError).to.be.true();
             });
             it('should return false if there is a rounding of 0.09%', async () => {
@@ -85,7 +85,7 @@ describe('Exchange libs', () => {
                 const denominator = new BigNumber(9991);
                 const target = new BigNumber(500);
                 // rounding error = ((20*500/9991) - floor(20*500/9991)) / (20*500/9991) = 0.09%
-                const isRoundingError = await libs.publicIsRoundingError.callAsync(numerator, denominator, target);
+                const isRoundingError = await libs.publicIsRoundingErrorFloor.callAsync(numerator, denominator, target);
                 expect(isRoundingError).to.be.false();
             });
             it('should return true if there is a rounding error of 0.11%', async () => {
@@ -93,7 +93,7 @@ describe('Exchange libs', () => {
                 const denominator = new BigNumber(9989);
                 const target = new BigNumber(500);
                 // rounding error = ((20*500/9989) - floor(20*500/9989)) / (20*500/9989) = 0.011%
-                const isRoundingError = await libs.publicIsRoundingError.callAsync(numerator, denominator, target);
+                const isRoundingError = await libs.publicIsRoundingErrorFloor.callAsync(numerator, denominator, target);
                 expect(isRoundingError).to.be.true();
             });
         });
diff --git a/packages/contracts/test/utils/fill_order_combinatorial_utils.ts b/packages/contracts/test/utils/fill_order_combinatorial_utils.ts
index a9318571c..92d0f4003 100644
--- a/packages/contracts/test/utils/fill_order_combinatorial_utils.ts
+++ b/packages/contracts/test/utils/fill_order_combinatorial_utils.ts
@@ -467,17 +467,17 @@ export class FillOrderCombinatorialUtils {
             ? remainingTakerAmountToFill
             : alreadyFilledTakerAmount.add(takerAssetFillAmount);
 
-        const expFilledMakerAmount = orderUtils.getPartialAmount(
+        const expFilledMakerAmount = orderUtils.getPartialAmountFloor(
             expFilledTakerAmount,
             signedOrder.takerAssetAmount,
             signedOrder.makerAssetAmount,
         );
-        const expMakerFeePaid = orderUtils.getPartialAmount(
+        const expMakerFeePaid = orderUtils.getPartialAmountFloor(
             expFilledTakerAmount,
             signedOrder.takerAssetAmount,
             signedOrder.makerFee,
         );
-        const expTakerFeePaid = orderUtils.getPartialAmount(
+        const expTakerFeePaid = orderUtils.getPartialAmountFloor(
             expFilledTakerAmount,
             signedOrder.takerAssetAmount,
             signedOrder.takerFee,
@@ -668,7 +668,7 @@ export class FillOrderCombinatorialUtils {
         signedOrder: SignedOrder,
         takerAssetFillAmount: BigNumber,
     ): Promise<void> {
-        const makerAssetFillAmount = orderUtils.getPartialAmount(
+        const makerAssetFillAmount = orderUtils.getPartialAmountFloor(
             takerAssetFillAmount,
             signedOrder.takerAssetAmount,
             signedOrder.makerAssetAmount,
@@ -705,7 +705,7 @@ export class FillOrderCombinatorialUtils {
                 );
         }
 
-        const makerFee = orderUtils.getPartialAmount(
+        const makerFee = orderUtils.getPartialAmountFloor(
             takerAssetFillAmount,
             signedOrder.takerAssetAmount,
             signedOrder.makerFee,
@@ -829,7 +829,7 @@ export class FillOrderCombinatorialUtils {
                 );
         }
 
-        const takerFee = orderUtils.getPartialAmount(
+        const takerFee = orderUtils.getPartialAmountFloor(
             takerAssetFillAmount,
             signedOrder.takerAssetAmount,
             signedOrder.takerFee,
diff --git a/packages/contracts/test/utils/order_utils.ts b/packages/contracts/test/utils/order_utils.ts
index 019f6e74b..444e27c44 100644
--- a/packages/contracts/test/utils/order_utils.ts
+++ b/packages/contracts/test/utils/order_utils.ts
@@ -5,7 +5,7 @@ import { constants } from './constants';
 import { CancelOrder, MatchOrder } from './types';
 
 export const orderUtils = {
-    getPartialAmount(numerator: BigNumber, denominator: BigNumber, target: BigNumber): BigNumber {
+    getPartialAmountFloor(numerator: BigNumber, denominator: BigNumber, target: BigNumber): BigNumber {
         const partialAmount = numerator
             .mul(target)
             .div(denominator)
-- 
cgit v1.2.3


From bc686fcbf3a0f9eea0c96107b9880314027f2d02 Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Fri, 24 Aug 2018 16:17:02 -0700
Subject: Stylistic fixes

---
 .../src/2.0.0/protocol/Exchange/libs/LibMath.sol   | 25 +++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

(limited to 'packages/contracts')

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 7d58f70ce..0e0fba5d2 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
@@ -39,7 +39,10 @@ contract LibMath is
         pure
         returns (uint256 partialAmount)
     {
-        require(denominator > 0, "DIVISION_BY_ZERO");
+        require(
+            denominator > 0,
+            "DIVISION_BY_ZERO"
+        );
         
         partialAmount = safeDiv(
             safeMul(numerator, target),
@@ -62,13 +65,19 @@ contract LibMath is
         pure
         returns (uint256 partialAmount)
     {
-        require(denominator > 0, "DIVISION_BY_ZERO");
+        require(
+            denominator > 0,
+            "DIVISION_BY_ZERO"
+        );
         
         // safeDiv computes `floor(a / b)`. We use the identity (a, b integer):
         //       ceil(a / b) = floor((a + b - 1) / b)
         // To implement `ceil(a / b)` using safeDiv.
         partialAmount = safeDiv(
-            safeAdd(safeMul(numerator, target), safeSub(denominator, 1)),
+            safeAdd(
+                safeMul(numerator, target),
+                safeSub(denominator, 1)
+            ),
             denominator
         );
         return partialAmount;
@@ -88,7 +97,10 @@ contract LibMath is
         pure
         returns (bool isError)
     {
-        require(denominator > 0, "DIVISION_BY_ZERO");
+        require(
+            denominator > 0,
+            "DIVISION_BY_ZERO"
+        );
         
         // The absolute rounding error is the difference between the rounded
         // value and the ideal value. The relative rounding error is the
@@ -135,7 +147,10 @@ contract LibMath is
         pure
         returns (bool isError)
     {
-        require(denominator > 0, "DIVISION_BY_ZERO");
+        require(
+            denominator > 0,
+            "DIVISION_BY_ZERO"
+        );
         
         // See the comments in `isRoundingError`.
         if (target == 0 || numerator == 0) {
-- 
cgit v1.2.3


From 11328bd93d498abfcfbfa38ed69db8cb268f12be Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Fri, 24 Aug 2018 16:26:48 -0700
Subject: Skip self-transfers

---
 .../contracts/src/2.0.0/protocol/Exchange/MixinAssetProxyDispatcher.sol | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(limited to 'packages/contracts')

diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinAssetProxyDispatcher.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinAssetProxyDispatcher.sol
index e9f882194..80475e6e3 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinAssetProxyDispatcher.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinAssetProxyDispatcher.sol
@@ -83,7 +83,7 @@ contract MixinAssetProxyDispatcher is
         internal
     {
         // Do nothing if no amount should be transferred.
-        if (amount > 0) {
+        if (amount > 0 && from != to) {
             // Ensure assetData length is valid
             require(
                 assetData.length > 3,
-- 
cgit v1.2.3


From cc1fac9bbee2656bdb327490de42922abfc5125a Mon Sep 17 00:00:00 2001
From: Amir Bandeali <abandeali1@gmail.com>
Date: Fri, 24 Aug 2018 17:02:42 -0700
Subject: Fix linting errors

---
 packages/contracts/test/exchange/internal.ts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

(limited to 'packages/contracts')

diff --git a/packages/contracts/test/exchange/internal.ts b/packages/contracts/test/exchange/internal.ts
index 14c0ff4cb..2521665c2 100644
--- a/packages/contracts/test/exchange/internal.ts
+++ b/packages/contracts/test/exchange/internal.ts
@@ -220,9 +220,9 @@ describe('Exchange core internal functions', () => {
             numerator: BigNumber,
             denominator: BigNumber,
             target: BigNumber,
-        ) {
+        ): Promise<BigNumber> {
             if (denominator.eq(0)) {
-                throw new Error('revert DIVISION_BY_ZERO');
+                throw divisionByZeroErrorForCall;
             }
             const product = numerator.mul(target);
             const offset = product.add(denominator.sub(1));
-- 
cgit v1.2.3


From 044415e23d3a079e6301d8fb838da60456794c4f Mon Sep 17 00:00:00 2001
From: Amir Bandeali <abandeali1@gmail.com>
Date: Mon, 20 Aug 2018 17:37:23 -0700
Subject: Remove redundant sload from getCurrentContextAddress

---
 packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

(limited to 'packages/contracts')

diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol
index 821d30279..4a59b6c0f 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol
@@ -155,7 +155,8 @@ contract MixinTransactions is
         view
         returns (address)
     {
-        address contextAddress = currentContextAddress == address(0) ? msg.sender : currentContextAddress;
+        address currentContextAddress_ = currentContextAddress;
+        address contextAddress = currentContextAddress_ == address(0) ? msg.sender : currentContextAddress_;
         return contextAddress;
     }
 }
-- 
cgit v1.2.3


From d8cb56caa33885397f557afac5a4ccb24efb47d0 Mon Sep 17 00:00:00 2001
From: Amir Bandeali <abandeali1@gmail.com>
Date: Mon, 20 Aug 2018 17:38:25 -0700
Subject: Add ReentrancyGuard contract

---
 .../utils/ReentrancyGuard/ReentrancyGuard.sol      | 44 ++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol

(limited to 'packages/contracts')

diff --git a/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol b/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol
new file mode 100644
index 000000000..70fc4ca6b
--- /dev/null
+++ b/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol
@@ -0,0 +1,44 @@
+/*
+
+  Copyright 2018 ZeroEx Intl.
+
+  Licensed under the Apache License, Version 2.0 (the "License");
+  you may not use this file except in compliance with the License.
+  You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+
+*/
+pragma solidity 0.4.24;
+
+
+contract ReentrancyGuard {
+
+    // Locked state of mutex
+    bool private locked = false;
+
+    /// @dev Functions with this modifier cannot be reentered.
+    modifier nonReentrant() {
+        // Ensure mutex is unlocked
+        require(
+            !locked,
+            "REENTRANCY_ILLEGAL"
+        );
+
+        // Lock mutex before function call
+        locked = true;
+
+        // Perform function call
+        _;
+
+        // Unlock mutex after function call
+        locked = false;
+    }
+
+}
-- 
cgit v1.2.3


From 6f88e9bdbdd8f44c45053b8c17c84411f9499084 Mon Sep 17 00:00:00 2001
From: Amir Bandeali <abandeali1@gmail.com>
Date: Tue, 21 Aug 2018 16:06:21 -0700
Subject: Add internal fill functions, add reentrancy guard to public functions
 that make external calls

---
 .../2.0.0/protocol/Exchange/MixinExchangeCore.sol  | 86 ++++++++++++++--------
 .../2.0.0/protocol/Exchange/MixinMatchOrders.sol   |  3 +
 .../protocol/Exchange/MixinWrapperFunctions.sol    | 46 +++++++++---
 .../protocol/Exchange/mixins/MExchangeCore.sol     | 13 ++++
 .../protocol/Exchange/mixins/MWrapperFunctions.sol | 40 ++++++++++
 .../utils/ReentrancyGuard/ReentrancyGuard.sol      |  1 -
 6 files changed, 148 insertions(+), 41 deletions(-)
 create mode 100644 packages/contracts/src/2.0.0/protocol/Exchange/mixins/MWrapperFunctions.sol

(limited to 'packages/contracts')

diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
index f3a0591b2..d00323b15 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
@@ -19,6 +19,7 @@
 pragma solidity 0.4.24;
 pragma experimental ABIEncoderV2;
 
+import "../../utils/ReentrancyGuard/ReentrancyGuard.sol";
 import "./libs/LibConstants.sol";
 import "./libs/LibFillResults.sol";
 import "./libs/LibOrder.sol";
@@ -30,6 +31,7 @@ import "./mixins/MAssetProxyDispatcher.sol";
 
 
 contract MixinExchangeCore is
+    ReentrancyGuard,
     LibConstants,
     LibMath,
     LibOrder,
@@ -86,43 +88,14 @@ contract MixinExchangeCore is
         bytes memory signature
     )
         public
+        nonReentrant
         returns (FillResults memory fillResults)
     {
-        // Fetch order info
-        OrderInfo memory orderInfo = getOrderInfo(order);
-
-        // Fetch taker address
-        address takerAddress = getCurrentContextAddress();
-
-        // Get amount of takerAsset to fill
-        uint256 remainingTakerAssetAmount = safeSub(order.takerAssetAmount, orderInfo.orderTakerAssetFilledAmount);
-        uint256 takerAssetFilledAmount = min256(takerAssetFillAmount, remainingTakerAssetAmount);
-
-        // Validate context
-        assertValidFill(
+        fillResults = fillOrderInternal(
             order,
-            orderInfo,
-            takerAddress,
             takerAssetFillAmount,
-            takerAssetFilledAmount,
             signature
         );
-
-        // Compute proportional fill amounts
-        fillResults = calculateFillResults(order, takerAssetFilledAmount);
-
-        // Update exchange internal state
-        updateFilledState(
-            order,
-            takerAddress,
-            orderInfo.orderHash,
-            orderInfo.orderTakerAssetFilledAmount,
-            fillResults
-        );
-    
-        // Settle order
-        settleOrder(order, takerAddress, fillResults);
-
         return fillResults;
     }
 
@@ -203,6 +176,57 @@ contract MixinExchangeCore is
         return orderInfo;
     }
 
+    /// @dev Fills the input order.
+    /// @param order Order struct containing order specifications.
+    /// @param takerAssetFillAmount Desired amount of takerAsset to sell.
+    /// @param signature Proof that order has been created by maker.
+    /// @return Amounts filled and fees paid by maker and taker.
+    function fillOrderInternal(
+        Order memory order,
+        uint256 takerAssetFillAmount,
+        bytes memory signature
+    )
+        internal
+        returns (FillResults memory fillResults)
+    {
+        // Fetch order info
+        OrderInfo memory orderInfo = getOrderInfo(order);
+
+        // Fetch taker address
+        address takerAddress = getCurrentContextAddress();
+
+        // Get amount of takerAsset to fill
+        uint256 remainingTakerAssetAmount = safeSub(order.takerAssetAmount, orderInfo.orderTakerAssetFilledAmount);
+        uint256 takerAssetFilledAmount = min256(takerAssetFillAmount, remainingTakerAssetAmount);
+
+        // Validate context
+        assertValidFill(
+            order,
+            orderInfo,
+            takerAddress,
+            takerAssetFillAmount,
+            takerAssetFilledAmount,
+            signature
+        );
+
+        // Compute proportional fill amounts
+        fillResults = calculateFillResults(order, takerAssetFilledAmount);
+
+        // Update exchange internal state
+        updateFilledState(
+            order,
+            takerAddress,
+            orderInfo.orderHash,
+            orderInfo.orderTakerAssetFilledAmount,
+            fillResults
+        );
+    
+        // Settle order
+        settleOrder(order, takerAddress, fillResults);
+
+        return fillResults;
+    }
+
     /// @dev Updates state with results of a fill order.
     /// @param order that was filled.
     /// @param takerAddress Address of taker who filled the order.
diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
index 20f4b1c33..25220a673 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
@@ -14,6 +14,7 @@
 pragma solidity 0.4.24;
 pragma experimental ABIEncoderV2;
 
+import "../../utils/ReentrancyGuard/ReentrancyGuard.sol";
 import "./libs/LibConstants.sol";
 import "./libs/LibMath.sol";
 import "./libs/LibOrder.sol";
@@ -25,6 +26,7 @@ import "./mixins/MAssetProxyDispatcher.sol";
 
 
 contract MixinMatchOrders is
+    ReentrancyGuard,
     LibConstants,
     LibMath,
     MAssetProxyDispatcher,
@@ -48,6 +50,7 @@ contract MixinMatchOrders is
         bytes memory rightSignature
     )
         public
+        nonReentrant
         returns (LibFillResults.MatchedFillResults memory matchedFillResults)
     {
         // We assume that rightOrder.takerAssetData == leftOrder.makerAssetData and rightOrder.makerAssetData == leftOrder.takerAssetData.
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 1b77cfbcb..ff1cb6995 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol
@@ -19,14 +19,17 @@
 pragma solidity 0.4.24;
 pragma experimental ABIEncoderV2;
 
+import "../../utils/ReentrancyGuard/ReentrancyGuard.sol";
 import "./libs/LibMath.sol";
 import "./libs/LibOrder.sol";
 import "./libs/LibFillResults.sol";
 import "./libs/LibAbiEncoder.sol";
 import "./mixins/MExchangeCore.sol";
+import "./mixins/MWrapperFunctions.sol";
 
 
 contract MixinWrapperFunctions is
+    ReentrancyGuard,
     LibMath,
     LibFillResults,
     LibAbiEncoder,
@@ -43,17 +46,14 @@ contract MixinWrapperFunctions is
         bytes memory signature
     )
         public
+        nonReentrant
         returns (FillResults memory fillResults)
     {
-        fillResults = fillOrder(
+        fillResults = fillOrKillOrderInternal(
             order,
             takerAssetFillAmount,
             signature
         );
-        require(
-            fillResults.takerAssetFilledAmount == takerAssetFillAmount,
-            "COMPLETE_FILL_FAILED"
-        );
         return fillResults;
     }
 
@@ -117,11 +117,12 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
+        nonReentrant
         returns (FillResults memory totalFillResults)
     {
         uint256 ordersLength = orders.length;
         for (uint256 i = 0; i != ordersLength; i++) {
-            FillResults memory singleFillResults = fillOrder(
+            FillResults memory singleFillResults = fillOrderInternal(
                 orders[i],
                 takerAssetFillAmounts[i],
                 signatures[i]
@@ -143,11 +144,12 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
+        nonReentrant
         returns (FillResults memory totalFillResults)
     {
         uint256 ordersLength = orders.length;
         for (uint256 i = 0; i != ordersLength; i++) {
-            FillResults memory singleFillResults = fillOrKillOrder(
+            FillResults memory singleFillResults = fillOrKillOrderInternal(
                 orders[i],
                 takerAssetFillAmounts[i],
                 signatures[i]
@@ -195,6 +197,7 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
+        nonReentrant
         returns (FillResults memory totalFillResults)
     {
         bytes memory takerAssetData = orders[0].takerAssetData;
@@ -210,7 +213,7 @@ contract MixinWrapperFunctions is
             uint256 remainingTakerAssetFillAmount = safeSub(takerAssetFillAmount, totalFillResults.takerAssetFilledAmount);
 
             // Attempt to sell the remaining amount of takerAsset
-            FillResults memory singleFillResults = fillOrder(
+            FillResults memory singleFillResults = fillOrderInternal(
                 orders[i],
                 remainingTakerAssetFillAmount,
                 signatures[i]
@@ -282,6 +285,7 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
+        nonReentrant
         returns (FillResults memory totalFillResults)
     {
         bytes memory makerAssetData = orders[0].makerAssetData;
@@ -305,7 +309,7 @@ contract MixinWrapperFunctions is
             );
 
             // Attempt to sell the remaining amount of takerAsset
-            FillResults memory singleFillResults = fillOrder(
+            FillResults memory singleFillResults = fillOrderInternal(
                 orders[i],
                 remainingTakerAssetFillAmount,
                 signatures[i]
@@ -400,4 +404,28 @@ contract MixinWrapperFunctions is
         }
         return ordersInfo;
     }
+
+    /// @dev Fills the input order. Reverts if exact takerAssetFillAmount not filled.
+    /// @param order Order struct containing order specifications.
+    /// @param takerAssetFillAmount Desired amount of takerAsset to sell.
+    /// @param signature Proof that order has been created by maker.
+    function fillOrKillOrderInternal(
+        LibOrder.Order memory order,
+        uint256 takerAssetFillAmount,
+        bytes memory signature
+    )
+        internal
+        returns (FillResults memory fillResults)
+    {
+        fillResults = fillOrderInternal(
+            order,
+            takerAssetFillAmount,
+            signature
+        );
+        require(
+            fillResults.takerAssetFilledAmount == takerAssetFillAmount,
+            "COMPLETE_FILL_FAILED"
+        );
+        return fillResults;
+    }
 }
diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MExchangeCore.sol
index c165b647c..41832fe4b 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MExchangeCore.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MExchangeCore.sol
@@ -59,6 +59,19 @@ contract MExchangeCore is
         uint256 orderEpoch                    // Orders with specified makerAddress and senderAddress with a salt less than this value are considered cancelled.
     );
 
+    /// @dev Fills the input order.
+    /// @param order Order struct containing order specifications.
+    /// @param takerAssetFillAmount Desired amount of takerAsset to sell.
+    /// @param signature Proof that order has been created by maker.
+    /// @return Amounts filled and fees paid by maker and taker.
+    function fillOrderInternal(
+        LibOrder.Order memory order,
+        uint256 takerAssetFillAmount,
+        bytes memory signature
+    )
+        internal
+        returns (LibFillResults.FillResults memory fillResults);
+
     /// @dev Updates state with results of a fill order.
     /// @param order that was filled.
     /// @param takerAddress Address of taker who filled the order.
diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MWrapperFunctions.sol b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MWrapperFunctions.sol
new file mode 100644
index 000000000..e04d4a429
--- /dev/null
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MWrapperFunctions.sol
@@ -0,0 +1,40 @@
+/*
+
+  Copyright 2018 ZeroEx Intl.
+
+  Licensed under the Apache License, Version 2.0 (the "License");
+  you may not use this file except in compliance with the License.
+  You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+
+*/
+
+pragma solidity 0.4.24;
+pragma experimental ABIEncoderV2;
+
+import "../libs/LibOrder.sol";
+import "../libs/LibFillResults.sol";
+import "../interfaces/IWrapperFunctions.sol";
+
+
+contract MWrapperFunctions {
+
+    /// @dev Fills the input order. Reverts if exact takerAssetFillAmount not filled.
+    /// @param order LibOrder.Order struct containing order specifications.
+    /// @param takerAssetFillAmount Desired amount of takerAsset to sell.
+    /// @param signature Proof that order has been created by maker.
+    function fillOrKillOrderInternal(
+        LibOrder.Order memory order,
+        uint256 takerAssetFillAmount,
+        bytes memory signature
+    )
+        internal
+        returns (LibFillResults.FillResults memory fillResults);
+}
diff --git a/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol b/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol
index 70fc4ca6b..5d6f6970d 100644
--- a/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol
+++ b/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol
@@ -40,5 +40,4 @@ contract ReentrancyGuard {
         // Unlock mutex after function call
         locked = false;
     }
-
 }
-- 
cgit v1.2.3


From cf12daea2f3a907f6a111c12d1e67c2e8b0a8797 Mon Sep 17 00:00:00 2001
From: Amir Bandeali <abandeali1@gmail.com>
Date: Tue, 21 Aug 2018 17:53:40 -0700
Subject: Add ReentrantToken

---
 packages/contracts/compiler.json                   |   1 +
 packages/contracts/package.json                    |   2 +-
 .../ReentrantERC20Token/ReentrantERC20Token.sol    | 173 +++++++++++++++++++++
 3 files changed, 175 insertions(+), 1 deletion(-)
 create mode 100644 packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol

(limited to 'packages/contracts')

diff --git a/packages/contracts/compiler.json b/packages/contracts/compiler.json
index 27bb69a36..f66114e87 100644
--- a/packages/contracts/compiler.json
+++ b/packages/contracts/compiler.json
@@ -40,6 +40,7 @@
         "MultiSigWallet",
         "MultiSigWalletWithTimeLock",
         "OrderValidator",
+        "ReentrantERC20Token",
         "TestAssetProxyOwner",
         "TestAssetProxyDispatcher",
         "TestConstants",
diff --git a/packages/contracts/package.json b/packages/contracts/package.json
index 0ead2f43f..8d75eb4ce 100644
--- a/packages/contracts/package.json
+++ b/packages/contracts/package.json
@@ -34,7 +34,7 @@
     },
     "config": {
         "abis":
-            "../migrations/artifacts/2.0.0/@(AssetProxyOwner|DummyERC20Token|DummyERC721Receiver|DummyERC721Token|DummyNoReturnERC20Token|ERC20Proxy|ERC721Proxy|Forwarder|Exchange|ExchangeWrapper|IAssetData|IAssetProxy|InvalidERC721Receiver|MixinAuthorizable|MultiSigWallet|MultiSigWalletWithTimeLock|OrderValidator|TestAssetProxyOwner|TestAssetProxyDispatcher|TestConstants|TestExchangeInternals|TestLibBytes|TestLibs|TestSignatureValidator|TestStaticCallReceiver|Validator|Wallet|TokenRegistry|Whitelist|WETH9|ZRXToken).json"
+            "../migrations/artifacts/2.0.0/@(AssetProxyOwner|DummyERC20Token|DummyERC721Receiver|DummyERC721Token|DummyNoReturnERC20Token|ERC20Proxy|ERC721Proxy|Forwarder|Exchange|ExchangeWrapper|IAssetData|IAssetProxy|InvalidERC721Receiver|MixinAuthorizable|MultiSigWallet|MultiSigWalletWithTimeLock|OrderValidator|ReentrantERC20Token|TestAssetProxyOwner|TestAssetProxyDispatcher|TestConstants|TestExchangeInternals|TestLibBytes|TestLibs|TestSignatureValidator|Validator|Wallet|TokenRegistry|Whitelist|WETH9|ZRXToken).json"
     },
     "repository": {
         "type": "git",
diff --git a/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol b/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol
new file mode 100644
index 000000000..f5c98177f
--- /dev/null
+++ b/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol
@@ -0,0 +1,173 @@
+/*
+
+  Copyright 2018 ZeroEx Intl.
+
+  Licensed under the Apache License, Version 2.0 (the "License");
+  you may not use this file except in compliance with the License.
+  You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+
+*/
+
+pragma solidity 0.4.24;
+pragma experimental ABIEncoderV2;
+
+import "../../tokens/ERC20Token/ERC20Token.sol";
+import "../../protocol/Exchange/interfaces/IExchange.sol";
+import "../../protocol/Exchange/libs/LibOrder.sol";
+
+
+contract ReentrantERC20Token is
+    ERC20Token
+{
+    // solhint-disable-next-line var-name-mixedcase
+    IExchange internal EXCHANGE;
+
+    // All of these functions are potentially vulnerable to reentrancy
+    enum ExchangeFunction {
+        FILL_ORDER,
+        FILL_OR_KILL_ORDER,
+        FILL_ORDER_NO_THROW,
+        BATCH_FILL_ORDERS,
+        BATCH_FILL_OR_KILL_ORDERS,
+        BATCH_FILL_ORDERS_NO_THROW,
+        MARKET_BUY_ORDERS,
+        MARKET_BUY_ORDERS_NO_THROW,
+        MARKET_SELL_ORDERS,
+        MARKET_SELL_ORDERS_NO_THROW,
+        MATCH_ORDERS
+    }
+
+    uint8 internal currentFunctionId = 0;
+
+    constructor (address _exchange)
+        public
+    {
+        EXCHANGE = IExchange(_exchange);
+    }
+
+    /// @dev Set the current function that will be called when `transferFrom` is called.
+    /// @param _currentFunctionId Id that corresponds to function name.
+    function setCurrentFunction(uint8 _currentFunctionId)
+        external
+    {
+        currentFunctionId = _currentFunctionId;
+    }
+
+    /// @dev A version of `transferFrom` that attempts to reenter the Exchange contract.
+    /// @param _from The address of the sender
+    /// @param _to The address of the recipient
+    /// @param _value The amount of token to be transferred
+    function transferFrom(
+        address _from,
+        address _to,
+        uint256 _value
+    )
+        external
+        returns (bool)
+    {
+        // This order would normally be invalid, but it will be used strictly for testing reentrnacy.
+        // Any reentrancy checks will happen before any other checks that invalidate the order.
+        LibOrder.Order memory order = LibOrder.Order({
+            makerAddress: address(0),
+            takerAddress: address(0),
+            feeRecipientAddress: address(0),
+            senderAddress: address(0),
+            makerAssetAmount: 0,
+            takerAssetAmount: 0,
+            makerFee: 0,
+            takerFee: 0,
+            expirationTimeSeconds: 0,
+            salt: 0,
+            makerAssetData: "",
+            takerAssetData: ""
+        });
+
+        // Initialize remaining null parameters
+        bytes memory signature = "";
+        LibOrder.Order[] memory orders = new LibOrder.Order[](1);
+        orders[0] = order;
+        uint256[] memory takerAssetFillAmounts = new uint256[](1);
+        takerAssetFillAmounts[0] = 0;
+        bytes[] memory signatures = new bytes[](1);
+        signatures[0] = signature;
+    
+        // Attempt to reenter Exchange by calling function with currentFunctionId
+        if (currentFunctionId == uint8(ExchangeFunction.FILL_ORDER)) {
+            EXCHANGE.fillOrder(
+                order,
+                0,
+                signature
+            );
+        } else if (currentFunctionId == uint8(ExchangeFunction.FILL_OR_KILL_ORDER)) {
+            EXCHANGE.fillOrKillOrder(
+                order,
+                0,
+                signature
+            );
+        } else if (currentFunctionId == uint8(ExchangeFunction.FILL_ORDER_NO_THROW)) {
+            EXCHANGE.fillOrderNoThrow(
+                order,
+                0,
+                signature
+            );
+        } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_FILL_ORDERS)) {
+            EXCHANGE.batchFillOrders(
+                orders,
+                takerAssetFillAmounts,
+                signatures
+            );
+        } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_FILL_OR_KILL_ORDERS)) {
+            EXCHANGE.batchFillOrKillOrders(
+                orders,
+                takerAssetFillAmounts,
+                signatures
+            );
+        } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_FILL_ORDERS_NO_THROW)) {
+            EXCHANGE.batchFillOrdersNoThrow(
+                orders,
+                takerAssetFillAmounts,
+                signatures
+            );
+        } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_BUY_ORDERS)) {
+            EXCHANGE.marketBuyOrders(
+                orders,
+                0,
+                signatures
+            );
+        } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_BUY_ORDERS_NO_THROW)) {
+            EXCHANGE.marketBuyOrdersNoThrow(
+                orders,
+                0,
+                signatures
+            );
+        } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_SELL_ORDERS)) {
+            EXCHANGE.marketSellOrders(
+                orders,
+                0,
+                signatures
+            );
+        } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_SELL_ORDERS_NO_THROW)) {
+            EXCHANGE.marketSellOrdersNoThrow(
+                orders,
+                0,
+                signatures
+            );
+        } else if (currentFunctionId == uint8(ExchangeFunction.MATCH_ORDERS)) {
+            EXCHANGE.matchOrders(
+                order,
+                order,
+                signature,
+                signature
+            );
+        }
+        return true;
+    }
+}
\ No newline at end of file
-- 
cgit v1.2.3


From 6d0dedc62cb1be54dbdd287fd59d7b568a199007 Mon Sep 17 00:00:00 2001
From: Amir Bandeali <abandeali1@gmail.com>
Date: Wed, 22 Aug 2018 17:57:57 -0700
Subject: Split modifiers into check only and check, lock, unlock

---
 .../src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol     | 13 +++++++++++++
 1 file changed, 13 insertions(+)

(limited to 'packages/contracts')

diff --git a/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol b/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol
index 5d6f6970d..2b980c7ca 100644
--- a/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol
+++ b/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol
@@ -31,6 +31,19 @@ contract ReentrancyGuard {
             "REENTRANCY_ILLEGAL"
         );
 
+        // Perform function call
+        _;
+    }
+
+    /// @dev Functions with this modifer cannot be reentered. The mutex will be locked
+    ///      before function execution and unlocked after.
+    modifier lockMutex() {
+        // Ensure mutex is unlocked
+        require(
+            !locked,
+            "REENTRANCY_ILLEGAL"
+        );
+
         // Lock mutex before function call
         locked = true;
 
-- 
cgit v1.2.3


From c75212bef0b3801ecda09a89d5e30e359dcf1b63 Mon Sep 17 00:00:00 2001
From: Amir Bandeali <abandeali1@gmail.com>
Date: Wed, 22 Aug 2018 17:58:47 -0700
Subject: Add nonReentrant modifiers on functions that use
 getCurrentContextAddress only, add lockMutex modifier on functions that make
 external calls

---
 .../2.0.0/protocol/Exchange/MixinExchangeCore.sol  |  4 +++-
 .../2.0.0/protocol/Exchange/MixinMatchOrders.sol   |  2 +-
 .../protocol/Exchange/MixinSignatureValidator.sol  |  3 +++
 .../protocol/Exchange/MixinWrapperFunctions.sol    | 23 ++++++++++------------
 4 files changed, 17 insertions(+), 15 deletions(-)

(limited to 'packages/contracts')

diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
index d00323b15..f02514fc6 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
@@ -56,6 +56,7 @@ contract MixinExchangeCore is
     /// @param targetOrderEpoch Orders created with a salt less or equal to this value will be cancelled.
     function cancelOrdersUpTo(uint256 targetOrderEpoch)
         external
+        nonReentrant
     {
         address makerAddress = getCurrentContextAddress();
         // If this function is called via `executeTransaction`, we only update the orderEpoch for the makerAddress/msg.sender combination.
@@ -88,7 +89,7 @@ contract MixinExchangeCore is
         bytes memory signature
     )
         public
-        nonReentrant
+        lockMutex
         returns (FillResults memory fillResults)
     {
         fillResults = fillOrderInternal(
@@ -104,6 +105,7 @@ contract MixinExchangeCore is
     /// @param order Order to cancel. Order must be OrderStatus.FILLABLE.
     function cancelOrder(Order memory order)
         public
+        nonReentrant
     {
         // Fetch current order status
         OrderInfo memory orderInfo = getOrderInfo(order);
diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
index 25220a673..39c47e6f9 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
@@ -50,7 +50,7 @@ contract MixinMatchOrders is
         bytes memory rightSignature
     )
         public
-        nonReentrant
+        lockMutex
         returns (LibFillResults.MatchedFillResults memory matchedFillResults)
     {
         // We assume that rightOrder.takerAssetData == leftOrder.makerAssetData and rightOrder.makerAssetData == leftOrder.takerAssetData.
diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol
index f30adcdb8..4eb6a2fa6 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol
@@ -19,6 +19,7 @@
 pragma solidity 0.4.24;
 
 import "../../utils/LibBytes/LibBytes.sol";
+import "../../utils/ReentrancyGuard/ReentrancyGuard.sol";
 import "./mixins/MSignatureValidator.sol";
 import "./mixins/MTransactions.sol";
 import "./interfaces/IWallet.sol";
@@ -26,6 +27,7 @@ import "./interfaces/IValidator.sol";
 
 
 contract MixinSignatureValidator is
+    ReentrancyGuard,
     MSignatureValidator,
     MTransactions
 {
@@ -69,6 +71,7 @@ contract MixinSignatureValidator is
         bool approval
     )
         external
+        nonReentrant
     {
         address signerAddress = getCurrentContextAddress();
         allowedValidators[signerAddress][validatorAddress] = approval;
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 ff1cb6995..b0474b110 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol
@@ -46,7 +46,7 @@ contract MixinWrapperFunctions is
         bytes memory signature
     )
         public
-        nonReentrant
+        lockMutex
         returns (FillResults memory fillResults)
     {
         fillResults = fillOrKillOrderInternal(
@@ -69,6 +69,7 @@ contract MixinWrapperFunctions is
         bytes memory signature
     )
         public
+        nonReentrant
         returns (FillResults memory fillResults)
     {
         // ABI encode calldata for `fillOrder`
@@ -88,14 +89,7 @@ contract MixinWrapperFunctions is
                 fillOrderCalldata,                  // write output over input
                 128                                 // output size is 128 bytes
             )
-            switch success
-            case 0 {
-                mstore(fillResults, 0)
-                mstore(add(fillResults, 32), 0)
-                mstore(add(fillResults, 64), 0)
-                mstore(add(fillResults, 96), 0)
-            }
-            case 1 {
+            if success {
                 mstore(fillResults, mload(fillOrderCalldata))
                 mstore(add(fillResults, 32), mload(add(fillOrderCalldata, 32)))
                 mstore(add(fillResults, 64), mload(add(fillOrderCalldata, 64)))
@@ -117,7 +111,7 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
-        nonReentrant
+        lockMutex
         returns (FillResults memory totalFillResults)
     {
         uint256 ordersLength = orders.length;
@@ -144,7 +138,7 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
-        nonReentrant
+        lockMutex
         returns (FillResults memory totalFillResults)
     {
         uint256 ordersLength = orders.length;
@@ -172,6 +166,7 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
+        nonReentrant
         returns (FillResults memory totalFillResults)
     {
         uint256 ordersLength = orders.length;
@@ -197,7 +192,7 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
-        nonReentrant
+        lockMutex
         returns (FillResults memory totalFillResults)
     {
         bytes memory takerAssetData = orders[0].takerAssetData;
@@ -242,6 +237,7 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
+        nonReentrant
         returns (FillResults memory totalFillResults)
     {
         bytes memory takerAssetData = orders[0].takerAssetData;
@@ -285,7 +281,7 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
-        nonReentrant
+        lockMutex
         returns (FillResults memory totalFillResults)
     {
         bytes memory makerAssetData = orders[0].makerAssetData;
@@ -338,6 +334,7 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
+        nonReentrant
         returns (FillResults memory totalFillResults)
     {
         bytes memory makerAssetData = orders[0].makerAssetData;
-- 
cgit v1.2.3


From 56c3c29febe6264eee7f1c3f1ed8f1dc7c4685c6 Mon Sep 17 00:00:00 2001
From: Amir Bandeali <abandeali1@gmail.com>
Date: Wed, 22 Aug 2018 18:00:01 -0700
Subject: Update ReentrantERC20Token with new functions and check that revert
 is occuring for correct reason

---
 .../ReentrantERC20Token/ReentrantERC20Token.sol    | 110 ++++++++++++++-------
 1 file changed, 75 insertions(+), 35 deletions(-)

(limited to 'packages/contracts')

diff --git a/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol b/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol
index f5c98177f..8d575d09a 100644
--- a/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol
+++ b/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol
@@ -19,6 +19,7 @@
 pragma solidity 0.4.24;
 pragma experimental ABIEncoderV2;
 
+import "../../utils/LibBytes/LibBytes.sol";
 import "../../tokens/ERC20Token/ERC20Token.sol";
 import "../../protocol/Exchange/interfaces/IExchange.sol";
 import "../../protocol/Exchange/libs/LibOrder.sol";
@@ -27,9 +28,17 @@ import "../../protocol/Exchange/libs/LibOrder.sol";
 contract ReentrantERC20Token is
     ERC20Token
 {
+
+    using LibBytes for bytes;
+
     // solhint-disable-next-line var-name-mixedcase
     IExchange internal EXCHANGE;
 
+    bytes internal constant REENTRANCY_ILLEGAL_REVERT_REASON = abi.encodeWithSelector(
+        bytes4(keccak256("Error(string)")),
+        "REENTRANCY_ILLEGAL"
+    );
+
     // All of these functions are potentially vulnerable to reentrancy
     enum ExchangeFunction {
         FILL_ORDER,
@@ -42,7 +51,10 @@ contract ReentrantERC20Token is
         MARKET_BUY_ORDERS_NO_THROW,
         MARKET_SELL_ORDERS,
         MARKET_SELL_ORDERS_NO_THROW,
-        MATCH_ORDERS
+        MATCH_ORDERS,
+        CANCEL_ORDER,
+        CANCEL_ORDERS_UP_TO,
+        SET_SIGNATURE_VALIDATOR_APPROVAL
     }
 
     uint8 internal currentFunctionId = 0;
@@ -75,99 +87,127 @@ contract ReentrantERC20Token is
     {
         // This order would normally be invalid, but it will be used strictly for testing reentrnacy.
         // Any reentrancy checks will happen before any other checks that invalidate the order.
-        LibOrder.Order memory order = LibOrder.Order({
-            makerAddress: address(0),
-            takerAddress: address(0),
-            feeRecipientAddress: address(0),
-            senderAddress: address(0),
-            makerAssetAmount: 0,
-            takerAssetAmount: 0,
-            makerFee: 0,
-            takerFee: 0,
-            expirationTimeSeconds: 0,
-            salt: 0,
-            makerAssetData: "",
-            takerAssetData: ""
-        });
+        LibOrder.Order memory order;
 
         // Initialize remaining null parameters
-        bytes memory signature = "";
-        LibOrder.Order[] memory orders = new LibOrder.Order[](1);
-        orders[0] = order;
-        uint256[] memory takerAssetFillAmounts = new uint256[](1);
-        takerAssetFillAmounts[0] = 0;
-        bytes[] memory signatures = new bytes[](1);
-        signatures[0] = signature;
-    
-        // Attempt to reenter Exchange by calling function with currentFunctionId
+        bytes memory signature;
+        LibOrder.Order[] memory orders;
+        uint256[] memory takerAssetFillAmounts;
+        bytes[] memory signatures;
+        bytes memory calldata;
+
+        // Create calldata for function that corresponds to currentFunctionId
         if (currentFunctionId == uint8(ExchangeFunction.FILL_ORDER)) {
-            EXCHANGE.fillOrder(
+            calldata = abi.encodeWithSelector(
+                EXCHANGE.fillOrder.selector,
                 order,
                 0,
                 signature
             );
         } else if (currentFunctionId == uint8(ExchangeFunction.FILL_OR_KILL_ORDER)) {
-            EXCHANGE.fillOrKillOrder(
+            calldata = abi.encodeWithSelector(
+                EXCHANGE.fillOrKillOrder.selector,
                 order,
                 0,
                 signature
             );
         } else if (currentFunctionId == uint8(ExchangeFunction.FILL_ORDER_NO_THROW)) {
-            EXCHANGE.fillOrderNoThrow(
+            calldata = abi.encodeWithSelector(
+                EXCHANGE.fillOrderNoThrow.selector,
                 order,
                 0,
                 signature
             );
         } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_FILL_ORDERS)) {
-            EXCHANGE.batchFillOrders(
+            calldata = abi.encodeWithSelector(
+                EXCHANGE.batchFillOrders.selector,
                 orders,
                 takerAssetFillAmounts,
                 signatures
             );
         } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_FILL_OR_KILL_ORDERS)) {
-            EXCHANGE.batchFillOrKillOrders(
+            calldata = abi.encodeWithSelector(
+                EXCHANGE.batchFillOrKillOrders.selector,
                 orders,
                 takerAssetFillAmounts,
                 signatures
             );
         } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_FILL_ORDERS_NO_THROW)) {
-            EXCHANGE.batchFillOrdersNoThrow(
+            calldata = abi.encodeWithSelector(
+                EXCHANGE.batchFillOrdersNoThrow.selector,
                 orders,
                 takerAssetFillAmounts,
                 signatures
             );
         } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_BUY_ORDERS)) {
-            EXCHANGE.marketBuyOrders(
+            calldata = abi.encodeWithSelector(
+                EXCHANGE.marketBuyOrders.selector,
                 orders,
                 0,
                 signatures
             );
         } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_BUY_ORDERS_NO_THROW)) {
-            EXCHANGE.marketBuyOrdersNoThrow(
+            calldata = abi.encodeWithSelector(
+                EXCHANGE.marketBuyOrdersNoThrow.selector,
                 orders,
                 0,
                 signatures
             );
         } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_SELL_ORDERS)) {
-            EXCHANGE.marketSellOrders(
+            calldata = abi.encodeWithSelector(
+                EXCHANGE.marketSellOrders.selector,
                 orders,
                 0,
                 signatures
             );
         } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_SELL_ORDERS_NO_THROW)) {
-            EXCHANGE.marketSellOrdersNoThrow(
+            calldata = abi.encodeWithSelector(
+                EXCHANGE.marketSellOrdersNoThrow.selector,
                 orders,
                 0,
                 signatures
             );
         } else if (currentFunctionId == uint8(ExchangeFunction.MATCH_ORDERS)) {
-            EXCHANGE.matchOrders(
+            calldata = abi.encodeWithSelector(
+                EXCHANGE.matchOrders.selector,
                 order,
                 order,
                 signature,
                 signature
             );
+        } else if (currentFunctionId == uint8(ExchangeFunction.CANCEL_ORDER)) {
+            calldata = abi.encodeWithSelector(
+                EXCHANGE.cancelOrder.selector,
+                order
+            );
+        } else if (currentFunctionId == uint8(ExchangeFunction.CANCEL_ORDERS_UP_TO)) {
+            calldata = abi.encodeWithSelector(
+                EXCHANGE.cancelOrdersUpTo.selector,
+                0
+            );
+        } else if (currentFunctionId == uint8(ExchangeFunction.SET_SIGNATURE_VALIDATOR_APPROVAL)) {
+            calldata = abi.encodeWithSelector(
+                EXCHANGE.setSignatureValidatorApproval.selector,
+                address(0),
+                false
+            );
         }
+
+        // Call Exchange function, swallow error
+        address(EXCHANGE).call(calldata);
+
+        // Revert reason is 100 bytes
+        bytes memory returnData = new bytes(100);
+
+        // Copy return data
+        assembly {
+            returndatacopy(add(returnData, 32), 0, 100)
+        }
+
+        // Revert if function reverted with REENTRANCY_ILLEGAL error
+        require(!REENTRANCY_ILLEGAL_REVERT_REASON.equals(returnData));
+
+        // Transfer will return true if function failed for any other reason
         return true;
     }
 }
\ No newline at end of file
-- 
cgit v1.2.3


From 7d5a23969d5514ca3c942f11a48a8e858b9d0188 Mon Sep 17 00:00:00 2001
From: Amir Bandeali <abandeali1@gmail.com>
Date: Wed, 22 Aug 2018 18:01:45 -0700
Subject: Add reentrancy tests for fillOrder and wrapper functions

---
 packages/contracts/test/exchange/core.ts    |  28 ++++
 packages/contracts/test/exchange/wrapper.ts | 193 ++++++++++++++++++++++++++++
 packages/contracts/test/utils/artifacts.ts  |   2 +
 packages/contracts/test/utils/constants.ts  |  16 +++
 4 files changed, 239 insertions(+)

(limited to 'packages/contracts')

diff --git a/packages/contracts/test/exchange/core.ts b/packages/contracts/test/exchange/core.ts
index f9d8b7783..8ba55a0ab 100644
--- a/packages/contracts/test/exchange/core.ts
+++ b/packages/contracts/test/exchange/core.ts
@@ -14,6 +14,7 @@ import { DummyNoReturnERC20TokenContract } from '../../generated_contract_wrappe
 import { ERC20ProxyContract } from '../../generated_contract_wrappers/erc20_proxy';
 import { ERC721ProxyContract } from '../../generated_contract_wrappers/erc721_proxy';
 import { ExchangeCancelEventArgs, ExchangeContract } from '../../generated_contract_wrappers/exchange';
+import { ReentrantERC20TokenContract } from '../../generated_contract_wrappers/reentrant_erc20_token';
 import { TestStaticCallReceiverContract } from '../../generated_contract_wrappers/test_static_call_receiver';
 import { artifacts } from '../utils/artifacts';
 import { expectTransactionFailedAsync } from '../utils/assertions';
@@ -42,6 +43,7 @@ describe('Exchange core', () => {
     let zrxToken: DummyERC20TokenContract;
     let erc721Token: DummyERC721TokenContract;
     let noReturnErc20Token: DummyNoReturnERC20TokenContract;
+    let reentrantErc20Token: ReentrantERC20TokenContract;
     let exchange: ExchangeContract;
     let erc20Proxy: ERC20ProxyContract;
     let erc721Proxy: ERC721ProxyContract;
@@ -117,6 +119,12 @@ describe('Exchange core', () => {
             provider,
             txDefaults,
         );
+        reentrantErc20Token = await ReentrantERC20TokenContract.deployFrom0xArtifactAsync(
+            artifacts.ReentrantERC20Token,
+            provider,
+            txDefaults,
+            exchange.address,
+        );
 
         defaultMakerAssetAddress = erc20TokenA.address;
         defaultTakerAssetAddress = erc20TokenB.address;
@@ -144,6 +152,26 @@ describe('Exchange core', () => {
             signedOrder = await orderFactory.newSignedOrderAsync();
         });
 
+        const reentrancyTest = (functionNames: string[]) => {
+            _.forEach(functionNames, async (functionName: string, functionId: number) => {
+                const description = `should not allow fillOrder to reenter the Exchange contract via ${functionName}`;
+                it(description, async () => {
+                    signedOrder = await orderFactory.newSignedOrderAsync({
+                        makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address),
+                    });
+                    await web3Wrapper.awaitTransactionSuccessAsync(
+                        await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId),
+                        constants.AWAIT_TRANSACTION_MINED_MS,
+                    );
+                    await expectTransactionFailedAsync(
+                        exchangeWrapper.fillOrderAsync(signedOrder, takerAddress),
+                        RevertReason.TransferFailed,
+                    );
+                });
+            });
+        };
+        describe('fillOrder reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX));
+
         it('should throw if signature is invalid', async () => {
             signedOrder = await orderFactory.newSignedOrderAsync({
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
diff --git a/packages/contracts/test/exchange/wrapper.ts b/packages/contracts/test/exchange/wrapper.ts
index d48441dca..aadb5ab59 100644
--- a/packages/contracts/test/exchange/wrapper.ts
+++ b/packages/contracts/test/exchange/wrapper.ts
@@ -11,6 +11,7 @@ import { DummyERC721TokenContract } from '../../generated_contract_wrappers/dumm
 import { ERC20ProxyContract } from '../../generated_contract_wrappers/erc20_proxy';
 import { ERC721ProxyContract } from '../../generated_contract_wrappers/erc721_proxy';
 import { ExchangeContract } from '../../generated_contract_wrappers/exchange';
+import { ReentrantERC20TokenContract } from '../../generated_contract_wrappers/reentrant_erc20_token';
 import { artifacts } from '../utils/artifacts';
 import { expectTransactionFailedAsync } from '../utils/assertions';
 import { getLatestBlockTimestampAsync, increaseTimeAndMineBlockAsync } from '../utils/block_timestamp';
@@ -40,6 +41,7 @@ describe('Exchange wrappers', () => {
     let exchange: ExchangeContract;
     let erc20Proxy: ERC20ProxyContract;
     let erc721Proxy: ERC721ProxyContract;
+    let reentrantErc20Token: ReentrantERC20TokenContract;
 
     let exchangeWrapper: ExchangeWrapper;
     let erc20Wrapper: ERC20Wrapper;
@@ -104,6 +106,13 @@ describe('Exchange wrappers', () => {
             constants.AWAIT_TRANSACTION_MINED_MS,
         );
 
+        reentrantErc20Token = await ReentrantERC20TokenContract.deployFrom0xArtifactAsync(
+            artifacts.ReentrantERC20Token,
+            provider,
+            txDefaults,
+            exchange.address,
+        );
+
         defaultMakerAssetAddress = erc20TokenA.address;
         defaultTakerAssetAddress = erc20TokenB.address;
 
@@ -126,6 +135,26 @@ describe('Exchange wrappers', () => {
         await blockchainLifecycle.revertAsync();
     });
     describe('fillOrKillOrder', () => {
+        const reentrancyTest = (functionNames: string[]) => {
+            _.forEach(functionNames, async (functionName: string, functionId: number) => {
+                const description = `should not allow fillOrKillOrder to reenter the Exchange contract via ${functionName}`;
+                it(description, async () => {
+                    const signedOrder = await orderFactory.newSignedOrderAsync({
+                        makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address),
+                    });
+                    await web3Wrapper.awaitTransactionSuccessAsync(
+                        await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId),
+                        constants.AWAIT_TRANSACTION_MINED_MS,
+                    );
+                    await expectTransactionFailedAsync(
+                        exchangeWrapper.fillOrKillOrderAsync(signedOrder, takerAddress),
+                        RevertReason.TransferFailed,
+                    );
+                });
+            });
+        };
+        describe('fillOrKillOrder reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX));
+
         it('should transfer the correct amounts', async () => {
             const signedOrder = await orderFactory.newSignedOrderAsync({
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18),
@@ -197,6 +226,25 @@ describe('Exchange wrappers', () => {
     });
 
     describe('fillOrderNoThrow', () => {
+        const reentrancyTest = (functionNames: string[]) => {
+            _.forEach(functionNames, async (functionName: string, functionId: number) => {
+                const description = `should not allow fillOrderNoThrow to reenter the Exchange contract via ${functionName}`;
+                it(description, async () => {
+                    const signedOrder = await orderFactory.newSignedOrderAsync({
+                        makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address),
+                    });
+                    await web3Wrapper.awaitTransactionSuccessAsync(
+                        await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId),
+                        constants.AWAIT_TRANSACTION_MINED_MS,
+                    );
+                    await exchangeWrapper.fillOrderNoThrowAsync(signedOrder, takerAddress);
+                    const newBalances = await erc20Wrapper.getBalancesAsync();
+                    expect(erc20Balances).to.deep.equal(newBalances);
+                });
+            });
+        };
+        describe('fillOrderNoThrow reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX));
+
         it('should transfer the correct amounts', async () => {
             const signedOrder = await orderFactory.newSignedOrderAsync({
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18),
@@ -397,6 +445,26 @@ describe('Exchange wrappers', () => {
         });
 
         describe('batchFillOrders', () => {
+            const reentrancyTest = (functionNames: string[]) => {
+                _.forEach(functionNames, async (functionName: string, functionId: number) => {
+                    const description = `should not allow batchFillOrders to reenter the Exchange contract via ${functionName}`;
+                    it(description, async () => {
+                        const signedOrder = await orderFactory.newSignedOrderAsync({
+                            makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address),
+                        });
+                        await web3Wrapper.awaitTransactionSuccessAsync(
+                            await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId),
+                            constants.AWAIT_TRANSACTION_MINED_MS,
+                        );
+                        await expectTransactionFailedAsync(
+                            exchangeWrapper.batchFillOrdersAsync([signedOrder], takerAddress),
+                            RevertReason.TransferFailed,
+                        );
+                    });
+                });
+            };
+            describe('batchFillOrders reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX));
+
             it('should transfer the correct amounts', async () => {
                 const takerAssetFillAmounts: BigNumber[] = [];
                 const makerAssetAddress = erc20TokenA.address;
@@ -446,6 +514,26 @@ describe('Exchange wrappers', () => {
         });
 
         describe('batchFillOrKillOrders', () => {
+            const reentrancyTest = (functionNames: string[]) => {
+                _.forEach(functionNames, async (functionName: string, functionId: number) => {
+                    const description = `should not allow batchFillOrKillOrders to reenter the Exchange contract via ${functionName}`;
+                    it(description, async () => {
+                        const signedOrder = await orderFactory.newSignedOrderAsync({
+                            makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address),
+                        });
+                        await web3Wrapper.awaitTransactionSuccessAsync(
+                            await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId),
+                            constants.AWAIT_TRANSACTION_MINED_MS,
+                        );
+                        await expectTransactionFailedAsync(
+                            exchangeWrapper.batchFillOrKillOrdersAsync([signedOrder], takerAddress),
+                            RevertReason.TransferFailed,
+                        );
+                    });
+                });
+            };
+            describe('batchFillOrKillOrders reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX));
+
             it('should transfer the correct amounts', async () => {
                 const takerAssetFillAmounts: BigNumber[] = [];
                 const makerAssetAddress = erc20TokenA.address;
@@ -512,6 +600,25 @@ describe('Exchange wrappers', () => {
         });
 
         describe('batchFillOrdersNoThrow', async () => {
+            const reentrancyTest = (functionNames: string[]) => {
+                _.forEach(functionNames, async (functionName: string, functionId: number) => {
+                    const description = `should not allow batchFillOrdersNoThrow to reenter the Exchange contract via ${functionName}`;
+                    it(description, async () => {
+                        const signedOrder = await orderFactory.newSignedOrderAsync({
+                            makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address),
+                        });
+                        await web3Wrapper.awaitTransactionSuccessAsync(
+                            await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId),
+                            constants.AWAIT_TRANSACTION_MINED_MS,
+                        );
+                        await exchangeWrapper.batchFillOrdersNoThrowAsync([signedOrder], takerAddress);
+                        const newBalances = await erc20Wrapper.getBalancesAsync();
+                        expect(erc20Balances).to.deep.equal(newBalances);
+                    });
+                });
+            };
+            describe('batchFillOrdersNoThrow reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX));
+
             it('should transfer the correct amounts', async () => {
                 const takerAssetFillAmounts: BigNumber[] = [];
                 const makerAssetAddress = erc20TokenA.address;
@@ -625,6 +732,28 @@ describe('Exchange wrappers', () => {
         });
 
         describe('marketSellOrders', () => {
+            const reentrancyTest = (functionNames: string[]) => {
+                _.forEach(functionNames, async (functionName: string, functionId: number) => {
+                    const description = `should not allow marketSellOrders to reenter the Exchange contract via ${functionName}`;
+                    it(description, async () => {
+                        const signedOrder = await orderFactory.newSignedOrderAsync({
+                            makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address),
+                        });
+                        await web3Wrapper.awaitTransactionSuccessAsync(
+                            await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId),
+                            constants.AWAIT_TRANSACTION_MINED_MS,
+                        );
+                        await expectTransactionFailedAsync(
+                            exchangeWrapper.marketSellOrdersAsync([signedOrder], takerAddress, {
+                                takerAssetFillAmount: signedOrder.takerAssetAmount,
+                            }),
+                            RevertReason.TransferFailed,
+                        );
+                    });
+                });
+            };
+            describe('marketSellOrders reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX));
+
             it('should stop when the entire takerAssetFillAmount is filled', async () => {
                 const takerAssetFillAmount = signedOrders[0].takerAssetAmount.plus(
                     signedOrders[1].takerAssetAmount.div(2),
@@ -717,6 +846,27 @@ describe('Exchange wrappers', () => {
         });
 
         describe('marketSellOrdersNoThrow', () => {
+            const reentrancyTest = (functionNames: string[]) => {
+                _.forEach(functionNames, async (functionName: string, functionId: number) => {
+                    const description = `should not allow marketSellOrdersNoThrow to reenter the Exchange contract via ${functionName}`;
+                    it(description, async () => {
+                        const signedOrder = await orderFactory.newSignedOrderAsync({
+                            makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address),
+                        });
+                        await web3Wrapper.awaitTransactionSuccessAsync(
+                            await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId),
+                            constants.AWAIT_TRANSACTION_MINED_MS,
+                        );
+                        await exchangeWrapper.marketSellOrdersNoThrowAsync([signedOrder], takerAddress, {
+                            takerAssetFillAmount: signedOrder.takerAssetAmount,
+                        });
+                        const newBalances = await erc20Wrapper.getBalancesAsync();
+                        expect(erc20Balances).to.deep.equal(newBalances);
+                    });
+                });
+            };
+            describe('marketSellOrdersNoThrow reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX));
+
             it('should stop when the entire takerAssetFillAmount is filled', async () => {
                 const takerAssetFillAmount = signedOrders[0].takerAssetAmount.plus(
                     signedOrders[1].takerAssetAmount.div(2),
@@ -843,6 +993,28 @@ describe('Exchange wrappers', () => {
         });
 
         describe('marketBuyOrders', () => {
+            const reentrancyTest = (functionNames: string[]) => {
+                _.forEach(functionNames, async (functionName: string, functionId: number) => {
+                    const description = `should not allow marketBuyOrders to reenter the Exchange contract via ${functionName}`;
+                    it(description, async () => {
+                        const signedOrder = await orderFactory.newSignedOrderAsync({
+                            makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address),
+                        });
+                        await web3Wrapper.awaitTransactionSuccessAsync(
+                            await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId),
+                            constants.AWAIT_TRANSACTION_MINED_MS,
+                        );
+                        await expectTransactionFailedAsync(
+                            exchangeWrapper.marketBuyOrdersAsync([signedOrder], takerAddress, {
+                                makerAssetFillAmount: signedOrder.makerAssetAmount,
+                            }),
+                            RevertReason.TransferFailed,
+                        );
+                    });
+                });
+            };
+            describe('marketBuyOrders reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX));
+
             it('should stop when the entire makerAssetFillAmount is filled', async () => {
                 const makerAssetFillAmount = signedOrders[0].makerAssetAmount.plus(
                     signedOrders[1].makerAssetAmount.div(2),
@@ -933,6 +1105,27 @@ describe('Exchange wrappers', () => {
         });
 
         describe('marketBuyOrdersNoThrow', () => {
+            const reentrancyTest = (functionNames: string[]) => {
+                _.forEach(functionNames, async (functionName: string, functionId: number) => {
+                    const description = `should not allow marketBuyOrdersNoThrow to reenter the Exchange contract via ${functionName}`;
+                    it(description, async () => {
+                        const signedOrder = await orderFactory.newSignedOrderAsync({
+                            makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address),
+                        });
+                        await web3Wrapper.awaitTransactionSuccessAsync(
+                            await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId),
+                            constants.AWAIT_TRANSACTION_MINED_MS,
+                        );
+                        await exchangeWrapper.marketBuyOrdersNoThrowAsync([signedOrder], takerAddress, {
+                            makerAssetFillAmount: signedOrder.makerAssetAmount,
+                        });
+                        const newBalances = await erc20Wrapper.getBalancesAsync();
+                        expect(erc20Balances).to.deep.equal(newBalances);
+                    });
+                });
+            };
+            describe('marketBuyOrdersNoThrow reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX));
+
             it('should stop when the entire makerAssetFillAmount is filled', async () => {
                 const makerAssetFillAmount = signedOrders[0].makerAssetAmount.plus(
                     signedOrders[1].makerAssetAmount.div(2),
diff --git a/packages/contracts/test/utils/artifacts.ts b/packages/contracts/test/utils/artifacts.ts
index 2f6fcef71..5ddb5cc7f 100644
--- a/packages/contracts/test/utils/artifacts.ts
+++ b/packages/contracts/test/utils/artifacts.ts
@@ -16,6 +16,7 @@ import * as MixinAuthorizable from '../../artifacts/MixinAuthorizable.json';
 import * as MultiSigWallet from '../../artifacts/MultiSigWallet.json';
 import * as MultiSigWalletWithTimeLock from '../../artifacts/MultiSigWalletWithTimeLock.json';
 import * as OrderValidator from '../../artifacts/OrderValidator.json';
+import * as ReentrantERC20Token from '../../artifacts/ReentrantERC20Token.json';
 import * as TestAssetProxyDispatcher from '../../artifacts/TestAssetProxyDispatcher.json';
 import * as TestAssetProxyOwner from '../../artifacts/TestAssetProxyOwner.json';
 import * as TestConstants from '../../artifacts/TestConstants.json';
@@ -49,6 +50,7 @@ export const artifacts = {
     MultiSigWallet: (MultiSigWallet as any) as ContractArtifact,
     MultiSigWalletWithTimeLock: (MultiSigWalletWithTimeLock as any) as ContractArtifact,
     OrderValidator: (OrderValidator as any) as ContractArtifact,
+    ReentrantERC20Token: (ReentrantERC20Token as any) as ContractArtifact,
     TestAssetProxyOwner: (TestAssetProxyOwner as any) as ContractArtifact,
     TestAssetProxyDispatcher: (TestAssetProxyDispatcher as any) as ContractArtifact,
     TestConstants: (TestConstants as any) as ContractArtifact,
diff --git a/packages/contracts/test/utils/constants.ts b/packages/contracts/test/utils/constants.ts
index 65eaee398..7060b5091 100644
--- a/packages/contracts/test/utils/constants.ts
+++ b/packages/contracts/test/utils/constants.ts
@@ -51,4 +51,20 @@ export const constants = {
     WORD_LENGTH: 32,
     ZERO_AMOUNT: new BigNumber(0),
     PERCENTAGE_DENOMINATOR: new BigNumber(10).pow(18),
+    FUNCTIONS_WITH_MUTEX: [
+        'FILL_ORDER',
+        'FILL_OR_KILL_ORDER',
+        'FILL_ORDER_NO_THROW',
+        'BATCH_FILL_ORDERS',
+        'BATCH_FILL_OR_KILL_ORDERS',
+        'BATCH_FILL_ORDERS_NO_THROW',
+        'MARKET_BUY_ORDERS',
+        'MARKET_BUY_ORDERS_NO_THROW',
+        'MARKET_SELL_ORDERS',
+        'MARKET_SELL_ORDERS_NO_THROW',
+        'MATCH_ORDERS',
+        'CANCEL_ORDER',
+        'CANCEL_ORDERS_UP_TO',
+        'SET_SIGNATURE_VALIDATOR_APPROVAL',
+    ],
 };
-- 
cgit v1.2.3


From a09ee907396d35468e0ad632fb990cb1c8870b49 Mon Sep 17 00:00:00 2001
From: Amir Bandeali <abandeali1@gmail.com>
Date: Wed, 22 Aug 2018 23:27:43 -0700
Subject: Add tests for matchOrders

---
 packages/contracts/test/exchange/core.ts         |   2 +-
 packages/contracts/test/exchange/match_orders.ts | 173 +++++++----------------
 2 files changed, 53 insertions(+), 122 deletions(-)

(limited to 'packages/contracts')

diff --git a/packages/contracts/test/exchange/core.ts b/packages/contracts/test/exchange/core.ts
index 8ba55a0ab..3bb71b58f 100644
--- a/packages/contracts/test/exchange/core.ts
+++ b/packages/contracts/test/exchange/core.ts
@@ -530,7 +530,7 @@ describe('Exchange core', () => {
                 // HACK(albrow): We need to hardcode the gas estimate here because
                 // the Geth gas estimator doesn't work with the way we use
                 // delegatecall and swallow errors.
-                gas: 490000,
+                gas: 600000,
             });
 
             const newBalances = await erc20Wrapper.getBalancesAsync();
diff --git a/packages/contracts/test/exchange/match_orders.ts b/packages/contracts/test/exchange/match_orders.ts
index 46b3569bd..8cd873a85 100644
--- a/packages/contracts/test/exchange/match_orders.ts
+++ b/packages/contracts/test/exchange/match_orders.ts
@@ -11,6 +11,7 @@ import { DummyERC721TokenContract } from '../../generated_contract_wrappers/dumm
 import { ERC20ProxyContract } from '../../generated_contract_wrappers/erc20_proxy';
 import { ERC721ProxyContract } from '../../generated_contract_wrappers/erc721_proxy';
 import { ExchangeContract } from '../../generated_contract_wrappers/exchange';
+import { ReentrantERC20TokenContract } from '../../generated_contract_wrappers/reentrant_erc20_token';
 import { artifacts } from '../utils/artifacts';
 import { expectTransactionFailedAsync } from '../utils/assertions';
 import { chaiSetup } from '../utils/chai_setup';
@@ -39,6 +40,7 @@ describe('matchOrders', () => {
     let erc20TokenB: DummyERC20TokenContract;
     let zrxToken: DummyERC20TokenContract;
     let erc721Token: DummyERC721TokenContract;
+    let reentrantErc20Token: ReentrantERC20TokenContract;
     let exchange: ExchangeContract;
     let erc20Proxy: ERC20ProxyContract;
     let erc721Proxy: ERC721ProxyContract;
@@ -127,21 +129,39 @@ describe('matchOrders', () => {
             }),
             constants.AWAIT_TRANSACTION_MINED_MS,
         );
+
+        reentrantErc20Token = await ReentrantERC20TokenContract.deployFrom0xArtifactAsync(
+            artifacts.ReentrantERC20Token,
+            provider,
+            txDefaults,
+            exchange.address,
+        );
+
         // Set default addresses
         defaultERC20MakerAssetAddress = erc20TokenA.address;
         defaultERC20TakerAssetAddress = erc20TokenB.address;
         defaultERC721AssetAddress = erc721Token.address;
         // Create default order parameters
-        const defaultOrderParams = {
+        const defaultOrderParamsLeft = {
             ...constants.STATIC_ORDER_PARAMS,
+            makerAddress: makerAddressLeft,
             exchangeAddress: exchange.address,
             makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
             takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
+            feeRecipientAddress: feeRecipientAddressLeft,
+        };
+        const defaultOrderParamsRight = {
+            ...constants.STATIC_ORDER_PARAMS,
+            makerAddress: makerAddressRight,
+            exchangeAddress: exchange.address,
+            makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
+            takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
+            feeRecipientAddress: feeRecipientAddressRight,
         };
         const privateKeyLeft = constants.TESTRPC_PRIVATE_KEYS[accounts.indexOf(makerAddressLeft)];
-        orderFactoryLeft = new OrderFactory(privateKeyLeft, defaultOrderParams);
+        orderFactoryLeft = new OrderFactory(privateKeyLeft, defaultOrderParamsLeft);
         const privateKeyRight = constants.TESTRPC_PRIVATE_KEYS[accounts.indexOf(makerAddressRight)];
-        orderFactoryRight = new OrderFactory(privateKeyRight, defaultOrderParams);
+        orderFactoryRight = new OrderFactory(privateKeyRight, defaultOrderParamsRight);
         // Set match order tester
         matchOrderTester = new MatchOrderTester(exchangeWrapper, erc20Wrapper, erc721Wrapper, zrxToken.address);
     });
@@ -157,21 +177,44 @@ describe('matchOrders', () => {
             erc721TokenIdsByOwner = await erc721Wrapper.getBalancesAsync();
         });
 
+        const reentrancyTest = (functionNames: string[]) => {
+            _.forEach(functionNames, async (functionName: string, functionId: number) => {
+                const description = `should not allow matchOrders to reenter the Exchange contract via ${functionName}`;
+                it(description, async () => {
+                    const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
+                        makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address),
+                        makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18),
+                        takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
+                    });
+                    const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
+                        makerAddress: makerAddressRight,
+                        takerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address),
+                        makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
+                        takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18),
+                        feeRecipientAddress: feeRecipientAddressRight,
+                    });
+                    await web3Wrapper.awaitTransactionSuccessAsync(
+                        await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId),
+                        constants.AWAIT_TRANSACTION_MINED_MS,
+                    );
+                    await expectTransactionFailedAsync(
+                        exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress),
+                        RevertReason.TransferFailed,
+                    );
+                });
+            });
+        };
+        describe('matchOrders reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX));
+
         it('should transfer the correct amounts when orders completely fill each other', async () => {
             // Create orders to match
             const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
-                makerAddress: makerAddressLeft,
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
-                feeRecipientAddress: feeRecipientAddressLeft,
             });
             const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
-                makerAddress: makerAddressRight,
-                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
-                takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18),
-                feeRecipientAddress: feeRecipientAddressRight,
             });
             // Match signedOrderLeft with signedOrderRight
             await matchOrderTester.matchOrdersAndVerifyBalancesAsync(
@@ -192,18 +235,12 @@ describe('matchOrders', () => {
         it('should transfer the correct amounts when orders completely fill each other and taker doesnt take a profit', async () => {
             // Create orders to match
             const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
-                makerAddress: makerAddressLeft,
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
-                feeRecipientAddress: feeRecipientAddressLeft,
             });
             const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
-                makerAddress: makerAddressRight,
-                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
-                takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18),
-                feeRecipientAddress: feeRecipientAddressRight,
             });
             // Store original taker balance
             const takerInitialBalances = _.cloneDeep(erc20BalancesByOwner[takerAddress][defaultERC20MakerAssetAddress]);
@@ -237,18 +274,12 @@ describe('matchOrders', () => {
         it('should transfer the correct amounts when left order is completely filled and right order is partially filled', async () => {
             // Create orders to match
             const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
-                makerAddress: makerAddressLeft,
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
-                feeRecipientAddress: feeRecipientAddressLeft,
             });
             const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
-                makerAddress: makerAddressRight,
-                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
-                takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(20), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(4), 18),
-                feeRecipientAddress: feeRecipientAddressRight,
             });
             // Match orders
             await matchOrderTester.matchOrdersAndVerifyBalancesAsync(
@@ -269,18 +300,12 @@ describe('matchOrders', () => {
         it('should transfer the correct amounts when right order is completely filled and left order is partially filled', async () => {
             // Create orders to match
             const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
-                makerAddress: makerAddressLeft,
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(50), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18),
-                feeRecipientAddress: feeRecipientAddressLeft,
             });
             const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
-                makerAddress: makerAddressRight,
-                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
-                takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18),
-                feeRecipientAddress: feeRecipientAddressRight,
             });
             // Match orders
             await matchOrderTester.matchOrdersAndVerifyBalancesAsync(
@@ -301,18 +326,12 @@ describe('matchOrders', () => {
         it('should transfer the correct amounts when consecutive calls are used to completely fill the left order', async () => {
             // Create orders to match
             const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
-                makerAddress: makerAddressLeft,
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(50), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18),
-                feeRecipientAddress: feeRecipientAddressLeft,
             });
             const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
-                makerAddress: makerAddressRight,
-                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
-                takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18),
-                feeRecipientAddress: feeRecipientAddressRight,
             });
             // Match orders
             let newERC20BalancesByOwner: ERC20BalancesByOwner;
@@ -340,12 +359,8 @@ describe('matchOrders', () => {
             //       However, we use 100/50 to ensure a partial fill as we want to go down the "left fill"
             //       branch in the contract twice for this test.
             const signedOrderRight2 = await orderFactoryRight.newSignedOrderAsync({
-                makerAddress: makerAddressRight,
-                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
-                takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(50), 18),
-                feeRecipientAddress: feeRecipientAddressRight,
             });
             // Match signedOrderLeft with signedOrderRight2
             const leftTakerAssetFilledAmount = signedOrderRight.makerAssetAmount;
@@ -370,19 +385,13 @@ describe('matchOrders', () => {
         it('should transfer the correct amounts when consecutive calls are used to completely fill the right order', async () => {
             // Create orders to match
             const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
-                makerAddress: makerAddressLeft,
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18),
-                feeRecipientAddress: feeRecipientAddressLeft,
             });
 
             const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
-                makerAddress: makerAddressRight,
-                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
-                takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(50), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18),
-                feeRecipientAddress: feeRecipientAddressRight,
             });
             // Match orders
             let newERC20BalancesByOwner: ERC20BalancesByOwner;
@@ -410,10 +419,8 @@ describe('matchOrders', () => {
             //       However, we use 100/50 to ensure a partial fill as we want to go down the "right fill"
             //       branch in the contract twice for this test.
             const signedOrderLeft2 = await orderFactoryLeft.newSignedOrderAsync({
-                makerAddress: makerAddressLeft,
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(50), 18),
-                feeRecipientAddress: feeRecipientAddressLeft,
             });
             // Match signedOrderLeft2 with signedOrderRight
             const leftTakerAssetFilledAmount = new BigNumber(0);
@@ -441,15 +448,11 @@ describe('matchOrders', () => {
         it('should transfer the correct amounts if fee recipient is the same across both matched orders', async () => {
             const feeRecipientAddress = feeRecipientAddressLeft;
             const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
-                makerAddress: makerAddressLeft,
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
                 feeRecipientAddress,
             });
             const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
-                makerAddress: makerAddressRight,
-                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
-                takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18),
                 feeRecipientAddress,
@@ -467,18 +470,12 @@ describe('matchOrders', () => {
         it('should transfer the correct amounts if taker is also the left order maker', async () => {
             // Create orders to match
             const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
-                makerAddress: makerAddressLeft,
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
-                feeRecipientAddress: feeRecipientAddressLeft,
             });
             const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
-                makerAddress: makerAddressRight,
-                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
-                takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18),
-                feeRecipientAddress: feeRecipientAddressRight,
             });
             // Match orders
             takerAddress = signedOrderLeft.makerAddress;
@@ -494,18 +491,12 @@ describe('matchOrders', () => {
         it('should transfer the correct amounts if taker is also the right order maker', async () => {
             // Create orders to match
             const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
-                makerAddress: makerAddressLeft,
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
-                feeRecipientAddress: feeRecipientAddressLeft,
             });
             const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
-                makerAddress: makerAddressRight,
-                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
-                takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18),
-                feeRecipientAddress: feeRecipientAddressRight,
             });
             // Match orders
             takerAddress = signedOrderRight.makerAddress;
@@ -521,18 +512,12 @@ describe('matchOrders', () => {
         it('should transfer the correct amounts if taker is also the left fee recipient', async () => {
             // Create orders to match
             const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
-                makerAddress: makerAddressLeft,
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
-                feeRecipientAddress: feeRecipientAddressLeft,
             });
             const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
-                makerAddress: makerAddressRight,
-                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
-                takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18),
-                feeRecipientAddress: feeRecipientAddressRight,
             });
             // Match orders
             takerAddress = feeRecipientAddressLeft;
@@ -548,18 +533,12 @@ describe('matchOrders', () => {
         it('should transfer the correct amounts if taker is also the right fee recipient', async () => {
             // Create orders to match
             const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
-                makerAddress: makerAddressLeft,
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
-                feeRecipientAddress: feeRecipientAddressLeft,
             });
             const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
-                makerAddress: makerAddressRight,
-                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
-                takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18),
-                feeRecipientAddress: feeRecipientAddressRight,
             });
             // Match orders
             takerAddress = feeRecipientAddressRight;
@@ -575,18 +554,12 @@ describe('matchOrders', () => {
         it('should transfer the correct amounts if left maker is the left fee recipient and right maker is the right fee recipient', async () => {
             // Create orders to match
             const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
-                makerAddress: makerAddressLeft,
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
-                feeRecipientAddress: makerAddressLeft,
             });
             const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
-                makerAddress: makerAddressRight,
-                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
-                takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18),
-                feeRecipientAddress: makerAddressRight,
             });
             // Match orders
             await matchOrderTester.matchOrdersAndVerifyBalancesAsync(
@@ -601,18 +574,12 @@ describe('matchOrders', () => {
         it('Should throw if left order is not fillable', async () => {
             // Create orders to match
             const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
-                makerAddress: makerAddressLeft,
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
-                feeRecipientAddress: feeRecipientAddressLeft,
             });
             const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
-                makerAddress: makerAddressRight,
-                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
-                takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18),
-                feeRecipientAddress: feeRecipientAddressRight,
             });
             // Cancel left order
             await exchangeWrapper.cancelOrderAsync(signedOrderLeft, signedOrderLeft.makerAddress);
@@ -626,18 +593,12 @@ describe('matchOrders', () => {
         it('Should throw if right order is not fillable', async () => {
             // Create orders to match
             const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
-                makerAddress: makerAddressLeft,
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
-                feeRecipientAddress: feeRecipientAddressLeft,
             });
             const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
-                makerAddress: makerAddressRight,
-                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
-                takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18),
-                feeRecipientAddress: feeRecipientAddressRight,
             });
             // Cancel right order
             await exchangeWrapper.cancelOrderAsync(signedOrderRight, signedOrderRight.makerAddress);
@@ -651,18 +612,12 @@ describe('matchOrders', () => {
         it('should throw if there is not a positive spread', async () => {
             // Create orders to match
             const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
-                makerAddress: makerAddressLeft,
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18),
-                feeRecipientAddress: feeRecipientAddressLeft,
             });
             const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
-                makerAddress: makerAddressRight,
-                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
-                takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(1), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(200), 18),
-                feeRecipientAddress: feeRecipientAddressRight,
             });
             // Match orders
             return expectTransactionFailedAsync(
@@ -674,18 +629,13 @@ describe('matchOrders', () => {
         it('should throw if the left maker asset is not equal to the right taker asset ', async () => {
             // Create orders to match
             const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
-                makerAddress: makerAddressLeft,
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
-                feeRecipientAddress: feeRecipientAddressLeft,
             });
             const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
-                makerAddress: makerAddressRight,
-                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
                 takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18),
-                feeRecipientAddress: feeRecipientAddressRight,
             });
             // Match orders
             return expectTransactionFailedAsync(
@@ -701,20 +651,13 @@ describe('matchOrders', () => {
         it('should throw if the right maker asset is not equal to the left taker asset', async () => {
             // Create orders to match
             const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
-                makerAddress: makerAddressLeft,
-                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
                 takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
-                feeRecipientAddress: feeRecipientAddressLeft,
             });
             const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
-                makerAddress: makerAddressRight,
-                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
-                takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18),
-                feeRecipientAddress: feeRecipientAddressRight,
             });
             // Match orders
             return expectTransactionFailedAsync(
@@ -727,20 +670,14 @@ describe('matchOrders', () => {
             // Create orders to match
             const erc721TokenToTransfer = erc721LeftMakerAssetIds[0];
             const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
-                makerAddress: makerAddressLeft,
                 makerAssetData: assetDataUtils.encodeERC721AssetData(defaultERC721AssetAddress, erc721TokenToTransfer),
-                takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
                 makerAssetAmount: new BigNumber(1),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
-                feeRecipientAddress: feeRecipientAddressLeft,
             });
             const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
-                makerAddress: makerAddressRight,
-                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress),
                 takerAssetData: assetDataUtils.encodeERC721AssetData(defaultERC721AssetAddress, erc721TokenToTransfer),
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
                 takerAssetAmount: new BigNumber(1),
-                feeRecipientAddress: feeRecipientAddressRight,
             });
             // Match orders
             await matchOrderTester.matchOrdersAndVerifyBalancesAsync(
@@ -762,20 +699,14 @@ describe('matchOrders', () => {
             // Create orders to match
             const erc721TokenToTransfer = erc721RightMakerAssetIds[0];
             const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
-                makerAddress: makerAddressLeft,
-                makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
                 takerAssetData: assetDataUtils.encodeERC721AssetData(defaultERC721AssetAddress, erc721TokenToTransfer),
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
                 takerAssetAmount: new BigNumber(1),
-                feeRecipientAddress: feeRecipientAddressLeft,
             });
             const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({
-                makerAddress: makerAddressRight,
                 makerAssetData: assetDataUtils.encodeERC721AssetData(defaultERC721AssetAddress, erc721TokenToTransfer),
-                takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress),
                 makerAssetAmount: new BigNumber(1),
                 takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
-                feeRecipientAddress: feeRecipientAddressRight,
             });
             // Match orders
             await matchOrderTester.matchOrdersAndVerifyBalancesAsync(
-- 
cgit v1.2.3


From c28c3db63fb80d5d5e82ccd3b327cb8c408cc6fa Mon Sep 17 00:00:00 2001
From: Amir Bandeali <abandeali1@gmail.com>
Date: Thu, 23 Aug 2018 16:43:46 -0700
Subject: Only use one nonReentrant modifier, remove modifier from
 fillOrderNoThrow variations

---
 .../2.0.0/protocol/Exchange/MixinExchangeCore.sol  |  2 +-
 .../2.0.0/protocol/Exchange/MixinMatchOrders.sol   |  2 +-
 .../protocol/Exchange/MixinWrapperFunctions.sol    | 17 +++++------
 .../ReentrantERC20Token/ReentrantERC20Token.sol    | 33 +---------------------
 .../utils/ReentrancyGuard/ReentrancyGuard.sol      | 14 +--------
 packages/contracts/test/utils/constants.ts         |  4 ---
 6 files changed, 11 insertions(+), 61 deletions(-)

(limited to 'packages/contracts')

diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
index f02514fc6..291af3792 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
@@ -89,7 +89,7 @@ contract MixinExchangeCore is
         bytes memory signature
     )
         public
-        lockMutex
+        nonReentrant
         returns (FillResults memory fillResults)
     {
         fillResults = fillOrderInternal(
diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
index 39c47e6f9..25220a673 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
@@ -50,7 +50,7 @@ contract MixinMatchOrders is
         bytes memory rightSignature
     )
         public
-        lockMutex
+        nonReentrant
         returns (LibFillResults.MatchedFillResults memory matchedFillResults)
     {
         // We assume that rightOrder.takerAssetData == leftOrder.makerAssetData and rightOrder.makerAssetData == leftOrder.takerAssetData.
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 b0474b110..39fa724cc 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol
@@ -33,7 +33,8 @@ contract MixinWrapperFunctions is
     LibMath,
     LibFillResults,
     LibAbiEncoder,
-    MExchangeCore
+    MExchangeCore,
+    MWrapperFunctions
 {
 
     /// @dev Fills the input order. Reverts if exact takerAssetFillAmount not filled.
@@ -46,7 +47,7 @@ contract MixinWrapperFunctions is
         bytes memory signature
     )
         public
-        lockMutex
+        nonReentrant
         returns (FillResults memory fillResults)
     {
         fillResults = fillOrKillOrderInternal(
@@ -69,7 +70,6 @@ contract MixinWrapperFunctions is
         bytes memory signature
     )
         public
-        nonReentrant
         returns (FillResults memory fillResults)
     {
         // ABI encode calldata for `fillOrder`
@@ -111,7 +111,7 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
-        lockMutex
+        nonReentrant
         returns (FillResults memory totalFillResults)
     {
         uint256 ordersLength = orders.length;
@@ -138,7 +138,7 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
-        lockMutex
+        nonReentrant
         returns (FillResults memory totalFillResults)
     {
         uint256 ordersLength = orders.length;
@@ -166,7 +166,6 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
-        nonReentrant
         returns (FillResults memory totalFillResults)
     {
         uint256 ordersLength = orders.length;
@@ -192,7 +191,7 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
-        lockMutex
+        nonReentrant
         returns (FillResults memory totalFillResults)
     {
         bytes memory takerAssetData = orders[0].takerAssetData;
@@ -237,7 +236,6 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
-        nonReentrant
         returns (FillResults memory totalFillResults)
     {
         bytes memory takerAssetData = orders[0].takerAssetData;
@@ -281,7 +279,7 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
-        lockMutex
+        nonReentrant
         returns (FillResults memory totalFillResults)
     {
         bytes memory makerAssetData = orders[0].makerAssetData;
@@ -334,7 +332,6 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
-        nonReentrant
         returns (FillResults memory totalFillResults)
     {
         bytes memory makerAssetData = orders[0].makerAssetData;
diff --git a/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol b/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol
index 8d575d09a..8bfdd2e66 100644
--- a/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol
+++ b/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol
@@ -40,17 +40,14 @@ contract ReentrantERC20Token is
     );
 
     // All of these functions are potentially vulnerable to reentrancy
+    // We do not test any "noThrow" functions because `fillOrderNoThrow` makes a delegatecall to `fillOrder`
     enum ExchangeFunction {
         FILL_ORDER,
         FILL_OR_KILL_ORDER,
-        FILL_ORDER_NO_THROW,
         BATCH_FILL_ORDERS,
         BATCH_FILL_OR_KILL_ORDERS,
-        BATCH_FILL_ORDERS_NO_THROW,
         MARKET_BUY_ORDERS,
-        MARKET_BUY_ORDERS_NO_THROW,
         MARKET_SELL_ORDERS,
-        MARKET_SELL_ORDERS_NO_THROW,
         MATCH_ORDERS,
         CANCEL_ORDER,
         CANCEL_ORDERS_UP_TO,
@@ -111,13 +108,6 @@ contract ReentrantERC20Token is
                 0,
                 signature
             );
-        } else if (currentFunctionId == uint8(ExchangeFunction.FILL_ORDER_NO_THROW)) {
-            calldata = abi.encodeWithSelector(
-                EXCHANGE.fillOrderNoThrow.selector,
-                order,
-                0,
-                signature
-            );
         } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_FILL_ORDERS)) {
             calldata = abi.encodeWithSelector(
                 EXCHANGE.batchFillOrders.selector,
@@ -132,13 +122,6 @@ contract ReentrantERC20Token is
                 takerAssetFillAmounts,
                 signatures
             );
-        } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_FILL_ORDERS_NO_THROW)) {
-            calldata = abi.encodeWithSelector(
-                EXCHANGE.batchFillOrdersNoThrow.selector,
-                orders,
-                takerAssetFillAmounts,
-                signatures
-            );
         } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_BUY_ORDERS)) {
             calldata = abi.encodeWithSelector(
                 EXCHANGE.marketBuyOrders.selector,
@@ -146,13 +129,6 @@ contract ReentrantERC20Token is
                 0,
                 signatures
             );
-        } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_BUY_ORDERS_NO_THROW)) {
-            calldata = abi.encodeWithSelector(
-                EXCHANGE.marketBuyOrdersNoThrow.selector,
-                orders,
-                0,
-                signatures
-            );
         } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_SELL_ORDERS)) {
             calldata = abi.encodeWithSelector(
                 EXCHANGE.marketSellOrders.selector,
@@ -160,13 +136,6 @@ contract ReentrantERC20Token is
                 0,
                 signatures
             );
-        } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_SELL_ORDERS_NO_THROW)) {
-            calldata = abi.encodeWithSelector(
-                EXCHANGE.marketSellOrdersNoThrow.selector,
-                orders,
-                0,
-                signatures
-            );
         } else if (currentFunctionId == uint8(ExchangeFunction.MATCH_ORDERS)) {
             calldata = abi.encodeWithSelector(
                 EXCHANGE.matchOrders.selector,
diff --git a/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol b/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol
index 2b980c7ca..1dee512d4 100644
--- a/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol
+++ b/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol
@@ -23,21 +23,9 @@ contract ReentrancyGuard {
     // Locked state of mutex
     bool private locked = false;
 
-    /// @dev Functions with this modifier cannot be reentered.
-    modifier nonReentrant() {
-        // Ensure mutex is unlocked
-        require(
-            !locked,
-            "REENTRANCY_ILLEGAL"
-        );
-
-        // Perform function call
-        _;
-    }
-
     /// @dev Functions with this modifer cannot be reentered. The mutex will be locked
     ///      before function execution and unlocked after.
-    modifier lockMutex() {
+    modifier nonReentrant() {
         // Ensure mutex is unlocked
         require(
             !locked,
diff --git a/packages/contracts/test/utils/constants.ts b/packages/contracts/test/utils/constants.ts
index 7060b5091..ee4378d2e 100644
--- a/packages/contracts/test/utils/constants.ts
+++ b/packages/contracts/test/utils/constants.ts
@@ -54,14 +54,10 @@ export const constants = {
     FUNCTIONS_WITH_MUTEX: [
         'FILL_ORDER',
         'FILL_OR_KILL_ORDER',
-        'FILL_ORDER_NO_THROW',
         'BATCH_FILL_ORDERS',
         'BATCH_FILL_OR_KILL_ORDERS',
-        'BATCH_FILL_ORDERS_NO_THROW',
         'MARKET_BUY_ORDERS',
-        'MARKET_BUY_ORDERS_NO_THROW',
         'MARKET_SELL_ORDERS',
-        'MARKET_SELL_ORDERS_NO_THROW',
         'MATCH_ORDERS',
         'CANCEL_ORDER',
         'CANCEL_ORDERS_UP_TO',
-- 
cgit v1.2.3


From c8500cab10b8ffd175acd760cbd1286577a1c4b1 Mon Sep 17 00:00:00 2001
From: Amir Bandeali <abandeali1@gmail.com>
Date: Fri, 24 Aug 2018 16:35:43 -0700
Subject: Fix build

---
 packages/contracts/package.json | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

(limited to 'packages/contracts')

diff --git a/packages/contracts/package.json b/packages/contracts/package.json
index 8d75eb4ce..1c1ac81da 100644
--- a/packages/contracts/package.json
+++ b/packages/contracts/package.json
@@ -20,11 +20,14 @@
         "test:coverage": "SOLIDITY_COVERAGE=true run-s build run_mocha coverage:report:text coverage:report:lcov",
         "test:profiler": "SOLIDITY_PROFILER=true run-s build run_mocha profiler:report:html",
         "test:trace": "SOLIDITY_REVERT_TRACE=true run-s build run_mocha",
-        "run_mocha": "mocha --require source-map-support/register --require make-promises-safe 'lib/test/**/*.js' --timeout 100000 --bail --exit",
+        "run_mocha":
+            "mocha --require source-map-support/register --require make-promises-safe 'lib/test/**/*.js' --timeout 100000 --bail --exit",
         "compile": "sol-compiler --contracts-dir src",
         "clean": "shx rm -rf lib generated_contract_wrappers",
-        "generate_contract_wrappers": "abi-gen --abis  ${npm_package_config_abis} --template ../contract_templates/contract.handlebars --partials '../contract_templates/partials/**/*.handlebars' --output generated_contract_wrappers --backend ethers",
-        "lint": "tslint --project . --exclude **/src/generated_contract_wrappers/**/* --exclude **/lib/**/* && yarn lint-contracts",
+        "generate_contract_wrappers":
+            "abi-gen --abis  ${npm_package_config_abis} --template ../contract_templates/contract.handlebars --partials '../contract_templates/partials/**/*.handlebars' --output generated_contract_wrappers --backend ethers",
+        "lint":
+            "tslint --project . --exclude **/src/generated_contract_wrappers/**/* --exclude **/lib/**/* && yarn lint-contracts",
         "coverage:report:text": "istanbul report text",
         "coverage:report:html": "istanbul report html && open coverage/index.html",
         "profiler:report:html": "istanbul report html && open coverage/index.html",
@@ -34,7 +37,7 @@
     },
     "config": {
         "abis":
-            "../migrations/artifacts/2.0.0/@(AssetProxyOwner|DummyERC20Token|DummyERC721Receiver|DummyERC721Token|DummyNoReturnERC20Token|ERC20Proxy|ERC721Proxy|Forwarder|Exchange|ExchangeWrapper|IAssetData|IAssetProxy|InvalidERC721Receiver|MixinAuthorizable|MultiSigWallet|MultiSigWalletWithTimeLock|OrderValidator|ReentrantERC20Token|TestAssetProxyOwner|TestAssetProxyDispatcher|TestConstants|TestExchangeInternals|TestLibBytes|TestLibs|TestSignatureValidator|Validator|Wallet|TokenRegistry|Whitelist|WETH9|ZRXToken).json"
+            "../migrations/artifacts/2.0.0/@(AssetProxyOwner|DummyERC20Token|DummyERC721Receiver|DummyERC721Token|DummyNoReturnERC20Token|ERC20Proxy|ERC721Proxy|Forwarder|Exchange|ExchangeWrapper|IAssetData|IAssetProxy|InvalidERC721Receiver|MixinAuthorizable|MultiSigWallet|MultiSigWalletWithTimeLock|OrderValidator|ReentrantERC20Token|TestAssetProxyOwner|TestAssetProxyDispatcher|TestConstants|TestExchangeInternals|TestLibBytes|TestLibs|TestSignatureValidator|TestStaticCallReceiver|Validator|Wallet|TokenRegistry|Whitelist|WETH9|ZRXToken).json"
     },
     "repository": {
         "type": "git",
-- 
cgit v1.2.3