From a7c3996627aaedfcd0ba37e7e15895480a0b065c Mon Sep 17 00:00:00 2001
From: Amir Bandeali <abandeali1@gmail.com>
Date: Tue, 18 Dec 2018 12:08:11 -0800
Subject: Check if amount == 0 before doing division

---
 .../protocol/AssetProxy/MultiAssetProxy.sol        |  5 +++-
 contracts/protocol/test/asset_proxy/proxies.ts     | 32 ++++++++++++++++++++++
 2 files changed, 36 insertions(+), 1 deletion(-)

(limited to 'contracts/protocol')

diff --git a/contracts/protocol/contracts/protocol/AssetProxy/MultiAssetProxy.sol b/contracts/protocol/contracts/protocol/AssetProxy/MultiAssetProxy.sol
index 42231e73b..b42aa79ee 100644
--- a/contracts/protocol/contracts/protocol/AssetProxy/MultiAssetProxy.sol
+++ b/contracts/protocol/contracts/protocol/AssetProxy/MultiAssetProxy.sol
@@ -182,7 +182,10 @@ contract MultiAssetProxy is
                     let totalAmount := mul(amountsElement, amount)
 
                     // Revert if multiplication resulted in an overflow
-                    if iszero(eq(div(totalAmount, amount), amountsElement)) {
+                    if and(
+                        gt(amount, 0),
+                        iszero(eq(div(totalAmount, amount), amountsElement))
+                    ) {
                         // Revert with `Error("UINT256_OVERFLOW")`
                         mstore(0, 0x08c379a000000000000000000000000000000000000000000000000000000000)
                         mstore(32, 0x0000002000000000000000000000000000000000000000000000000000000000)
diff --git a/contracts/protocol/test/asset_proxy/proxies.ts b/contracts/protocol/test/asset_proxy/proxies.ts
index c4bd95905..89c8b390c 100644
--- a/contracts/protocol/test/asset_proxy/proxies.ts
+++ b/contracts/protocol/test/asset_proxy/proxies.ts
@@ -12,6 +12,7 @@ import {
 import {
     artifacts as tokensArtifacts,
     DummyERC20TokenContract,
+    DummyERC20TokenTransferEventArgs,
     DummyERC721ReceiverContract,
     DummyERC721TokenContract,
     DummyMultipleReturnERC20TokenContract,
@@ -22,6 +23,7 @@ import { assetDataUtils } from '@0x/order-utils';
 import { RevertReason } from '@0x/types';
 import { BigNumber } from '@0x/utils';
 import * as chai from 'chai';
+import { LogWithDecodedArgs } from 'ethereum-types';
 import * as _ from 'lodash';
 
 import { ERC20ProxyContract } from '../../generated-wrappers/erc20_proxy';
@@ -738,6 +740,36 @@ describe('Asset Transfer Proxies', () => {
                     erc20Balances[toAddress][erc20TokenA.address].add(totalAmount),
                 );
             });
+            it('should dispatch an ERC20 transfer when input amount is 0', async () => {
+                const inputAmount = constants.ZERO_AMOUNT;
+                const erc20Amount = new BigNumber(10);
+                const erc20AssetData = assetDataUtils.encodeERC20AssetData(erc20TokenA.address);
+                const amounts = [erc20Amount];
+                const nestedAssetData = [erc20AssetData];
+                const assetData = assetDataInterface.MultiAsset.getABIEncodedTransactionData(amounts, nestedAssetData);
+                const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData(
+                    assetData,
+                    fromAddress,
+                    toAddress,
+                    inputAmount,
+                );
+                const erc20Balances = await erc20Wrapper.getBalancesAsync();
+                const logDecoder = new LogDecoder(web3Wrapper, { ...artifacts, ...tokensArtifacts });
+                const tx = await logDecoder.getTxWithDecodedLogsAsync(
+                    await web3Wrapper.sendTransactionAsync({
+                        to: multiAssetProxy.address,
+                        data,
+                        from: authorized,
+                    }),
+                );
+                expect(tx.logs.length).to.be.equal(1);
+                const log = tx.logs[0] as LogWithDecodedArgs<DummyERC20TokenTransferEventArgs>;
+                const transferEventName = 'Transfer';
+                expect(log.event).to.equal(transferEventName);
+                expect(log.args._value).to.be.bignumber.equal(constants.ZERO_AMOUNT);
+                const newBalances = await erc20Wrapper.getBalancesAsync();
+                expect(newBalances).to.deep.equal(erc20Balances);
+            });
             it('should successfully transfer multiple of the same ERC20 token', async () => {
                 const inputAmount = new BigNumber(1);
                 const erc20Amount1 = new BigNumber(10);
-- 
cgit v1.2.3


From 2a2260de45925bc309b6d6baf990d5ea6a171a90 Mon Sep 17 00:00:00 2001
From: Amir Bandeali <abandeali1@gmail.com>
Date: Tue, 18 Dec 2018 15:55:24 -0800
Subject: Use more efficient check for overflow

---
 .../protocol/contracts/protocol/AssetProxy/MultiAssetProxy.sol | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

(limited to 'contracts/protocol')

diff --git a/contracts/protocol/contracts/protocol/AssetProxy/MultiAssetProxy.sol b/contracts/protocol/contracts/protocol/AssetProxy/MultiAssetProxy.sol
index b42aa79ee..4285725d0 100644
--- a/contracts/protocol/contracts/protocol/AssetProxy/MultiAssetProxy.sol
+++ b/contracts/protocol/contracts/protocol/AssetProxy/MultiAssetProxy.sol
@@ -181,11 +181,11 @@ contract MultiAssetProxy is
                     let amountsElement := calldataload(add(amountsContentsStart, i))
                     let totalAmount := mul(amountsElement, amount)
 
-                    // Revert if multiplication resulted in an overflow
-                    if and(
-                        gt(amount, 0),
-                        iszero(eq(div(totalAmount, amount), amountsElement))
-                    ) {
+                    // Revert if `amount` != 0 and multiplication resulted in an overflow 
+                    if iszero(or(
+                        iszero(amount),
+                        eq(div(totalAmount, amount), amountsElement)
+                    )) {
                         // Revert with `Error("UINT256_OVERFLOW")`
                         mstore(0, 0x08c379a000000000000000000000000000000000000000000000000000000000)
                         mstore(32, 0x0000002000000000000000000000000000000000000000000000000000000000)
-- 
cgit v1.2.3


From 04729c44b451bcf4818048621c890960bb7f8afb Mon Sep 17 00:00:00 2001
From: Amir Bandeali <abandeali1@gmail.com>
Date: Wed, 19 Dec 2018 11:27:10 -0800
Subject: Add note about input validation

---
 contracts/protocol/contracts/protocol/AssetProxy/MultiAssetProxy.sol | 3 +++
 1 file changed, 3 insertions(+)

(limited to 'contracts/protocol')

diff --git a/contracts/protocol/contracts/protocol/AssetProxy/MultiAssetProxy.sol b/contracts/protocol/contracts/protocol/AssetProxy/MultiAssetProxy.sol
index 4285725d0..5bc32c214 100644
--- a/contracts/protocol/contracts/protocol/AssetProxy/MultiAssetProxy.sol
+++ b/contracts/protocol/contracts/protocol/AssetProxy/MultiAssetProxy.sol
@@ -33,6 +33,9 @@ contract MultiAssetProxy is
     function ()
         external
     {
+        // NOTE: The below assembly assumes that clients do some input validation and that the input is properly encoded according to the AbiV2 specification.
+        // It is technically possible for inputs with very large lengths and offsets to cause overflows. However, this would make the calldata prohibitively expensive
+        // and we therefore do not check for overflows in these scenarios.
         assembly {
             // The first 4 bytes of calldata holds the function selector
             let selector := and(calldataload(0), 0xffffffff00000000000000000000000000000000000000000000000000000000)
-- 
cgit v1.2.3


From 99e32869e602e9b01d74048af23594986aa639a1 Mon Sep 17 00:00:00 2001
From: Amir Bandeali <abandeali1@gmail.com>
Date: Wed, 19 Dec 2018 14:35:28 -0800
Subject: Use more efficient equality checks

---
 contracts/protocol/contracts/protocol/AssetProxy/MultiAssetProxy.sol | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

(limited to 'contracts/protocol')

diff --git a/contracts/protocol/contracts/protocol/AssetProxy/MultiAssetProxy.sol b/contracts/protocol/contracts/protocol/AssetProxy/MultiAssetProxy.sol
index 5bc32c214..377325384 100644
--- a/contracts/protocol/contracts/protocol/AssetProxy/MultiAssetProxy.sol
+++ b/contracts/protocol/contracts/protocol/AssetProxy/MultiAssetProxy.sol
@@ -148,7 +148,7 @@ contract MultiAssetProxy is
                 let nestedAssetDataLen := calldataload(sub(nestedAssetDataContentsStart, 32))
 
                 // Revert if number of elements in `amounts` differs from number of elements in `nestedAssetData`
-                if iszero(eq(amountsLen, nestedAssetDataLen)) {
+                if sub(amountsLen, nestedAssetDataLen) {
                     // Revert with `Error("LENGTH_MISMATCH")`
                     mstore(0, 0x08c379a000000000000000000000000000000000000000000000000000000000)
                     mstore(32, 0x0000002000000000000000000000000000000000000000000000000000000000)
@@ -236,7 +236,7 @@ contract MultiAssetProxy is
 
                     // Only load `assetProxy` if `currentAssetProxyId` does not equal `assetProxyId`
                     // We do not need to check if `currentAssetProxyId` is 0 since `assetProxy` is also initialized to 0
-                    if iszero(eq(currentAssetProxyId, assetProxyId)) {
+                    if sub(currentAssetProxyId, assetProxyId) {
                         // Update `assetProxyId`
                         assetProxyId := currentAssetProxyId
                         // To lookup a value in a mapping, we load from the storage location keccak256(k, p),
-- 
cgit v1.2.3


From 8e5f0f9c5cffd44108b427585e571bec9eeb1b40 Mon Sep 17 00:00:00 2001
From: Amir Bandeali <abandeali1@gmail.com>
Date: Thu, 20 Dec 2018 09:11:13 -0800
Subject: Update CHANGELOG

---
 contracts/protocol/CHANGELOG.json | 4 ++++
 1 file changed, 4 insertions(+)

(limited to 'contracts/protocol')

diff --git a/contracts/protocol/CHANGELOG.json b/contracts/protocol/CHANGELOG.json
index 2dca9794a..be374d892 100644
--- a/contracts/protocol/CHANGELOG.json
+++ b/contracts/protocol/CHANGELOG.json
@@ -5,6 +5,10 @@
             {
                 "note": "Added LibAddressArray",
                 "pr": 1383
+            },
+            {
+                "note": "Add validation and comments to MultiAssetProxy",
+                "pr": 1455
             }
         ]
     },
-- 
cgit v1.2.3