From a7c3996627aaedfcd0ba37e7e15895480a0b065c Mon Sep 17 00:00:00 2001 From: Amir Bandeali 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(-) 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; + 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 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(-) 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 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(+) 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 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(-) 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 Date: Thu, 20 Dec 2018 09:11:13 -0800 Subject: Update CHANGELOG --- contracts/protocol/CHANGELOG.json | 4 ++++ 1 file changed, 4 insertions(+) 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