diff --git a/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol b/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol index b44d5f8c14..9b7b1e2e78 100644 --- a/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol +++ b/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol @@ -53,6 +53,7 @@ contract MixinExchangeWrapper { // ")" // ))); bytes4 constant public EXCHANGE_V2_ORDER_ID = 0x770501f8; + bytes4 constant internal ERC20_BRIDGE_PROXY_ID = 0xdc1600f3; // solhint-disable var-name-mixedcase IExchange internal EXCHANGE; @@ -123,6 +124,14 @@ contract MixinExchangeWrapper { internal returns (SellFillResults memory sellFillResults) { + // If the maker asset is ERC20Bridge, take a snapshot of the Forwarder contract's balance. + bytes4 makerAssetProxyId = order.makerAssetData.readBytes4(0); + address tokenAddress; + uint256 balanceBefore; + if (makerAssetProxyId == ERC20_BRIDGE_PROXY_ID) { + tokenAddress = order.makerAssetData.readAddress(16); + balanceBefore = IERC20Token(tokenAddress).balanceOf(address(this)); + } // No taker fee or percentage fee if ( order.takerFee == 0 || @@ -170,6 +179,16 @@ contract MixinExchangeWrapper { LibRichErrors.rrevert(LibForwarderRichErrors.UnsupportedFeeError(order.takerFeeAssetData)); } + // Account for the ERC20Bridge transfering more of the maker asset than expected. + if (makerAssetProxyId == ERC20_BRIDGE_PROXY_ID) { + uint256 balanceAfter = IERC20Token(tokenAddress).balanceOf(address(this)); + sellFillResults.makerAssetAcquiredAmount = LibSafeMath.max256( + balanceAfter.safeSub(balanceBefore), + sellFillResults.makerAssetAcquiredAmount + ); + } + + order.makerAssetData.transferOut(sellFillResults.makerAssetAcquiredAmount); return sellFillResults; } @@ -192,7 +211,6 @@ contract MixinExchangeWrapper { ) { uint256 protocolFee = tx.gasprice.safeMul(EXCHANGE.protocolFeeMultiplier()); - bytes4 erc20BridgeProxyId = IAssetData(address(0)).ERC20Bridge.selector; for (uint256 i = 0; i != orders.length; i++) { // Preemptively skip to avoid division by zero in _marketSellSingleOrder @@ -205,32 +223,12 @@ contract MixinExchangeWrapper { .safeSub(totalWethSpentAmount) .safeSub(_isV2Order(orders[i]) ? 0 : protocolFee); - // If the maker asset is ERC20Bridge, take a snapshot of the Forwarder contract's balance. - bytes4 makerAssetProxyId = orders[i].makerAssetData.readBytes4(0); - address tokenAddress; - uint256 balanceBefore; - if (makerAssetProxyId == erc20BridgeProxyId) { - tokenAddress = orders[i].makerAssetData.readAddress(16); - balanceBefore = IERC20Token(tokenAddress).balanceOf(address(this)); - } - SellFillResults memory sellFillResults = _marketSellSingleOrder( orders[i], signatures[i], remainingTakerAssetFillAmount ); - // Account for the ERC20Bridge transfering more of the maker asset than expected. - if (makerAssetProxyId == erc20BridgeProxyId) { - uint256 balanceAfter = IERC20Token(tokenAddress).balanceOf(address(this)); - sellFillResults.makerAssetAcquiredAmount = LibSafeMath.max256( - balanceAfter.safeSub(balanceBefore), - sellFillResults.makerAssetAcquiredAmount - ); - } - - orders[i].makerAssetData.transferOut(sellFillResults.makerAssetAcquiredAmount); - totalWethSpentAmount = totalWethSpentAmount .safeAdd(sellFillResults.wethSpentAmount) .safeAdd(sellFillResults.protocolFeePaid); @@ -262,7 +260,6 @@ contract MixinExchangeWrapper { uint256 totalMakerAssetAcquiredAmount ) { - bytes4 erc20BridgeProxyId = IAssetData(address(0)).ERC20Bridge.selector; uint256 totalProtocolFeePaid; for (uint256 i = 0; i != orders.length; i++) { @@ -275,32 +272,12 @@ contract MixinExchangeWrapper { uint256 remainingTakerAssetFillAmount = wethSellAmount .safeSub(totalWethSpentAmount); - // If the maker asset is ERC20Bridge, take a snapshot of the Forwarder contract's balance. - bytes4 makerAssetProxyId = orders[i].makerAssetData.readBytes4(0); - address tokenAddress; - uint256 balanceBefore; - if (makerAssetProxyId == erc20BridgeProxyId) { - tokenAddress = orders[i].makerAssetData.readAddress(16); - balanceBefore = IERC20Token(tokenAddress).balanceOf(address(this)); - } - SellFillResults memory sellFillResults = _marketSellSingleOrder( orders[i], signatures[i], remainingTakerAssetFillAmount ); - // Account for the ERC20Bridge transfering more of the maker asset than expected. - if (makerAssetProxyId == erc20BridgeProxyId) { - uint256 balanceAfter = IERC20Token(tokenAddress).balanceOf(address(this)); - sellFillResults.makerAssetAcquiredAmount = LibSafeMath.max256( - balanceAfter.safeSub(balanceBefore), - sellFillResults.makerAssetAcquiredAmount - ); - } - - orders[i].makerAssetData.transferOut(sellFillResults.makerAssetAcquiredAmount); - totalWethSpentAmount = totalWethSpentAmount .safeAdd(sellFillResults.wethSpentAmount); totalMakerAssetAcquiredAmount = totalMakerAssetAcquiredAmount @@ -410,8 +387,6 @@ contract MixinExchangeWrapper { uint256 totalMakerAssetAcquiredAmount ) { - bytes4 erc20BridgeProxyId = IAssetData(address(0)).ERC20Bridge.selector; - uint256 ordersLength = orders.length; for (uint256 i = 0; i != ordersLength; i++) { // Preemptively skip to avoid division by zero in _marketBuySingleOrder @@ -426,7 +401,7 @@ contract MixinExchangeWrapper { bytes4 makerAssetProxyId = orders[i].makerAssetData.readBytes4(0); address tokenAddress; uint256 balanceBefore; - if (makerAssetProxyId == erc20BridgeProxyId) { + if (makerAssetProxyId == ERC20_BRIDGE_PROXY_ID) { tokenAddress = orders[i].makerAssetData.readAddress(16); balanceBefore = IERC20Token(tokenAddress).balanceOf(address(this)); } @@ -441,7 +416,7 @@ contract MixinExchangeWrapper { ); // Account for the ERC20Bridge transfering more of the maker asset than expected. - if (makerAssetProxyId == erc20BridgeProxyId) { + if (makerAssetProxyId == ERC20_BRIDGE_PROXY_ID) { uint256 balanceAfter = IERC20Token(tokenAddress).balanceOf(address(this)); makerAssetAcquiredAmount = LibSafeMath.max256( balanceAfter.safeSub(balanceBefore), diff --git a/contracts/integrations/test/forwarder/forwarder_test.ts b/contracts/integrations/test/forwarder/forwarder_test.ts index b156066a08..a797c34c78 100644 --- a/contracts/integrations/test/forwarder/forwarder_test.ts +++ b/contracts/integrations/test/forwarder/forwarder_test.ts @@ -157,7 +157,7 @@ blockchainTests('Forwarder integration tests', env => { ); }); }); - blockchainTests.resets.only('marketSellOrdersWithEth without extra fees', () => { + blockchainTests.resets('marketSellOrdersWithEth without extra fees', () => { it('should fill a single order without a taker fee', async () => { const orderWithoutFee = await maker.signOrderAsync(); await testFactory.marketSellTestAsync([orderWithoutFee], 0.78); @@ -449,13 +449,12 @@ blockchainTests('Forwarder integration tests', env => { }); }); blockchainTests.resets('marketSellAmountWithEth', () => { - const protocolFee = new BigNumber(150000).times(DeploymentManager.gasPrice); it('should fail if the supplied amount is not sold', async () => { const order = await maker.signOrderAsync(); const ethSellAmount = order.takerAssetAmount; const revertError = new ExchangeForwarderRevertErrors.CompleteSellFailedError( ethSellAmount, - order.takerAssetAmount.times(0.5).plus(protocolFee), + order.takerAssetAmount.times(0.5).plus(DeploymentManager.protocolFee), ); await testFactory.marketSellAmountTestAsync([order], ethSellAmount, 0.5, { revertError,