From 6fb157488cf337a1b3c96fc4a9b04d81098bd27d Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 10 Jul 2018 15:41:33 -0700 Subject: Update transferEthFeeAndRefund, add check to ERC721 transfer --- .../src/2.0.0/forwarder/LibForwarderErrors.sol | 34 ++++++++++++++++++ .../contracts/src/2.0.0/forwarder/MixinAssets.sol | 12 +++++-- .../src/2.0.0/forwarder/MixinErrorMessages.sol | 33 ------------------ .../src/2.0.0/forwarder/MixinForwarderCore.sol | 13 ++++--- .../contracts/src/2.0.0/forwarder/MixinWeth.sol | 40 +++++++++++++++------- .../src/2.0.0/forwarder/mixins/MAssets.sol | 6 +++- .../src/2.0.0/protocol/Exchange/libs/LibMath.sol | 6 ++-- 7 files changed, 86 insertions(+), 58 deletions(-) create mode 100644 packages/contracts/src/2.0.0/forwarder/LibForwarderErrors.sol delete mode 100644 packages/contracts/src/2.0.0/forwarder/MixinErrorMessages.sol (limited to 'packages/contracts/src/2.0.0') diff --git a/packages/contracts/src/2.0.0/forwarder/LibForwarderErrors.sol b/packages/contracts/src/2.0.0/forwarder/LibForwarderErrors.sol new file mode 100644 index 000000000..69108738a --- /dev/null +++ b/packages/contracts/src/2.0.0/forwarder/LibForwarderErrors.sol @@ -0,0 +1,34 @@ +/* + + 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. + +*/ + +// solhint-disable +pragma solidity 0.4.24; + + +/// This contract is intended to serve as a reference, but is not actually used for efficiency reasons. +contract LibForwarderErrors { + string constant FEE_PERCENTAGE_TOO_LARGE = "FEE_PROPORTION_TOO_LARGE"; // Provided fee percentage greater than 5%. + string constant MAX_FEE_EXCEEDED = "MAX_FEE_EXCEEDED"; // Not enough ETH remaining to pay feeRecipient. + string constant OVERSOLD_WETH = "OVERSOLD_WETH"; // More WETH sold than provided with current message call. + string constant COMPLETE_FILL_FAILED = "COMPLETE_FILL_FAILED"; // Desired purchase amount not completely filled (required for ZRX fees only). + string constant TRANSFER_FAILED = "TRANSFER_FAILED"; // Asset transfer failed. + string constant UNSUPPORTED_TOKEN_PROXY = "UNSUPPORTED_TOKEN_PROXY"; // Proxy in assetData not supported. + string constant DEFAULT_FUNCTION_WETH_CONTRACT_ONLY = "DEFAULT_FUNCTION_WETH_CONTRACT_ONLY"; // Fallback function may only be used for WETH withdrawals. + string constant INVALID_MSG_VALUE = "INVALID_MSG_VALUE"; // msg.value must be greater than 0. + string constant INVALID_AMOUNT = "INVALID_AMOUNT"; // Amount must equal 1. +} diff --git a/packages/contracts/src/2.0.0/forwarder/MixinAssets.sol b/packages/contracts/src/2.0.0/forwarder/MixinAssets.sol index 325af5518..cd150764e 100644 --- a/packages/contracts/src/2.0.0/forwarder/MixinAssets.sol +++ b/packages/contracts/src/2.0.0/forwarder/MixinAssets.sol @@ -95,7 +95,7 @@ contract MixinAssets is if (proxyId == ERC20_DATA_ID) { transferERC20Token(assetData, amount); } else if (proxyId == ERC721_DATA_ID) { - transferERC721Token(assetData); + transferERC721Token(assetData, amount); } else { revert("UNSUPPORTED_TOKEN_PROXY"); } @@ -149,9 +149,17 @@ contract MixinAssets is /// @dev Decodes ERC721 assetData and transfers given amount to sender. /// @param assetData Byte array encoded for the respective asset proxy. - function transferERC721Token(bytes memory assetData) + /// @param amount Amount of asset to transfer to sender. + function transferERC721Token( + bytes memory assetData, + uint256 amount + ) internal { + require( + amount == 1, + "INVALID_AMOUNT" + ); // Decode asset data. address token = assetData.readAddress(16); uint256 tokenId = assetData.readUint256(36); diff --git a/packages/contracts/src/2.0.0/forwarder/MixinErrorMessages.sol b/packages/contracts/src/2.0.0/forwarder/MixinErrorMessages.sol deleted file mode 100644 index 91758eadc..000000000 --- a/packages/contracts/src/2.0.0/forwarder/MixinErrorMessages.sol +++ /dev/null @@ -1,33 +0,0 @@ -/* - - 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. - -*/ - -// solhint-disable -pragma solidity 0.4.24; - - -/// This contract is intended to serve as a reference, but is not actually used for efficiency reasons. -contract MixinErrorMessages { - string constant FEE_PERCENTAGE_TOO_LARGE = "FEE_PROPORTION_TOO_LARGE"; // Provided fee percentage greater than 5%. - string constant MAX_FEE_EXCEEDED = "MAX_FEE_EXCEEDED"; // Not enough ETH remaining to pay feeRecipient. - string constant OVERSOLD_WETH = "OVERSOLD_WETH"; // More WETH sold than provided with current message call. - string constant COMPLETE_FILL_FAILED = "COMPLETE_FILL_FAILED"; // Desired purchase amount not completely filled (required for ZRX fees only). - string constant TRANSFER_FAILED = "TRANSFER_FAILED"; // Asset transfer failed. - string constant UNSUPPORTED_TOKEN_PROXY = "UNSUPPORTED_TOKEN_PROXY"; // Proxy in assetData not supported. - string constant DEFAULT_FUNCTION_WETH_CONTRACT_ONLY = "DEFAULT_FUNCTION_WETH_CONTRACT_ONLY"; // Fallback function may only be used for WETH withdrawals. - string constant INVALID_MSG_VALUE = "INVALID_MSG_VALUE"; // msg.value must be greater than 0. -} diff --git a/packages/contracts/src/2.0.0/forwarder/MixinForwarderCore.sol b/packages/contracts/src/2.0.0/forwarder/MixinForwarderCore.sol index 1a0687ab5..88ec5f472 100644 --- a/packages/contracts/src/2.0.0/forwarder/MixinForwarderCore.sol +++ b/packages/contracts/src/2.0.0/forwarder/MixinForwarderCore.sol @@ -80,7 +80,7 @@ contract MixinForwarderCore is // Attempt to sell 95% of WETH. // ZRX fees are payed with this contract's balance. - marketSellEth( + orderFillResults = marketSellEth( orders, wethAvailable, signatures @@ -258,6 +258,7 @@ contract MixinForwarderCore is bytes memory zrxAssetData = ZRX_ASSET_DATA; bytes memory wethAssetData = WETH_ASSET_DATA; + uint256 zrxPurchased = 0; for (uint256 i = 0; i < orders.length; i++) { @@ -266,10 +267,7 @@ contract MixinForwarderCore is orders[i].takerAssetData = wethAssetData; // Calculate the remaining amount of ZRX to buy. - uint256 remainingZrxBuyAmount = safeAdd( - safeSub(zrxBuyAmount, totalFillResults.makerAssetFilledAmount), - totalFillResults.takerFeePaid - ); + uint256 remainingZrxBuyAmount = safeSub(zrxBuyAmount, zrxPurchased); // 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. @@ -288,16 +286,17 @@ contract MixinForwarderCore is // Update amounts filled and fees paid by maker and taker. addFillResults(totalFillResults, singleFillResult); + zrxPurchased = safeSub(totalFillResults.makerAssetFilledAmount, totalFillResults.takerFeePaid); // Stop execution if the entire amount of ZRX has been bought. - if (totalFillResults.makerAssetFilledAmount == zrxBuyAmount) { + if (zrxPurchased == zrxBuyAmount) { break; } } // Ensure that all ZRX spent while filling primary orders has been repurchased. require( - totalFillResults.makerAssetFilledAmount == zrxBuyAmount, + zrxPurchased == zrxBuyAmount, "COMPLETE_FILL_FAILED" ); } diff --git a/packages/contracts/src/2.0.0/forwarder/MixinWeth.sol b/packages/contracts/src/2.0.0/forwarder/MixinWeth.sol index cbc81a11c..1d0c315f5 100644 --- a/packages/contracts/src/2.0.0/forwarder/MixinWeth.sol +++ b/packages/contracts/src/2.0.0/forwarder/MixinWeth.sol @@ -78,32 +78,46 @@ contract MixinWeth is ) internal { + // Ensure feePercentage is less than 5%. + require( + feePercentage <= MAX_FEE_PERCENTAGE, + "FEE_PERCENTAGE_TOO_LARGE" + ); + + // Calculate amount of WETH that hasn't been sold. uint256 wethRemaining = safeSub( msg.value, safeAdd(wethSoldExcludingFeeOrders, wethSoldForZrx) ); - ETHER_TOKEN.withdraw(wethRemaining); - require( - feePercentage <= MAX_FEE_PERCENTAGE, - "FEE_PERCENTAGE_TOO_LARGE" - ); + // Calculate ETH fee to pay to feeRecipient. uint256 ethFee = getPartialAmount( feePercentage, PERCENTAGE_DENOMINATOR, wethSoldExcludingFeeOrders ); + + // Ensure fee is less than amount of WETH remaining. require( - ethFee < wethRemaining, + ethFee <= wethRemaining, "MAX_FEE_EXCEEDED" ); - if (ethFee > 0) { - feeRecipient.transfer(ethFee); - } - - uint256 ethRefund = safeSub(wethRemaining, ethFee); - if (ethRefund > 0) { - msg.sender.transfer(ethRefund); + + // Do nothing if no WETH remaining + if (wethRemaining > 0) { + // Convert remaining WETH to ETH + ETHER_TOKEN.withdraw(wethRemaining); + + // Pay ETH to feeRecipient + if (ethFee > 0) { + feeRecipient.transfer(ethFee); + } + + // Refund remaining ETH to msg.sender. + uint256 ethRefund = safeSub(wethRemaining, ethFee); + if (ethRefund > 0) { + msg.sender.transfer(ethRefund); + } } } } diff --git a/packages/contracts/src/2.0.0/forwarder/mixins/MAssets.sol b/packages/contracts/src/2.0.0/forwarder/mixins/MAssets.sol index b69f7482d..340ee0bcb 100644 --- a/packages/contracts/src/2.0.0/forwarder/mixins/MAssets.sol +++ b/packages/contracts/src/2.0.0/forwarder/mixins/MAssets.sol @@ -45,6 +45,10 @@ contract MAssets is /// @dev Decodes ERC721 assetData and transfers given amount to sender. /// @param assetData Byte array encoded for the respective asset proxy. - function transferERC721Token(bytes memory assetData) + /// @param amount Amount of asset to transfer to sender. + function transferERC721Token( + bytes memory assetData, + uint256 amount + ) internal; } 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 46c13d390..fa09da6ac 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 @@ -33,7 +33,8 @@ contract LibMath is function getPartialAmount( uint256 numerator, uint256 denominator, - uint256 target) + uint256 target + ) internal pure returns (uint256 partialAmount) @@ -53,7 +54,8 @@ contract LibMath is function isRoundingError( uint256 numerator, uint256 denominator, - uint256 target) + uint256 target + ) internal pure returns (bool isError) -- cgit