From 1b8a9e16e24bc3d7f1d1e169b85ca9586b6fbffa Mon Sep 17 00:00:00 2001 From: Michael Zhu Date: Fri, 12 Jul 2019 14:48:53 -0700 Subject: [PATCH] slightly restructure contracts to fix bug in the WETH-fee case --- .../contracts/src/MixinExchangeWrapper.sol | 103 +++++++++++++----- .../contracts/src/MixinForwarderCore.sol | 24 +++- .../contracts/src/LibFillResults.sol | 15 --- 3 files changed, 93 insertions(+), 49 deletions(-) diff --git a/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol b/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol index 2a4bb8d1b6..6a59d421ff 100644 --- a/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol +++ b/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol @@ -89,11 +89,13 @@ contract MixinExchangeWrapper is bytes[] memory signatures ) internal - returns (FillResults memory totalFillResults) + returns ( + FillResults memory totalFillResults, + uint256 wethSpentAmount, + uint256 makerAssetAcquiredAmount + ) { uint256 ordersLength = orders.length; - // The remaining amount of WETH to sell - uint256 remainingTakerAssetFillAmount = wethSellAmount; for (uint256 i = 0; i != ordersLength; i++) { require( @@ -101,34 +103,62 @@ contract MixinExchangeWrapper is "MAKER_ASSET_MISMATCH" ); - // Attempt to sell the remaining amount of WETH - FillResults memory singleFillResults = _fillOrderNoThrow( - orders[i], - remainingTakerAssetFillAmount, - signatures[i] + // The remaining amount of WETH to sell + uint256 remainingTakerAssetFillAmount = _safeSub( + wethSellAmount, + wethSpentAmount ); + FillResults memory singleFillResults; // Update amounts filled and the remaining amount of WETH to sell if (orders[i].makerAssetData.equals(orders[i].takerFeeAssetData)) { - _addFillResultsDeductFees(totalFillResults, singleFillResults); - } else { - _addFillResults(totalFillResults, singleFillResults); - remainingTakerAssetFillAmount = _safeSub( + // Attempt to sell the remaining amount of WETH + singleFillResults = _fillOrderNoThrow( + orders[i], remainingTakerAssetFillAmount, - singleFillResults.takerFeePaid + signatures[i] + ); + + makerAssetAcquiredAmount = _safeAdd( + makerAssetAcquiredAmount, + _safeSub(singleFillResults.makerAssetFilledAmount, singleFillResults.takerFeePaid) + ); + wethSpentAmount = _safeAdd( + wethSpentAmount, + singleFillResults.takerAssetFilledAmount + ); + } else { + // We first sell WETH as the takerAsset, then use it to pay the takerFee. + // This ensures that we reserve enough to pay the fee. + uint256 takerAssetFillAmount = _getPartialAmountCeil( + orders[i].takerAssetAmount, + _safeAdd(orders[i].takerAssetAmount, orders[i].takerFee), + remainingTakerAssetFillAmount + ); + singleFillResults = _fillOrderNoThrow( + orders[i], + takerAssetFillAmount, + signatures[i] + ); + + wethSpentAmount = _safeAdd( + wethSpentAmount, + _safeAdd(singleFillResults.takerAssetFilledAmount, singleFillResults.takerFeePaid) + ); + makerAssetAcquiredAmount = _safeAdd( + makerAssetAcquiredAmount, + singleFillResults.makerAssetFilledAmount ); } - remainingTakerAssetFillAmount = _safeSub( - remainingTakerAssetFillAmount, - singleFillResults.takerAssetFilledAmount - ); + + _addFillResults(totalFillResults, singleFillResults); // Stop execution if the entire amount of takerAsset has been sold - if (totalFillResults.takerAssetFilledAmount >= wethSellAmount) { + if (wethSpentAmount >= wethSellAmount) { break; } } - return totalFillResults; + return (totalFillResults, wethSpentAmount, makerAssetAcquiredAmount); } /// @dev Synchronously executes multiple fill orders in a single transaction until total amount is bought by taker. @@ -144,10 +174,13 @@ contract MixinExchangeWrapper is bytes[] memory signatures ) internal - returns (FillResults memory totalFillResults) + returns ( + FillResults memory totalFillResults, + uint256 wethSpentAmount, + uint256 makerAssetAcquiredAmount + ) { uint256 ordersLength = orders.length; - uint256 makerAssetFilledAmount = 0; for (uint256 i = 0; i != ordersLength; i++) { require( @@ -156,7 +189,7 @@ contract MixinExchangeWrapper is ); // Calculate the remaining amount of makerAsset to buy - uint256 remainingMakerAssetFillAmount = _safeSub(makerAssetFillAmount, totalFillResults.makerAssetFilledAmount); + uint256 remainingMakerAssetFillAmount = _safeSub(makerAssetFillAmount, makerAssetAcquiredAmount); // 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. @@ -177,22 +210,36 @@ contract MixinExchangeWrapper is // Update amounts filled and fees paid by maker and taker if (orders[i].makerAssetData.equals(orders[i].takerFeeAssetData)) { - _addFillResultsDeductFees(totalFillResults, singleFillResults); + makerAssetAcquiredAmount = _safeAdd( + makerAssetAcquiredAmount, + _safeSub(singleFillResults.makerAssetFilledAmount, singleFillResults.takerFeePaid) + ); + wethSpentAmount = _safeAdd( + wethSpentAmount, + singleFillResults.takerAssetFilledAmount + ); } else { - _addFillResults(totalFillResults, singleFillResults); + makerAssetAcquiredAmount = _safeAdd( + makerAssetAcquiredAmount, + _safeSub(singleFillResults.makerAssetFilledAmount, singleFillResults.takerFeePaid) + ); + wethSpentAmount = _safeAdd( + wethSpentAmount, + singleFillResults.takerAssetFilledAmount + ); } + _addFillResults(totalFillResults, singleFillResults); // Stop execution if the entire amount of makerAsset has been bought - makerAssetFilledAmount = totalFillResults.makerAssetFilledAmount; - if (makerAssetFilledAmount >= makerAssetFillAmount) { + if (makerAssetAcquiredAmount >= makerAssetFillAmount) { break; } } require( - makerAssetFilledAmount >= makerAssetFillAmount, + makerAssetAcquiredAmount >= makerAssetFillAmount, "COMPLETE_FILL_FAILED" ); - return totalFillResults; + return (totalFillResults, wethSpentAmount, makerAssetAcquiredAmount); } } diff --git a/contracts/exchange-forwarder/contracts/src/MixinForwarderCore.sol b/contracts/exchange-forwarder/contracts/src/MixinForwarderCore.sol index 5359bc33ee..c59c5c578b 100644 --- a/contracts/exchange-forwarder/contracts/src/MixinForwarderCore.sol +++ b/contracts/exchange-forwarder/contracts/src/MixinForwarderCore.sol @@ -88,7 +88,13 @@ contract MixinForwarderCore is // Market sell 95% of WETH. // ZRX fees are payed with this contract's balance. - orderFillResults = _marketSellWeth( + uint256 wethSpentAmount; + uint256 makerAssetAcquiredAmount; + ( + orderFillResults, + wethSpentAmount, + makerAssetAcquiredAmount + ) = _marketSellWeth( orders, wethSellAmount, signatures @@ -97,7 +103,7 @@ contract MixinForwarderCore is // Transfer feePercentage of total ETH spent on primary orders to feeRecipient. // Refund remaining ETH to msg.sender. _transferEthFeeAndRefund( - orderFillResults.takerAssetFilledAmount, + wethSpentAmount, feePercentage, feeRecipient ); @@ -105,7 +111,7 @@ contract MixinForwarderCore is // Transfer purchased assets to msg.sender. _transferAssetToSender( orders[0].makerAssetData, - orderFillResults.makerAssetFilledAmount + makerAssetAcquiredAmount ); } @@ -136,7 +142,13 @@ contract MixinForwarderCore is // Attempt to purchase desired amount of makerAsset. // ZRX fees are payed with this contract's balance. - orderFillResults = _marketBuyExactAmountWithWeth( + uint256 wethSpentAmount; + uint256 makerAssetAcquiredAmount; + ( + orderFillResults, + wethSpentAmount, + makerAssetAcquiredAmount + ) = _marketBuyExactAmountWithWeth( orders, makerAssetFillAmount, signatures @@ -145,7 +157,7 @@ contract MixinForwarderCore is // Transfer feePercentage of total ETH spent on primary orders to feeRecipient. // Refund remaining ETH to msg.sender. _transferEthFeeAndRefund( - orderFillResults.takerAssetFilledAmount, + wethSpentAmount, feePercentage, feeRecipient ); @@ -153,7 +165,7 @@ contract MixinForwarderCore is // Transfer purchased assets to msg.sender. _transferAssetToSender( orders[0].makerAssetData, - orderFillResults.makerAssetFilledAmount + makerAssetAcquiredAmount ); } } diff --git a/contracts/exchange-libs/contracts/src/LibFillResults.sol b/contracts/exchange-libs/contracts/src/LibFillResults.sol index 374e35d200..d3781b4841 100644 --- a/contracts/exchange-libs/contracts/src/LibFillResults.sol +++ b/contracts/exchange-libs/contracts/src/LibFillResults.sol @@ -402,19 +402,4 @@ library LibFillResults { return matchedFillResults; } - - /// @dev Adds properties of both FillResults instances. - /// Modifies the first FillResults instance specified. - /// @param totalFillResults Fill results instance that will be added onto. - /// @param singleFillResults Fill results instance that will be added to totalFillResults. - function _addFillResultsDeductFees(FillResults memory totalFillResults, FillResults memory singleFillResults) - internal - pure - { - totalFillResults.makerAssetFilledAmount = _safeAdd(totalFillResults.makerAssetFilledAmount, singleFillResults.makerAssetFilledAmount); - totalFillResults.takerAssetFilledAmount = _safeAdd(totalFillResults.takerAssetFilledAmount, singleFillResults.takerAssetFilledAmount); - - // The fee amount must be deducted from the amount transfered back to sender. - totalFillResults.makerAssetFilledAmount = _safeSub(totalFillResults.makerAssetFilledAmount, singleFillResults.takerFeePaid); - } }