From fa18f78aeb7f7e4c2bdf19b212581f92b474842d Mon Sep 17 00:00:00 2001 From: James Towle Date: Sun, 7 Jul 2019 01:32:41 -0500 Subject: [PATCH] Adapted tests from matchOrders to test matchOrdersWithMaximalFill --- contracts/exchange/test/match_orders.ts | 256 +++++++++++++++++++++++- 1 file changed, 255 insertions(+), 1 deletion(-) diff --git a/contracts/exchange/test/match_orders.ts b/contracts/exchange/test/match_orders.ts index e7fd19718c..a93405a1d3 100644 --- a/contracts/exchange/test/match_orders.ts +++ b/contracts/exchange/test/match_orders.ts @@ -2187,7 +2187,7 @@ describe('matchOrders', () => { // Taker rightMakerAssetReceivedByTakerAmount: Web3Wrapper.toBaseUnitAmount(7, 0), leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100% - rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(3333, 0), // 3333 rounded down to 3333 + rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(3333, 0), // 3333.3 repeating rounded down to 3333 }; // Match signedOrderLeft with signedOrderRight await matchOrderTester.matchOrdersAndAssertEffectsAsync( @@ -2200,6 +2200,260 @@ describe('matchOrders', () => { true, ); }); + + it('Should give left maker and left taker a favorable fee price when rounding', async () => { + // Create orders to match + const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ + makerAddress: makerAddressLeft, + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(12, 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(97, 0), + feeRecipientAddress: feeRecipientAddressLeft, + makerFee: Web3Wrapper.toBaseUnitAmount(10000, 0), + takerFee: Web3Wrapper.toBaseUnitAmount(10000, 0), + }); + const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ + makerAddress: makerAddressRight, + makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), + takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(89, 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(1, 0), + feeRecipientAddress: feeRecipientAddressRight, + }); + // Note: + // The maker/taker fee percentage paid on the left order differs because + // they received different sale prices. The left maker pays a + // fee slightly lower than the left taker. + const expectedTransferAmounts = { + // Left Maker + leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(11, 0), + leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(9166, 0), // 9166.6 rounded down to 9166 + // Right Maker + rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(89, 0), + leftMakerAssetBoughtByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(1, 0), + rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100% + // Taker + leftMakerAssetReceivedByTakerAmount: Web3Wrapper.toBaseUnitAmount(10, 0), + leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(9175, 0), // 9175.2 rounded down to 9175 + rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100% + }; + // Match signedOrderLeft with signedOrderRight + await matchOrderTester.matchOrdersAndAssertEffectsAsync( + { + leftOrder: signedOrderLeft, + rightOrder: signedOrderRight, + }, + takerAddress, + expectedTransferAmounts, + true, + ); + }); + + it('Should give left maker a better sell price when rounding', async () => { + // Create orders to match + const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ + makerAddress: makerAddressLeft, + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(12, 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(97, 0), + feeRecipientAddress: feeRecipientAddressLeft, + }); + const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ + makerAddress: makerAddressRight, + makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), + takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(89, 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(1, 0), + feeRecipientAddress: feeRecipientAddressRight, + }); + // Note: + // The maker/taker fee percentage paid on the left order differs because + // they received different sale prices. The left maker pays a fee + // slightly lower than the left taker. + const expectedTransferAmounts = { + // Left Maker + leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(11, 0), + leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount( + new BigNumber('91.6666666666666666'), + 16, + ), // 91.6% + // Right Maker + rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(89, 0), + leftMakerAssetBoughtByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(1, 0), + rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100% + // Taker + leftMakerAssetReceivedByTakerAmount: Web3Wrapper.toBaseUnitAmount(10, 0), + leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount( + new BigNumber('91.7525773195876288'), + 16, + ), // 91.75% + rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100% + }; + // Match signedOrderLeft with signedOrderRight + await matchOrderTester.matchOrdersAndAssertEffectsAsync( + { + leftOrder: signedOrderLeft, + rightOrder: signedOrderRight, + }, + takerAddress, + expectedTransferAmounts, + true, + ); + }); + + it('should transfer the correct amounts when consecutive calls are used to completely fill the left order', async () => { + // Create orders to match + const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(50, 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(100, 18), + }); + const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(10, 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(2, 18), + }); + // Match orders + const expectedTransferAmounts = { + // Left Maker + leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(5, 18), + leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(10, 16), // 10% + // Right Maker + rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(10, 18), + leftMakerAssetBoughtByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(2, 18), + rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100% + // Taker + leftMakerAssetReceivedByTakerAmount: Web3Wrapper.toBaseUnitAmount(3, 18), + leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(10, 16), // 10% + rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100% + }; + // prettier-ignore + const matchResults = await matchOrderTester.matchOrdersAndAssertEffectsAsync( + { + leftOrder: signedOrderLeft, + rightOrder: signedOrderRight, + }, + takerAddress, + expectedTransferAmounts, + true, + ); + // Construct second right order + // Note: This order needs makerAssetAmount=90/takerAssetAmount=[anything <= 45] to fully fill the right order. + // However, we use 100/50 to ensure a partial fill as we want to go down the "left fill" + // branch in the contract twice for this test. + const signedOrderRight2 = await orderFactoryRight.newSignedOrderAsync({ + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(100, 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(50, 18), + }); + // Match signedOrderLeft with signedOrderRight2 + const expectedTransferAmounts2 = { + // Left Maker + leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(45, 18), + leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(90, 16), // 90% (10% paid earlier) + // Right Maker + rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(90, 18), + rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(90, 16), // 90% + // Taker + leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(90, 16), // 90% (10% paid earlier) + rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(90, 16), // 90% + }; + + await matchOrderTester.matchOrdersAndAssertEffectsAsync( + { + leftOrder: signedOrderLeft, + rightOrder: signedOrderRight2, + leftOrderTakerAssetFilledAmount: matchResults.orders.leftOrderTakerAssetFilledAmount, + }, + takerAddress, + expectedTransferAmounts2, + true, + await matchOrderTester.getBalancesAsync(), + ); + }); + + it('Should transfer correct amounts when right order fill amount deviates from amount derived by `Exchange.fillOrder`', async () => { + // Create orders to match + const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ + makerAddress: makerAddressLeft, + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(1000, 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(1005, 0), + feeRecipientAddress: feeRecipientAddressLeft, + }); + const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ + makerAddress: makerAddressRight, + makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), + takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(2126, 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(1063, 0), + feeRecipientAddress: feeRecipientAddressRight, + }); + const expectedTransferAmounts = { + // Left Maker + leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(1000, 0), + rightMakerAssetBoughtByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(1005, 0), + leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100% + // Right Maker + // Notes: + // i. + // The left order is fully filled by the right order, so the right maker must sell 1005 units of their asset to the left maker. + // By selling 1005 units, the right maker should theoretically receive 502.5 units of the left maker's asset. + // Since the transfer amount must be an integer, this value must be rounded down to 502 or up to 503. + // ii. + // If the right order were filled via `Exchange.fillOrder` the respective fill amounts would be [1004, 502] or [1006, 503]. + // It follows that we cannot trigger a sale of 1005 units of the right maker's asset through `Exchange.fillOrder`. + // iii. + // For an optimal match, the algorithm must choose either [1005, 502] or [1005, 503] as fill amounts for the right order. + // The algorithm favors the right maker when the exchange rate must be rounded, so the final fill for the right order is [1005, 503]. + // iv. + // The right maker fee differs from the right taker fee because their exchange rate differs. + // The right maker always receives the better exchange and fee price. + rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(2000, 0), + rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount( + new BigNumber('94.0733772342427093'), + 16, + ), // 94.07% + // Taker + rightMakerAssetReceivedByTakerAmount: Web3Wrapper.toBaseUnitAmount(995, 0), + leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100% + rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount( + new BigNumber('94.0733772342427093'), + 16, + ), // 94.07% + }; + // Match signedOrderLeft with signedOrderRight + await matchOrderTester.matchOrdersAndAssertEffectsAsync( + { + leftOrder: signedOrderLeft, + rightOrder: signedOrderRight, + }, + takerAddress, + expectedTransferAmounts, + true, + ); + }); + + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow matchOrdersWithMaximalFill to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(5, 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(10, 18), + }); + const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ + makerAddress: makerAddressRight, + takerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(10, 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(2, 18), + feeRecipientAddress: feeRecipientAddressRight, + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + const tx = exchangeWrapper.matchOrdersWithMaximalFillAsync(signedOrderLeft, signedOrderRight, takerAddress); + return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal); + }); + }); + }; + describe('matchOrdersWithMaximalFill reentrancy tests', () => reentrancyTest(exchangeConstants.FUNCTIONS_WITH_MUTEX)); }); describe('batchMatchOrders and batchMatchOrdersWithMaximalFill rich errors', async () => {