From 7f2b715cebac5a57bde85b272c6607098536ee93 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Tue, 30 Jul 2019 17:30:26 -0700 Subject: [PATCH 1/2] Rewrite _dispatchTransferFrom with a Solidity implementation --- .../src/MixinAssetProxyDispatcher.sol | 107 +++--------------- .../contracts/src/interfaces/IAssetProxy.sol | 2 +- 2 files changed, 18 insertions(+), 91 deletions(-) diff --git a/contracts/exchange/contracts/src/MixinAssetProxyDispatcher.sol b/contracts/exchange/contracts/src/MixinAssetProxyDispatcher.sol index 4a022f7fc0..5accd6e49a 100644 --- a/contracts/exchange/contracts/src/MixinAssetProxyDispatcher.sol +++ b/contracts/exchange/contracts/src/MixinAssetProxyDispatcher.sol @@ -19,6 +19,7 @@ pragma solidity ^0.5.9; import "@0x/contracts-utils/contracts/src/Ownable.sol"; +import "@0x/contracts-utils/contracts/src/LibBytes.sol"; import "@0x/contracts-utils/contracts/src/LibRichErrors.sol"; import "./interfaces/IAssetProxy.sol"; import "./interfaces/IAssetProxyDispatcher.sol"; @@ -30,6 +31,8 @@ contract MixinAssetProxyDispatcher is Ownable, IAssetProxyDispatcher { + using LibBytes for bytes; + // Mapping from Asset Proxy Id's to their respective Asset Proxy mapping (bytes4 => address) public assetProxies; @@ -92,14 +95,8 @@ contract MixinAssetProxyDispatcher is )); } - // Lookup assetProxy. We do not use `LibBytes.readBytes4` for gas efficiency reasons. - bytes4 assetProxyId; - assembly { - assetProxyId := and(mload( - add(assetData, 32)), - 0xFFFFFFFF00000000000000000000000000000000000000000000000000000000 - ) - } + // Lookup assetProxy. + bytes4 assetProxyId = assetData.readBytes4(0); address assetProxy = assetProxies[assetProxyId]; // Ensure that assetProxy exists @@ -111,89 +108,19 @@ contract MixinAssetProxyDispatcher is )); } - // Whether the AssetProxy transfer succeeded. - bool didSucceed; - // On failure, the revert data thrown by the asset proxy. - bytes memory revertData; - - // We construct calldata for the `assetProxy.transferFrom` ABI. - // The layout of this calldata is in the table below. - // - // | Area | Offset | Length | Contents | - // | -------- |--------|---------|-------------------------------------------- | - // | Header | 0 | 4 | function selector | - // | Params | | 4 * 32 | function parameters: | - // | | 4 | | 1. offset to assetData (*) | - // | | 36 | | 2. from | - // | | 68 | | 3. to | - // | | 100 | | 4. amount | - // | Data | | | assetData: | - // | | 132 | 32 | assetData Length | - // | | 164 | ** | assetData Contents | - - assembly { - /////// Setup State /////// - // `cdStart` is the start of the calldata for `assetProxy.transferFrom` (equal to free memory ptr). - let cdStart := mload(64) - // `dataAreaLength` is the total number of words needed to store `assetData` - // As-per the ABI spec, this value is padded up to the nearest multiple of 32, - // and includes 32-bytes for length. - let dataAreaLength := and(add(mload(assetData), 63), 0xFFFFFFFFFFFE0) - // `cdEnd` is the end of the calldata for `assetProxy.transferFrom`. - let cdEnd := add(cdStart, add(132, dataAreaLength)) - - /////// Setup Header Area /////// - // This area holds the 4-byte `transferFromSelector`. - // bytes4(keccak256("transferFrom(bytes,address,address,uint256)")) = 0xa85e59e4 - mstore(cdStart, 0xa85e59e400000000000000000000000000000000000000000000000000000000) - - /////// Setup Params Area /////// - // Each parameter is padded to 32-bytes. The entire Params Area is 128 bytes. - // Notes: - // 1. The offset to `assetData` is the length of the Params Area (128 bytes). - // 2. A 20-byte mask is applied to addresses to zero-out the unused bytes. - mstore(add(cdStart, 4), 128) - mstore(add(cdStart, 36), and(from, 0xffffffffffffffffffffffffffffffffffffffff)) - mstore(add(cdStart, 68), and(to, 0xffffffffffffffffffffffffffffffffffffffff)) - mstore(add(cdStart, 100), amount) - - /////// Setup Data Area /////// - // This area holds `assetData`. - let dataArea := add(cdStart, 132) - let assetDataArea := assetData - // solhint-disable-next-line no-empty-blocks - for {} lt(dataArea, cdEnd) {} { - mstore(dataArea, mload(assetDataArea)) - dataArea := add(dataArea, 32) - assetDataArea := add(assetDataArea, 32) - } - - /////// Call `assetProxy.transferFrom` using the constructed calldata /////// - didSucceed := call( - gas, // forward all gas - assetProxy, // call address of asset proxy - 0, // don't send any ETH - cdStart, // pointer to start of input - sub(cdEnd, cdStart), // length of input - 0, // don't store the returndata - 0 // don't store the returndata - ) - - if iszero(didSucceed) { // Call reverted. - let revertDataSize := returndatasize() - // Construct a `bytes memory` type to hold the revert data - // at `cdStart`. - // The first 32 bytes are the length of the data. - mstore(cdStart, revertDataSize) - // Copy the revert data immediately after the length. - returndatacopy(add(cdStart, 32), 0, revertDataSize) - // We need to move the free memory pointer because we - // still have solidity logic that executes after this assembly. - mstore(64, add(cdStart, add(revertDataSize, 32))) - revertData := cdStart - } - } + // Construct the calldata for the transferFrom call. + bytes memory proxyCalldata = abi.encodeWithSelector( + IAssetProxy(address(0)).transferFrom.selector, + assetData, + from, + to, + amount + ); + + // Call the asset proxy's transferFrom function with the constructed calldata. + (bool didSucceed, bytes memory revertData) = assetProxy.call(proxyCalldata); + // If the transaction did not succeed, revert with the returned data. if (!didSucceed) { LibRichErrors._rrevert(LibExchangeRichErrors.AssetProxyTransferError( orderHash, diff --git a/contracts/exchange/contracts/src/interfaces/IAssetProxy.sol b/contracts/exchange/contracts/src/interfaces/IAssetProxy.sol index 719b4359c6..a33760ae8b 100644 --- a/contracts/exchange/contracts/src/interfaces/IAssetProxy.sol +++ b/contracts/exchange/contracts/src/interfaces/IAssetProxy.sol @@ -33,7 +33,7 @@ contract IAssetProxy { uint256 amount ) external; - + /// @dev Gets the proxy id associated with the proxy address. /// @return Proxy id. function getProxyId() From bf8fae2025e00dbf98d9fb09157151ba7e69d88e Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Tue, 30 Jul 2019 17:36:13 -0700 Subject: [PATCH 2/2] Update changelog --- contracts/exchange/CHANGELOG.json | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contracts/exchange/CHANGELOG.json b/contracts/exchange/CHANGELOG.json index 21340834a5..fd52350d72 100644 --- a/contracts/exchange/CHANGELOG.json +++ b/contracts/exchange/CHANGELOG.json @@ -113,6 +113,10 @@ { "note": "Updated RichErrors to the library pattern", "pr": 1913 + }, + { + "note": "Rewrote _dispatchTransferFrom in Solidity", + "pr": 2020 } ] },