diff options
Diffstat (limited to 'packages/contracts')
38 files changed, 2888 insertions, 775 deletions
diff --git a/packages/contracts/compiler.json b/packages/contracts/compiler.json index 16524231c..f66114e87 100644 --- a/packages/contracts/compiler.json +++ b/packages/contracts/compiler.json @@ -40,6 +40,7 @@ "MultiSigWallet", "MultiSigWalletWithTimeLock", "OrderValidator", + "ReentrantERC20Token", "TestAssetProxyOwner", "TestAssetProxyDispatcher", "TestConstants", @@ -47,6 +48,7 @@ "TestLibs", "TestExchangeInternals", "TestSignatureValidator", + "TestStaticCallReceiver", "TokenRegistry", "Validator", "Wallet", diff --git a/packages/contracts/package.json b/packages/contracts/package.json index 10ab09767..5d2f290ac 100644 --- a/packages/contracts/package.json +++ b/packages/contracts/package.json @@ -1,7 +1,7 @@ { "private": true, "name": "contracts", - "version": "2.1.41", + "version": "2.1.42", "engines": { "node": ">=6.12" }, @@ -33,7 +33,7 @@ "lint-contracts": "solhint src/2.0.0/**/**/**/**/*.sol" }, "config": { - "abis": "../migrations/artifacts/2.0.0/@(AssetProxyOwner|DummyERC20Token|DummyERC721Receiver|DummyERC721Token|DummyNoReturnERC20Token|ERC20Proxy|ERC721Proxy|Forwarder|Exchange|ExchangeWrapper|IAssetData|IAssetProxy|InvalidERC721Receiver|MixinAuthorizable|MultiSigWallet|MultiSigWalletWithTimeLock|OrderValidator|TestAssetProxyOwner|TestAssetProxyDispatcher|TestConstants|TestExchangeInternals|TestLibBytes|TestLibs|TestSignatureValidator|Validator|Wallet|TokenRegistry|Whitelist|WETH9|ZRXToken).json" + "abis": "../migrations/artifacts/2.0.0/@(AssetProxyOwner|DummyERC20Token|DummyERC721Receiver|DummyERC721Token|DummyNoReturnERC20Token|ERC20Proxy|ERC721Proxy|Forwarder|Exchange|ExchangeWrapper|IAssetData|IAssetProxy|InvalidERC721Receiver|MixinAuthorizable|MultiSigWallet|MultiSigWalletWithTimeLock|OrderValidator|ReentrantERC20Token|TestAssetProxyOwner|TestAssetProxyDispatcher|TestConstants|TestExchangeInternals|TestLibBytes|TestLibs|TestSignatureValidator|TestStaticCallReceiver|Validator|Wallet|TokenRegistry|Whitelist|WETH9|ZRXToken).json" }, "repository": { "type": "git", @@ -46,11 +46,11 @@ }, "homepage": "https://github.com/0xProject/0x-monorepo/packages/contracts/README.md", "devDependencies": { - "@0xproject/abi-gen": "^1.0.6", - "@0xproject/dev-utils": "^1.0.5", - "@0xproject/sol-compiler": "^1.1.0", - "@0xproject/sol-cov": "^2.1.0", - "@0xproject/subproviders": "^2.0.0", + "@0xproject/abi-gen": "^1.0.7", + "@0xproject/dev-utils": "^1.0.6", + "@0xproject/sol-compiler": "^1.1.1", + "@0xproject/sol-cov": "^2.1.1", + "@0xproject/subproviders": "^2.0.1", "@0xproject/tslint-config": "^1.0.6", "@types/bn.js": "^4.11.0", "@types/ethereumjs-abi": "^0.6.0", @@ -73,12 +73,12 @@ "yargs": "^10.0.3" }, "dependencies": { - "@0xproject/base-contract": "^2.0.0", - "@0xproject/order-utils": "^1.0.1-rc.4", - "@0xproject/types": "^1.0.1-rc.5", + "@0xproject/base-contract": "^2.0.1", + "@0xproject/order-utils": "^1.0.1-rc.6", + "@0xproject/types": "^1.0.1-rc.6", "@0xproject/typescript-typings": "^1.0.5", - "@0xproject/utils": "^1.0.6", - "@0xproject/web3-wrapper": "^2.0.0", + "@0xproject/utils": "^1.0.7", + "@0xproject/web3-wrapper": "^2.0.1", "@types/js-combinatorics": "^0.5.29", "bn.js": "^4.11.8", "ethereum-types": "^1.0.5", diff --git a/packages/contracts/src/2.0.0/examples/Whitelist/Whitelist.sol b/packages/contracts/src/2.0.0/examples/Whitelist/Whitelist.sol index 60cac26ea..e4e25038c 100644 --- a/packages/contracts/src/2.0.0/examples/Whitelist/Whitelist.sol +++ b/packages/contracts/src/2.0.0/examples/Whitelist/Whitelist.sol @@ -37,7 +37,7 @@ contract Whitelist is bytes internal TX_ORIGIN_SIGNATURE; // solhint-enable var-name-mixedcase - byte constant internal VALIDATOR_SIGNATURE_BYTE = "\x06"; + byte constant internal VALIDATOR_SIGNATURE_BYTE = "\x05"; constructor (address _exchange) public diff --git a/packages/contracts/src/2.0.0/extensions/Forwarder/MixinExchangeWrapper.sol b/packages/contracts/src/2.0.0/extensions/Forwarder/MixinExchangeWrapper.sol index 218713d3c..9e816716c 100644 --- a/packages/contracts/src/2.0.0/extensions/Forwarder/MixinExchangeWrapper.sol +++ b/packages/contracts/src/2.0.0/extensions/Forwarder/MixinExchangeWrapper.sol @@ -69,20 +69,14 @@ contract MixinExchangeWrapper is fillOrderCalldata, // write output over input 128 // output size is 128 bytes ) - switch success - case 0 { - mstore(fillResults, 0) - mstore(add(fillResults, 32), 0) - mstore(add(fillResults, 64), 0) - mstore(add(fillResults, 96), 0) - } - case 1 { + if success { mstore(fillResults, mload(fillOrderCalldata)) mstore(add(fillResults, 32), mload(add(fillOrderCalldata, 32))) mstore(add(fillResults, 64), mload(add(fillOrderCalldata, 64))) mstore(add(fillResults, 96), mload(add(fillOrderCalldata, 96))) } } + // fillResults values will be 0 by default if call was unsuccessful return fillResults; } @@ -163,7 +157,7 @@ contract MixinExchangeWrapper is // 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 - uint256 remainingTakerAssetFillAmount = getPartialAmount( + uint256 remainingTakerAssetFillAmount = getPartialAmountFloor( orders[i].takerAssetAmount, orders[i].makerAssetAmount, remainingMakerAssetFillAmount @@ -231,7 +225,7 @@ contract MixinExchangeWrapper is // 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. - uint256 remainingWethSellAmount = getPartialAmount( + uint256 remainingWethSellAmount = getPartialAmountFloor( orders[i].takerAssetAmount, safeSub(orders[i].makerAssetAmount, orders[i].takerFee), // our exchange rate after fees remainingZrxBuyAmount diff --git a/packages/contracts/src/2.0.0/extensions/Forwarder/MixinForwarderCore.sol b/packages/contracts/src/2.0.0/extensions/Forwarder/MixinForwarderCore.sol index 42cec4d36..14f191879 100644 --- a/packages/contracts/src/2.0.0/extensions/Forwarder/MixinForwarderCore.sol +++ b/packages/contracts/src/2.0.0/extensions/Forwarder/MixinForwarderCore.sol @@ -87,7 +87,7 @@ contract MixinForwarderCore is uint256 makerAssetAmountPurchased; if (orders[0].makerAssetData.equals(ZRX_ASSET_DATA)) { // Calculate amount of WETH that won't be spent on ETH fees. - wethSellAmount = getPartialAmount( + wethSellAmount = getPartialAmountFloor( PERCENTAGE_DENOMINATOR, safeAdd(PERCENTAGE_DENOMINATOR, feePercentage), msg.value @@ -103,7 +103,7 @@ contract MixinForwarderCore is makerAssetAmountPurchased = safeSub(orderFillResults.makerAssetFilledAmount, orderFillResults.takerFeePaid); } else { // 5% of WETH is reserved for filling feeOrders and paying feeRecipient. - wethSellAmount = getPartialAmount( + wethSellAmount = getPartialAmountFloor( MAX_WETH_FILL_PERCENTAGE, PERCENTAGE_DENOMINATOR, msg.value diff --git a/packages/contracts/src/2.0.0/extensions/Forwarder/MixinWeth.sol b/packages/contracts/src/2.0.0/extensions/Forwarder/MixinWeth.sol index 93e85e599..5863b522d 100644 --- a/packages/contracts/src/2.0.0/extensions/Forwarder/MixinWeth.sol +++ b/packages/contracts/src/2.0.0/extensions/Forwarder/MixinWeth.sol @@ -82,7 +82,7 @@ contract MixinWeth is uint256 wethRemaining = safeSub(msg.value, wethSold); // Calculate ETH fee to pay to feeRecipient. - uint256 ethFee = getPartialAmount( + uint256 ethFee = getPartialAmountFloor( feePercentage, PERCENTAGE_DENOMINATOR, wethSoldExcludingFeeOrders diff --git a/packages/contracts/src/2.0.0/protocol/AssetProxy/ERC20Proxy.sol b/packages/contracts/src/2.0.0/protocol/AssetProxy/ERC20Proxy.sol index b5cec6b64..004c3892d 100644 --- a/packages/contracts/src/2.0.0/protocol/AssetProxy/ERC20Proxy.sol +++ b/packages/contracts/src/2.0.0/protocol/AssetProxy/ERC20Proxy.sol @@ -118,6 +118,9 @@ contract ERC20Proxy is mstore(96, 0) revert(0, 100) } + + // Revert if undefined function is called + revert(0, 0) } } diff --git a/packages/contracts/src/2.0.0/protocol/AssetProxy/ERC721Proxy.sol b/packages/contracts/src/2.0.0/protocol/AssetProxy/ERC721Proxy.sol index 6a70c9f60..9d0bc0f74 100644 --- a/packages/contracts/src/2.0.0/protocol/AssetProxy/ERC721Proxy.sol +++ b/packages/contracts/src/2.0.0/protocol/AssetProxy/ERC721Proxy.sol @@ -152,6 +152,9 @@ contract ERC721Proxy is mstore(96, 0) revert(0, 100) } + + // Revert if undefined function is called + revert(0, 0) } } diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinAssetProxyDispatcher.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinAssetProxyDispatcher.sol index e9f882194..80475e6e3 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinAssetProxyDispatcher.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinAssetProxyDispatcher.sol @@ -83,7 +83,7 @@ contract MixinAssetProxyDispatcher is internal { // Do nothing if no amount should be transferred. - if (amount > 0) { + if (amount > 0 && from != to) { // Ensure assetData length is valid require( assetData.length > 3, diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol index ab5c6e507..ff908917d 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol @@ -19,6 +19,7 @@ pragma solidity 0.4.24; pragma experimental ABIEncoderV2; +import "../../utils/ReentrancyGuard/ReentrancyGuard.sol"; import "./libs/LibConstants.sol"; import "./libs/LibFillResults.sol"; import "./libs/LibOrder.sol"; @@ -30,6 +31,7 @@ import "./mixins/MAssetProxyDispatcher.sol"; contract MixinExchangeCore is + ReentrancyGuard, LibConstants, LibMath, LibOrder, @@ -54,6 +56,7 @@ contract MixinExchangeCore is /// @param targetOrderEpoch Orders created with a salt less or equal to this value will be cancelled. function cancelOrdersUpTo(uint256 targetOrderEpoch) external + nonReentrant { address makerAddress = getCurrentContextAddress(); // If this function is called via `executeTransaction`, we only update the orderEpoch for the makerAddress/msg.sender combination. @@ -86,43 +89,14 @@ contract MixinExchangeCore is bytes memory signature ) public + nonReentrant returns (FillResults memory fillResults) { - // Fetch order info - OrderInfo memory orderInfo = getOrderInfo(order); - - // Fetch taker address - address takerAddress = getCurrentContextAddress(); - - // Get amount of takerAsset to fill - uint256 remainingTakerAssetAmount = safeSub(order.takerAssetAmount, orderInfo.orderTakerAssetFilledAmount); - uint256 takerAssetFilledAmount = min256(takerAssetFillAmount, remainingTakerAssetAmount); - - // Validate context - assertValidFill( + fillResults = fillOrderInternal( order, - orderInfo, - takerAddress, takerAssetFillAmount, - takerAssetFilledAmount, signature ); - - // Compute proportional fill amounts - fillResults = calculateFillResults(order, takerAssetFilledAmount); - - // Update exchange internal state - updateFilledState( - order, - takerAddress, - orderInfo.orderHash, - orderInfo.orderTakerAssetFilledAmount, - fillResults - ); - - // Settle order - settleOrder(order, takerAddress, fillResults); - return fillResults; } @@ -131,6 +105,7 @@ contract MixinExchangeCore is /// @param order Order to cancel. Order must be OrderStatus.FILLABLE. function cancelOrder(Order memory order) public + nonReentrant { // Fetch current order status OrderInfo memory orderInfo = getOrderInfo(order); @@ -203,6 +178,64 @@ contract MixinExchangeCore is return orderInfo; } + /// @dev Fills the input order. + /// @param order Order struct containing order specifications. + /// @param takerAssetFillAmount Desired amount of takerAsset to sell. + /// @param signature Proof that order has been created by maker. + /// @return Amounts filled and fees paid by maker and taker. + function fillOrderInternal( + Order memory order, + uint256 takerAssetFillAmount, + bytes memory signature + ) + internal + returns (FillResults memory fillResults) + { + // Fetch order info + OrderInfo memory orderInfo = getOrderInfo(order); + + // Fetch taker address + address takerAddress = getCurrentContextAddress(); + + // Assert that the order is fillable by taker + assertFillableOrder( + order, + orderInfo, + takerAddress, + signature + ); + + // Get amount of takerAsset to fill + uint256 remainingTakerAssetAmount = safeSub(order.takerAssetAmount, orderInfo.orderTakerAssetFilledAmount); + uint256 takerAssetFilledAmount = min256(takerAssetFillAmount, remainingTakerAssetAmount); + + // Validate context + assertValidFill( + order, + orderInfo, + takerAssetFillAmount, + takerAssetFilledAmount, + fillResults.makerAssetFilledAmount + ); + + // Compute proportional fill amounts + fillResults = calculateFillResults(order, takerAssetFilledAmount); + + // Update exchange internal state + updateFilledState( + order, + takerAddress, + orderInfo.orderHash, + orderInfo.orderTakerAssetFilledAmount, + fillResults + ); + + // Settle order + settleOrder(order, takerAddress, fillResults); + + return fillResults; + } + /// @dev Updates state with results of a fill order. /// @param order that was filled. /// @param takerAddress Address of taker who filled the order. @@ -259,20 +292,16 @@ contract MixinExchangeCore is order.takerAssetData ); } - + /// @dev Validates context for fillOrder. Succeeds or throws. /// @param order to be filled. /// @param orderInfo OrderStatus, orderHash, and amount already filled of order. /// @param takerAddress Address of order taker. - /// @param takerAssetFillAmount Desired amount of order to fill by taker. - /// @param takerAssetFilledAmount Amount of takerAsset that will be filled. /// @param signature Proof that the orders was created by its maker. - function assertValidFill( + function assertFillableOrder( Order memory order, OrderInfo memory orderInfo, address takerAddress, - uint256 takerAssetFillAmount, - uint256 takerAssetFilledAmount, bytes memory signature ) internal @@ -283,13 +312,7 @@ contract MixinExchangeCore is orderInfo.orderStatus == uint8(OrderStatus.FILLABLE), "ORDER_UNFILLABLE" ); - - // Revert if fill amount is invalid - require( - takerAssetFillAmount != 0, - "INVALID_TAKER_AMOUNT" - ); - + // Validate sender is allowed to fill this order if (order.senderAddress != address(0)) { require( @@ -297,7 +320,7 @@ contract MixinExchangeCore is "INVALID_SENDER" ); } - + // Validate taker is allowed to fill this order if (order.takerAddress != address(0)) { require( @@ -305,7 +328,7 @@ contract MixinExchangeCore is "INVALID_TAKER" ); } - + // Validate Maker signature (check only if first time seen) if (orderInfo.orderTakerAssetFilledAmount == 0) { require( @@ -317,15 +340,69 @@ contract MixinExchangeCore is "INVALID_ORDER_SIGNATURE" ); } - - // Validate fill order rounding + } + + /// @dev Validates context for fillOrder. Succeeds or throws. + /// @param order to be filled. + /// @param orderInfo OrderStatus, orderHash, and amount already filled of order. + /// @param takerAssetFillAmount Desired amount of order to fill by taker. + /// @param takerAssetFilledAmount Amount of takerAsset that will be filled. + /// @param makerAssetFilledAmount Amount of makerAsset that will be transfered. + function assertValidFill( + Order memory order, + OrderInfo memory orderInfo, + uint256 takerAssetFillAmount, // TODO: use FillResults + uint256 takerAssetFilledAmount, + uint256 makerAssetFilledAmount + ) + internal + view + { + // Revert if fill amount is invalid + // TODO: reconsider necessity for v2.1 + require( + takerAssetFillAmount != 0, + "INVALID_TAKER_AMOUNT" + ); + + // Make sure taker does not pay more than desired amount + // NOTE: This assertion should never fail, it is here + // as an extra defence against potential bugs. + require( + takerAssetFilledAmount <= takerAssetFillAmount, + "TAKER_OVERPAY" + ); + + // Make sure order is not overfilled + // NOTE: This assertion should never fail, it is here + // as an extra defence against potential bugs. + require( + safeAdd(orderInfo.orderTakerAssetFilledAmount, takerAssetFilledAmount) <= order.takerAssetAmount, + "ORDER_OVERFILL" + ); + + // Make sure order is filled at acceptable price. + // The order has an implied price from the makers perspective: + // order price = order.makerAssetAmount / order.takerAssetAmount + // i.e. the number of makerAsset maker is paying per takerAsset. The + // maker is guaranteed to get this price or a better (lower) one. The + // actual price maker is getting in this fill is: + // fill price = makerAssetFilledAmount / takerAssetFilledAmount + // We need `fill price <= order price` for the fill to be fair to maker. + // This amounts to: + // makerAssetFilledAmount order.makerAssetAmount + // ------------------------ <= ----------------------- + // takerAssetFilledAmount order.takerAssetAmount + // or, equivalently: + // makerAssetFilledAmount * order.takerAssetAmount <= + // order.makerAssetAmount * takerAssetFilledAmount + // NOTE: This assertion should never fail, it is here + // as an extra defence against potential bugs. require( - !isRoundingError( - takerAssetFilledAmount, - order.takerAssetAmount, - order.makerAssetAmount - ), - "ROUNDING_ERROR" + safeMul(makerAssetFilledAmount, order.takerAssetAmount) + <= + safeMul(order.makerAssetAmount, takerAssetFilledAmount), + "INVALID_FILL_PRICE" ); } @@ -376,17 +453,17 @@ contract MixinExchangeCore is { // Compute proportional transfer amounts fillResults.takerAssetFilledAmount = takerAssetFilledAmount; - fillResults.makerAssetFilledAmount = getPartialAmount( + fillResults.makerAssetFilledAmount = safeGetPartialAmountFloor( takerAssetFilledAmount, order.takerAssetAmount, order.makerAssetAmount ); - fillResults.makerFeePaid = getPartialAmount( - takerAssetFilledAmount, - order.takerAssetAmount, + fillResults.makerFeePaid = safeGetPartialAmountFloor( + fillResults.makerAssetFilledAmount, + order.makerAssetAmount, order.makerFee ); - fillResults.takerFeePaid = getPartialAmount( + fillResults.takerFeePaid = safeGetPartialAmountFloor( takerAssetFilledAmount, order.takerAssetAmount, order.takerFee diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol index 56b309a1b..b4f6bdb26 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol @@ -14,6 +14,7 @@ pragma solidity 0.4.24; pragma experimental ABIEncoderV2; +import "../../utils/ReentrancyGuard/ReentrancyGuard.sol"; import "./libs/LibConstants.sol"; import "./libs/LibMath.sol"; import "./libs/LibOrder.sol"; @@ -25,6 +26,7 @@ import "./mixins/MAssetProxyDispatcher.sol"; contract MixinMatchOrders is + ReentrancyGuard, LibConstants, LibMath, MAssetProxyDispatcher, @@ -48,6 +50,7 @@ contract MixinMatchOrders is bytes memory rightSignature ) public + nonReentrant returns (LibFillResults.MatchedFillResults memory matchedFillResults) { // We assume that rightOrder.takerAssetData == leftOrder.makerAssetData and rightOrder.makerAssetData == leftOrder.takerAssetData. @@ -61,8 +64,20 @@ contract MixinMatchOrders is // Fetch taker address address takerAddress = getCurrentContextAddress(); - + // Either our context is valid or we revert + assertFillableOrder( + leftOrder, + leftOrderInfo, + takerAddress, + leftSignature + ); + assertFillableOrder( + rightOrder, + rightOrderInfo, + takerAddress, + rightSignature + ); assertValidMatch(leftOrder, rightOrder); // Compute proportional fill amounts @@ -77,20 +92,18 @@ contract MixinMatchOrders is assertValidFill( leftOrder, leftOrderInfo, - takerAddress, matchedFillResults.left.takerAssetFilledAmount, matchedFillResults.left.takerAssetFilledAmount, - leftSignature + matchedFillResults.left.makerAssetFilledAmount ); assertValidFill( rightOrder, rightOrderInfo, - takerAddress, matchedFillResults.right.takerAssetFilledAmount, matchedFillResults.right.takerAssetFilledAmount, - rightSignature + matchedFillResults.right.makerAssetFilledAmount ); - + // Update exchange state updateFilledState( leftOrder, @@ -106,7 +119,7 @@ contract MixinMatchOrders is rightOrderInfo.orderTakerAssetFilledAmount, matchedFillResults.right ); - + // Settle matched orders. Succeeds or throws. settleMatchedOrders( leftOrder, @@ -162,62 +175,85 @@ contract MixinMatchOrders is pure returns (LibFillResults.MatchedFillResults memory matchedFillResults) { - // We settle orders at the exchange rate of the right order. - // The amount saved by the left maker goes to the taker. - // Either the left or right order will be fully filled; possibly both. - // The left order is fully filled iff the right order can sell more than left can buy. - // That is: the amount required to fill the left order is less than or equal to - // the amount we can spend from the right order: - // <leftTakerAssetAmountRemaining> <= <rightTakerAssetAmountRemaining> * <rightMakerToTakerRatio> - // <leftTakerAssetAmountRemaining> <= <rightTakerAssetAmountRemaining> * <rightOrder.makerAssetAmount> / <rightOrder.takerAssetAmount> - // <leftTakerAssetAmountRemaining> * <rightOrder.takerAssetAmount> <= <rightTakerAssetAmountRemaining> * <rightOrder.makerAssetAmount> + // Derive maker asset amounts for left & right orders, given store taker assert amounts uint256 leftTakerAssetAmountRemaining = safeSub(leftOrder.takerAssetAmount, leftOrderTakerAssetFilledAmount); + uint256 leftMakerAssetAmountRemaining = safeGetPartialAmountFloor( + leftOrder.makerAssetAmount, + leftOrder.takerAssetAmount, + leftTakerAssetAmountRemaining + ); uint256 rightTakerAssetAmountRemaining = safeSub(rightOrder.takerAssetAmount, rightOrderTakerAssetFilledAmount); - uint256 leftTakerAssetFilledAmount; - uint256 rightTakerAssetFilledAmount; - if ( - safeMul(leftTakerAssetAmountRemaining, rightOrder.takerAssetAmount) <= - safeMul(rightTakerAssetAmountRemaining, rightOrder.makerAssetAmount) - ) { - // Left order will be fully filled: maximally fill left - leftTakerAssetFilledAmount = leftTakerAssetAmountRemaining; + uint256 rightMakerAssetAmountRemaining = safeGetPartialAmountFloor( + rightOrder.makerAssetAmount, + rightOrder.takerAssetAmount, + rightTakerAssetAmountRemaining + ); - // The right order receives an amount proportional to how much was spent. - rightTakerAssetFilledAmount = getPartialAmount( - rightOrder.takerAssetAmount, - rightOrder.makerAssetAmount, - leftTakerAssetFilledAmount + // Calculate fill results for maker and taker assets: at least one order will be fully filled. + // The maximum amount the left maker can buy is `leftTakerAssetAmountRemaining` + // The maximum amount the right maker can sell is `rightMakerAssetAmountRemaining` + // We have two distinct cases for calculating the fill results: + // Case 1. + // If the left maker can buy more than the right maker can sell, then only the right order is fully filled. + // If the left maker can buy exactly what the right maker can sell, then both orders are fully filled. + // Case 2. + // If the left maker cannot buy more than the right maker can sell, then only the left order is fully filled. + if (leftTakerAssetAmountRemaining >= rightMakerAssetAmountRemaining) { + // Case 1: Right order is fully filled + matchedFillResults.right.makerAssetFilledAmount = rightMakerAssetAmountRemaining; + matchedFillResults.right.takerAssetFilledAmount = rightTakerAssetAmountRemaining; + matchedFillResults.left.takerAssetFilledAmount = matchedFillResults.right.makerAssetFilledAmount; + // Round down to ensure the maker's exchange rate does not exceed the price specified by the order. + // We favor the maker when the exchange rate must be rounded. + matchedFillResults.left.makerAssetFilledAmount = safeGetPartialAmountFloor( + leftOrder.makerAssetAmount, + leftOrder.takerAssetAmount, + matchedFillResults.left.takerAssetFilledAmount ); } else { - // Right order will be fully filled: maximally fill right - rightTakerAssetFilledAmount = rightTakerAssetAmountRemaining; - - // The left order receives an amount proportional to how much was spent. - leftTakerAssetFilledAmount = getPartialAmount( - rightOrder.makerAssetAmount, + // Case 2: Left order is fully filled + matchedFillResults.left.makerAssetFilledAmount = leftMakerAssetAmountRemaining; + matchedFillResults.left.takerAssetFilledAmount = leftTakerAssetAmountRemaining; + matchedFillResults.right.makerAssetFilledAmount = matchedFillResults.left.takerAssetFilledAmount; + // Round up to ensure the maker's exchange rate does not exceed the price specified by the order. + // We favor the maker when the exchange rate must be rounded. + matchedFillResults.right.takerAssetFilledAmount = safeGetPartialAmountCeil( rightOrder.takerAssetAmount, - rightTakerAssetFilledAmount + rightOrder.makerAssetAmount, + matchedFillResults.right.makerAssetFilledAmount ); } - // Calculate fill results for left order - matchedFillResults.left = calculateFillResults( - leftOrder, - leftTakerAssetFilledAmount - ); - - // Calculate fill results for right order - matchedFillResults.right = calculateFillResults( - rightOrder, - rightTakerAssetFilledAmount - ); - // Calculate amount given to taker matchedFillResults.leftMakerAssetSpreadAmount = safeSub( matchedFillResults.left.makerAssetFilledAmount, matchedFillResults.right.takerAssetFilledAmount ); + // Compute fees for left order + matchedFillResults.left.makerFeePaid = safeGetPartialAmountFloor( + matchedFillResults.left.makerAssetFilledAmount, + leftOrder.makerAssetAmount, + leftOrder.makerFee + ); + matchedFillResults.left.takerFeePaid = safeGetPartialAmountFloor( + matchedFillResults.left.takerAssetFilledAmount, + leftOrder.takerAssetAmount, + leftOrder.takerFee + ); + + // Compute fees for right order + matchedFillResults.right.makerFeePaid = safeGetPartialAmountFloor( + matchedFillResults.right.makerAssetFilledAmount, + rightOrder.makerAssetAmount, + rightOrder.makerFee + ); + matchedFillResults.right.takerFeePaid = safeGetPartialAmountFloor( + matchedFillResults.right.takerAssetFilledAmount, + rightOrder.takerAssetAmount, + rightOrder.takerFee + ); + // Return fill results return matchedFillResults; } diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol index 44de54817..4eb6a2fa6 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol @@ -19,6 +19,7 @@ pragma solidity 0.4.24; import "../../utils/LibBytes/LibBytes.sol"; +import "../../utils/ReentrancyGuard/ReentrancyGuard.sol"; import "./mixins/MSignatureValidator.sol"; import "./mixins/MTransactions.sol"; import "./interfaces/IWallet.sol"; @@ -26,6 +27,7 @@ import "./interfaces/IValidator.sol"; contract MixinSignatureValidator is + ReentrancyGuard, MSignatureValidator, MTransactions { @@ -48,14 +50,16 @@ contract MixinSignatureValidator is ) external { - require( - isValidSignature( - hash, - signerAddress, - signature - ), - "INVALID_SIGNATURE" - ); + if (signerAddress != msg.sender) { + require( + isValidSignature( + hash, + signerAddress, + signature + ), + "INVALID_SIGNATURE" + ); + } preSigned[hash][signerAddress] = true; } @@ -67,6 +71,7 @@ contract MixinSignatureValidator is bool approval ) external + nonReentrant { address signerAddress = getCurrentContextAddress(); allowedValidators[signerAddress][validatorAddress] = approval; @@ -172,26 +177,14 @@ contract MixinSignatureValidator is isValid = signerAddress == recovered; return isValid; - // Implicitly signed by caller. - // The signer has initiated the call. In the case of non-contract - // accounts it means the transaction itself was signed. - // Example: let's say for a particular operation three signatures - // A, B and C are required. To submit the transaction, A and B can - // give a signature to C, who can then submit the transaction using - // `Caller` for his own signature. Or A and C can sign and B can - // submit using `Caller`. Having `Caller` allows this flexibility. - } else if (signatureType == SignatureType.Caller) { - require( - signature.length == 0, - "LENGTH_0_REQUIRED" - ); - isValid = signerAddress == msg.sender; - return isValid; - // Signature verified by wallet contract. // If used with an order, the maker of the order is the wallet contract. } else if (signatureType == SignatureType.Wallet) { - isValid = IWallet(signerAddress).isValidSignature(hash, signature); + isValid = isValidWalletSignature( + hash, + signerAddress, + signature + ); return isValid; // Signature verified by validator contract. @@ -209,7 +202,8 @@ contract MixinSignatureValidator is if (!allowedValidators[signerAddress][validatorAddress]) { return false; } - isValid = IValidator(validatorAddress).isValidSignature( + isValid = isValidValidatorSignature( + validatorAddress, hash, signerAddress, signature @@ -220,34 +214,6 @@ contract MixinSignatureValidator is } else if (signatureType == SignatureType.PreSigned) { isValid = preSigned[hash][signerAddress]; return isValid; - - // Signature from Trezor hardware wallet. - // It differs from web3.eth_sign in the encoding of message length - // (Bitcoin varint encoding vs ascii-decimal, the latter is not - // self-terminating which leads to ambiguities). - // See also: - // https://en.bitcoin.it/wiki/Protocol_documentation#Variable_length_integer - // https://github.com/trezor/trezor-mcu/blob/master/firmware/ethereum.c#L602 - // https://github.com/trezor/trezor-mcu/blob/master/firmware/crypto.c#L36 - } else if (signatureType == SignatureType.Trezor) { - require( - signature.length == 65, - "LENGTH_65_REQUIRED" - ); - v = uint8(signature[0]); - r = signature.readBytes32(1); - s = signature.readBytes32(33); - recovered = ecrecover( - keccak256(abi.encodePacked( - "\x19Ethereum Signed Message:\n\x20", - hash - )), - v, - r, - s - ); - isValid = signerAddress == recovered; - return isValid; } // Anything else is illegal (We do not return false because @@ -257,4 +223,102 @@ contract MixinSignatureValidator is // signature was invalid.) revert("SIGNATURE_UNSUPPORTED"); } + + /// @dev Verifies signature using logic defined by Wallet contract. + /// @param hash Any 32 byte hash. + /// @param walletAddress Address that should have signed the given hash + /// and defines its own signature verification method. + /// @param signature Proof that the hash has been signed by signer. + /// @return True if signature is valid for given wallet.. + function isValidWalletSignature( + bytes32 hash, + address walletAddress, + bytes signature + ) + internal + view + returns (bool isValid) + { + bytes memory calldata = abi.encodeWithSelector( + IWallet(walletAddress).isValidSignature.selector, + hash, + signature + ); + assembly { + let cdStart := add(calldata, 32) + let success := staticcall( + gas, // forward all gas + walletAddress, // address of Wallet contract + cdStart, // pointer to start of input + mload(calldata), // length of input + cdStart, // write input over output + 32 // output size is 32 bytes + ) + + switch success + case 0 { + // Revert with `Error("WALLET_ERROR")` + mstore(0, 0x08c379a000000000000000000000000000000000000000000000000000000000) + mstore(32, 0x0000002000000000000000000000000000000000000000000000000000000000) + mstore(64, 0x0000000c57414c4c45545f4552524f5200000000000000000000000000000000) + mstore(96, 0) + revert(0, 100) + } + case 1 { + // Signature is valid if call did not revert and returned true + isValid := mload(cdStart) + } + } + return isValid; + } + + /// @dev Verifies signature using logic defined by Validator contract. + /// @param validatorAddress Address of validator contract. + /// @param hash Any 32 byte hash. + /// @param signerAddress Address that should have signed the given hash. + /// @param signature Proof that the hash has been signed by signer. + /// @return True if the address recovered from the provided signature matches the input signer address. + function isValidValidatorSignature( + address validatorAddress, + bytes32 hash, + address signerAddress, + bytes signature + ) + internal + view + returns (bool isValid) + { + bytes memory calldata = abi.encodeWithSelector( + IValidator(signerAddress).isValidSignature.selector, + hash, + signerAddress, + signature + ); + assembly { + let cdStart := add(calldata, 32) + let success := staticcall( + gas, // forward all gas + validatorAddress, // address of Validator contract + cdStart, // pointer to start of input + mload(calldata), // length of input + cdStart, // write input over output + 32 // output size is 32 bytes + ) + + switch success + case 0 { + // Revert with `Error("VALIDATOR_ERROR")` + mstore(0, 0x08c379a000000000000000000000000000000000000000000000000000000000) + mstore(32, 0x0000002000000000000000000000000000000000000000000000000000000000) + mstore(64, 0x0000000f56414c494441544f525f4552524f5200000000000000000000000000) + mstore(96, 0) + revert(0, 100) + } + case 1 { + // Signature is valid if call did not revert and returned true + isValid := mload(cdStart) + } + } + return isValid; + } } diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol index 821d30279..4a59b6c0f 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol @@ -155,7 +155,8 @@ contract MixinTransactions is view returns (address) { - address contextAddress = currentContextAddress == address(0) ? msg.sender : currentContextAddress; + address currentContextAddress_ = currentContextAddress; + address contextAddress = currentContextAddress_ == address(0) ? msg.sender : currentContextAddress_; return contextAddress; } } diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol index 86194f461..a5459a21e 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol @@ -19,18 +19,22 @@ pragma solidity 0.4.24; pragma experimental ABIEncoderV2; +import "../../utils/ReentrancyGuard/ReentrancyGuard.sol"; import "./libs/LibMath.sol"; import "./libs/LibOrder.sol"; import "./libs/LibFillResults.sol"; import "./libs/LibAbiEncoder.sol"; import "./mixins/MExchangeCore.sol"; +import "./mixins/MWrapperFunctions.sol"; contract MixinWrapperFunctions is + ReentrancyGuard, LibMath, LibFillResults, LibAbiEncoder, - MExchangeCore + MExchangeCore, + MWrapperFunctions { /// @dev Fills the input order. Reverts if exact takerAssetFillAmount not filled. @@ -43,17 +47,14 @@ contract MixinWrapperFunctions is bytes memory signature ) public + nonReentrant returns (FillResults memory fillResults) { - fillResults = fillOrder( + fillResults = fillOrKillOrderInternal( order, takerAssetFillAmount, signature ); - require( - fillResults.takerAssetFilledAmount == takerAssetFillAmount, - "COMPLETE_FILL_FAILED" - ); return fillResults; } @@ -88,20 +89,14 @@ contract MixinWrapperFunctions is fillOrderCalldata, // write output over input 128 // output size is 128 bytes ) - switch success - case 0 { - mstore(fillResults, 0) - mstore(add(fillResults, 32), 0) - mstore(add(fillResults, 64), 0) - mstore(add(fillResults, 96), 0) - } - case 1 { + if success { mstore(fillResults, mload(fillOrderCalldata)) mstore(add(fillResults, 32), mload(add(fillOrderCalldata, 32))) mstore(add(fillResults, 64), mload(add(fillOrderCalldata, 64))) mstore(add(fillResults, 96), mload(add(fillOrderCalldata, 96))) } } + // fillResults values will be 0 by default if call was unsuccessful return fillResults; } @@ -117,11 +112,12 @@ contract MixinWrapperFunctions is bytes[] memory signatures ) public + nonReentrant returns (FillResults memory totalFillResults) { uint256 ordersLength = orders.length; for (uint256 i = 0; i != ordersLength; i++) { - FillResults memory singleFillResults = fillOrder( + FillResults memory singleFillResults = fillOrderInternal( orders[i], takerAssetFillAmounts[i], signatures[i] @@ -143,11 +139,12 @@ contract MixinWrapperFunctions is bytes[] memory signatures ) public + nonReentrant returns (FillResults memory totalFillResults) { uint256 ordersLength = orders.length; for (uint256 i = 0; i != ordersLength; i++) { - FillResults memory singleFillResults = fillOrKillOrder( + FillResults memory singleFillResults = fillOrKillOrderInternal( orders[i], takerAssetFillAmounts[i], signatures[i] @@ -195,6 +192,7 @@ contract MixinWrapperFunctions is bytes[] memory signatures ) public + nonReentrant returns (FillResults memory totalFillResults) { bytes memory takerAssetData = orders[0].takerAssetData; @@ -210,7 +208,7 @@ contract MixinWrapperFunctions is uint256 remainingTakerAssetFillAmount = safeSub(takerAssetFillAmount, totalFillResults.takerAssetFilledAmount); // Attempt to sell the remaining amount of takerAsset - FillResults memory singleFillResults = fillOrder( + FillResults memory singleFillResults = fillOrderInternal( orders[i], remainingTakerAssetFillAmount, signatures[i] @@ -282,6 +280,7 @@ contract MixinWrapperFunctions is bytes[] memory signatures ) public + nonReentrant returns (FillResults memory totalFillResults) { bytes memory makerAssetData = orders[0].makerAssetData; @@ -298,14 +297,14 @@ contract MixinWrapperFunctions is // 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 - uint256 remainingTakerAssetFillAmount = getPartialAmount( + uint256 remainingTakerAssetFillAmount = getPartialAmountFloor( orders[i].takerAssetAmount, orders[i].makerAssetAmount, remainingMakerAssetFillAmount ); // Attempt to sell the remaining amount of takerAsset - FillResults memory singleFillResults = fillOrder( + FillResults memory singleFillResults = fillOrderInternal( orders[i], remainingTakerAssetFillAmount, signatures[i] @@ -350,7 +349,7 @@ contract MixinWrapperFunctions is // 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 - uint256 remainingTakerAssetFillAmount = getPartialAmount( + uint256 remainingTakerAssetFillAmount = getPartialAmountFloor( orders[i].takerAssetAmount, orders[i].makerAssetAmount, remainingMakerAssetFillAmount @@ -400,4 +399,28 @@ contract MixinWrapperFunctions is } return ordersInfo; } + + /// @dev Fills the input order. Reverts if exact takerAssetFillAmount not filled. + /// @param order Order struct containing order specifications. + /// @param takerAssetFillAmount Desired amount of takerAsset to sell. + /// @param signature Proof that order has been created by maker. + function fillOrKillOrderInternal( + LibOrder.Order memory order, + uint256 takerAssetFillAmount, + bytes memory signature + ) + internal + returns (FillResults memory fillResults) + { + fillResults = fillOrderInternal( + order, + takerAssetFillAmount, + signature + ); + require( + fillResults.takerAssetFilledAmount == takerAssetFillAmount, + "COMPLETE_FILL_FAILED" + ); + return fillResults; + } } 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 fa09da6ac..57fd53f29 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 @@ -25,12 +25,13 @@ contract LibMath is SafeMath { - /// @dev Calculates partial value given a numerator and denominator. + /// @dev Calculates partial value given a numerator and denominator rounded down. + /// Reverts if rounding error is >= 0.1% /// @param numerator Numerator. /// @param denominator Denominator. /// @param target Value to calculate partial of. - /// @return Partial value of target. - function getPartialAmount( + /// @return Partial value of target rounded down. + function safeGetPartialAmountFloor( uint256 numerator, uint256 denominator, uint256 target @@ -39,19 +40,133 @@ contract LibMath is pure returns (uint256 partialAmount) { + require( + denominator > 0, + "DIVISION_BY_ZERO" + ); + + require( + !isRoundingErrorFloor( + numerator, + denominator, + target + ), + "ROUNDING_ERROR" + ); + + partialAmount = safeDiv( + safeMul(numerator, target), + denominator + ); + return partialAmount; + } + + /// @dev Calculates partial value given a numerator and denominator rounded down. + /// Reverts if rounding error is >= 0.1% + /// @param numerator Numerator. + /// @param denominator Denominator. + /// @param target Value to calculate partial of. + /// @return Partial value of target rounded up. + function safeGetPartialAmountCeil( + uint256 numerator, + uint256 denominator, + uint256 target + ) + internal + pure + returns (uint256 partialAmount) + { + require( + denominator > 0, + "DIVISION_BY_ZERO" + ); + + require( + !isRoundingErrorCeil( + numerator, + denominator, + target + ), + "ROUNDING_ERROR" + ); + + // safeDiv computes `floor(a / b)`. We use the identity (a, b integer): + // ceil(a / b) = floor((a + b - 1) / b) + // To implement `ceil(a / b)` using safeDiv. + partialAmount = safeDiv( + safeAdd( + safeMul(numerator, target), + safeSub(denominator, 1) + ), + denominator + ); + return partialAmount; + } + + /// @dev Calculates partial value given a numerator and denominator rounded down. + /// @param numerator Numerator. + /// @param denominator Denominator. + /// @param target Value to calculate partial of. + /// @return Partial value of target rounded down. + function getPartialAmountFloor( + uint256 numerator, + uint256 denominator, + uint256 target + ) + internal + pure + returns (uint256 partialAmount) + { + require( + denominator > 0, + "DIVISION_BY_ZERO" + ); + partialAmount = safeDiv( safeMul(numerator, target), denominator ); return partialAmount; } + + /// @dev Calculates partial value given a numerator and denominator rounded down. + /// @param numerator Numerator. + /// @param denominator Denominator. + /// @param target Value to calculate partial of. + /// @return Partial value of target rounded up. + function getPartialAmountCeil( + uint256 numerator, + uint256 denominator, + uint256 target + ) + internal + pure + returns (uint256 partialAmount) + { + require( + denominator > 0, + "DIVISION_BY_ZERO" + ); - /// @dev Checks if rounding error > 0.1%. + // safeDiv computes `floor(a / b)`. We use the identity (a, b integer): + // ceil(a / b) = floor((a + b - 1) / b) + // To implement `ceil(a / b)` using safeDiv. + partialAmount = safeDiv( + safeAdd( + safeMul(numerator, target), + safeSub(denominator, 1) + ), + denominator + ); + return partialAmount; + } + + /// @dev Checks if rounding error >= 0.1% when rounding down. /// @param numerator Numerator. /// @param denominator Denominator. /// @param target Value to multiply with numerator/denominator. /// @return Rounding error is present. - function isRoundingError( + function isRoundingErrorFloor( uint256 numerator, uint256 denominator, uint256 target @@ -60,16 +175,73 @@ contract LibMath is pure returns (bool isError) { - uint256 remainder = mulmod(target, numerator, denominator); - if (remainder == 0) { - return false; // No rounding error. + require( + denominator > 0, + "DIVISION_BY_ZERO" + ); + + // The absolute rounding error is the difference between the rounded + // value and the ideal value. The relative rounding error is the + // absolute rounding error divided by the absolute value of the + // ideal value. This is undefined when the ideal value is zero. + // + // The ideal value is `numerator * target / denominator`. + // Let's call `numerator * target % denominator` the remainder. + // The absolute error is `remainder / denominator`. + // + // When the ideal value is zero, we require the absolute error to + // be zero. Fortunately, this is always the case. The ideal value is + // zero iff `numerator == 0` and/or `target == 0`. In this case the + // remainder and absolute error are also zero. + if (target == 0 || numerator == 0) { + return false; } - - uint256 errPercentageTimes1000000 = safeDiv( - safeMul(remainder, 1000000), - safeMul(numerator, target) + + // Otherwise, we want the relative rounding error to be strictly + // less than 0.1%. + // The relative error is `remainder / (numerator * target)`. + // We want the relative error less than 1 / 1000: + // remainder / (numerator * denominator) < 1 / 1000 + // or equivalently: + // 1000 * remainder < numerator * target + // so we have a rounding error iff: + // 1000 * remainder >= numerator * target + uint256 remainder = mulmod(target, numerator, denominator); + isError = safeMul(1000, remainder) >= safeMul(numerator, target); + return isError; + } + + /// @dev Checks if rounding error >= 0.1% when rounding up. + /// @param numerator Numerator. + /// @param denominator Denominator. + /// @param target Value to multiply with numerator/denominator. + /// @return Rounding error is present. + function isRoundingErrorCeil( + uint256 numerator, + uint256 denominator, + uint256 target + ) + internal + pure + returns (bool isError) + { + require( + denominator > 0, + "DIVISION_BY_ZERO" ); - isError = errPercentageTimes1000000 > 1000; + + // See the comments in `isRoundingError`. + if (target == 0 || numerator == 0) { + // When either is zero, the ideal value and rounded value are zero + // and there is no rounding error. (Although the relative error + // is undefined.) + return false; + } + // Compute remainder as before + uint256 remainder = mulmod(target, numerator, denominator); + // TODO: safeMod + remainder = safeSub(denominator, remainder) % denominator; + isError = safeMul(1000, remainder) >= safeMul(numerator, target); return isError; } } diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MExchangeCore.sol index c165b647c..d85913e0f 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MExchangeCore.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MExchangeCore.sol @@ -59,6 +59,19 @@ contract MExchangeCore is uint256 orderEpoch // Orders with specified makerAddress and senderAddress with a salt less than this value are considered cancelled. ); + /// @dev Fills the input order. + /// @param order Order struct containing order specifications. + /// @param takerAssetFillAmount Desired amount of takerAsset to sell. + /// @param signature Proof that order has been created by maker. + /// @return Amounts filled and fees paid by maker and taker. + function fillOrderInternal( + LibOrder.Order memory order, + uint256 takerAssetFillAmount, + bytes memory signature + ) + internal + returns (LibFillResults.FillResults memory fillResults); + /// @dev Updates state with results of a fill order. /// @param order that was filled. /// @param takerAddress Address of taker who filled the order. @@ -83,21 +96,33 @@ contract MExchangeCore is bytes32 orderHash ) internal; - + /// @dev Validates context for fillOrder. Succeeds or throws. /// @param order to be filled. - /// @param orderInfo Status, orderHash, and amount already filled of order. + /// @param orderInfo OrderStatus, orderHash, and amount already filled of order. /// @param takerAddress Address of order taker. + /// @param signature Proof that the orders was created by its maker. + function assertFillableOrder( + LibOrder.Order memory order, + LibOrder.OrderInfo memory orderInfo, + address takerAddress, + bytes memory signature + ) + internal + view; + + /// @dev Validates context for fillOrder. Succeeds or throws. + /// @param order to be filled. + /// @param orderInfo Status, orderHash, and amount already filled of order. /// @param takerAssetFillAmount Desired amount of order to fill by taker. /// @param takerAssetFilledAmount Amount of takerAsset that will be filled. - /// @param signature Proof that the orders was created by its maker. + /// @param makerAssetFilledAmount Amount of makerAsset that will be transfered. function assertValidFill( LibOrder.Order memory order, LibOrder.OrderInfo memory orderInfo, - address takerAddress, uint256 takerAssetFillAmount, uint256 takerAssetFilledAmount, - bytes memory signature + uint256 makerAssetFilledAmount ) internal view; diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MSignatureValidator.sol b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MSignatureValidator.sol index f14f2ba00..1fe88b908 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MSignatureValidator.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MSignatureValidator.sol @@ -36,11 +36,40 @@ contract MSignatureValidator is Invalid, // 0x01 EIP712, // 0x02 EthSign, // 0x03 - Caller, // 0x04 - Wallet, // 0x05 - Validator, // 0x06 - PreSigned, // 0x07 - Trezor, // 0x08 - NSignatureTypes // 0x09, number of signature types. Always leave at end. + Wallet, // 0x04 + Validator, // 0x05 + PreSigned, // 0x06 + NSignatureTypes // 0x07, number of signature types. Always leave at end. } + + /// @dev Verifies signature using logic defined by Wallet contract. + /// @param hash Any 32 byte hash. + /// @param walletAddress Address that should have signed the given hash + /// and defines its own signature verification method. + /// @param signature Proof that the hash has been signed by signer. + /// @return True if the address recovered from the provided signature matches the input signer address. + function isValidWalletSignature( + bytes32 hash, + address walletAddress, + bytes signature + ) + internal + view + returns (bool isValid); + + /// @dev Verifies signature using logic defined by Validator contract. + /// @param validatorAddress Address of validator contract. + /// @param hash Any 32 byte hash. + /// @param signerAddress Address that should have signed the given hash. + /// @param signature Proof that the hash has been signed by signer. + /// @return True if the address recovered from the provided signature matches the input signer address. + function isValidValidatorSignature( + address validatorAddress, + bytes32 hash, + address signerAddress, + bytes signature + ) + internal + view + returns (bool isValid); } diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MWrapperFunctions.sol b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MWrapperFunctions.sol new file mode 100644 index 000000000..e04d4a429 --- /dev/null +++ b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MWrapperFunctions.sol @@ -0,0 +1,40 @@ +/* + + 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. + +*/ + +pragma solidity 0.4.24; +pragma experimental ABIEncoderV2; + +import "../libs/LibOrder.sol"; +import "../libs/LibFillResults.sol"; +import "../interfaces/IWrapperFunctions.sol"; + + +contract MWrapperFunctions { + + /// @dev Fills the input order. Reverts if exact takerAssetFillAmount not filled. + /// @param order LibOrder.Order struct containing order specifications. + /// @param takerAssetFillAmount Desired amount of takerAsset to sell. + /// @param signature Proof that order has been created by maker. + function fillOrKillOrderInternal( + LibOrder.Order memory order, + uint256 takerAssetFillAmount, + bytes memory signature + ) + internal + returns (LibFillResults.FillResults memory fillResults); +} diff --git a/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol b/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol new file mode 100644 index 000000000..4f4e28302 --- /dev/null +++ b/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol @@ -0,0 +1,183 @@ +/* + + 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. + +*/ + +pragma solidity 0.4.24; +pragma experimental ABIEncoderV2; + +import "../../utils/LibBytes/LibBytes.sol"; +import "../../tokens/ERC20Token/ERC20Token.sol"; +import "../../protocol/Exchange/interfaces/IExchange.sol"; +import "../../protocol/Exchange/libs/LibOrder.sol"; + + +// solhint-disable no-unused-vars +contract ReentrantERC20Token is + ERC20Token +{ + + using LibBytes for bytes; + + // solhint-disable-next-line var-name-mixedcase + IExchange internal EXCHANGE; + + bytes internal constant REENTRANCY_ILLEGAL_REVERT_REASON = abi.encodeWithSelector( + bytes4(keccak256("Error(string)")), + "REENTRANCY_ILLEGAL" + ); + + // All of these functions are potentially vulnerable to reentrancy + // We do not test any "noThrow" functions because `fillOrderNoThrow` makes a delegatecall to `fillOrder` + enum ExchangeFunction { + FILL_ORDER, + FILL_OR_KILL_ORDER, + BATCH_FILL_ORDERS, + BATCH_FILL_OR_KILL_ORDERS, + MARKET_BUY_ORDERS, + MARKET_SELL_ORDERS, + MATCH_ORDERS, + CANCEL_ORDER, + CANCEL_ORDERS_UP_TO, + SET_SIGNATURE_VALIDATOR_APPROVAL + } + + uint8 internal currentFunctionId = 0; + + constructor (address _exchange) + public + { + EXCHANGE = IExchange(_exchange); + } + + /// @dev Set the current function that will be called when `transferFrom` is called. + /// @param _currentFunctionId Id that corresponds to function name. + function setCurrentFunction(uint8 _currentFunctionId) + external + { + currentFunctionId = _currentFunctionId; + } + + /// @dev A version of `transferFrom` that attempts to reenter the Exchange contract. + /// @param _from The address of the sender + /// @param _to The address of the recipient + /// @param _value The amount of token to be transferred + function transferFrom( + address _from, + address _to, + uint256 _value + ) + external + returns (bool) + { + // This order would normally be invalid, but it will be used strictly for testing reentrnacy. + // Any reentrancy checks will happen before any other checks that invalidate the order. + LibOrder.Order memory order; + + // Initialize remaining null parameters + bytes memory signature; + LibOrder.Order[] memory orders; + uint256[] memory takerAssetFillAmounts; + bytes[] memory signatures; + bytes memory calldata; + + // Create calldata for function that corresponds to currentFunctionId + if (currentFunctionId == uint8(ExchangeFunction.FILL_ORDER)) { + calldata = abi.encodeWithSelector( + EXCHANGE.fillOrder.selector, + order, + 0, + signature + ); + } else if (currentFunctionId == uint8(ExchangeFunction.FILL_OR_KILL_ORDER)) { + calldata = abi.encodeWithSelector( + EXCHANGE.fillOrKillOrder.selector, + order, + 0, + signature + ); + } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_FILL_ORDERS)) { + calldata = abi.encodeWithSelector( + EXCHANGE.batchFillOrders.selector, + orders, + takerAssetFillAmounts, + signatures + ); + } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_FILL_OR_KILL_ORDERS)) { + calldata = abi.encodeWithSelector( + EXCHANGE.batchFillOrKillOrders.selector, + orders, + takerAssetFillAmounts, + signatures + ); + } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_BUY_ORDERS)) { + calldata = abi.encodeWithSelector( + EXCHANGE.marketBuyOrders.selector, + orders, + 0, + signatures + ); + } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_SELL_ORDERS)) { + calldata = abi.encodeWithSelector( + EXCHANGE.marketSellOrders.selector, + orders, + 0, + signatures + ); + } else if (currentFunctionId == uint8(ExchangeFunction.MATCH_ORDERS)) { + calldata = abi.encodeWithSelector( + EXCHANGE.matchOrders.selector, + order, + order, + signature, + signature + ); + } else if (currentFunctionId == uint8(ExchangeFunction.CANCEL_ORDER)) { + calldata = abi.encodeWithSelector( + EXCHANGE.cancelOrder.selector, + order + ); + } else if (currentFunctionId == uint8(ExchangeFunction.CANCEL_ORDERS_UP_TO)) { + calldata = abi.encodeWithSelector( + EXCHANGE.cancelOrdersUpTo.selector, + 0 + ); + } else if (currentFunctionId == uint8(ExchangeFunction.SET_SIGNATURE_VALIDATOR_APPROVAL)) { + calldata = abi.encodeWithSelector( + EXCHANGE.setSignatureValidatorApproval.selector, + address(0), + false + ); + } + + // Call Exchange function, swallow error + address(EXCHANGE).call(calldata); + + // Revert reason is 100 bytes + bytes memory returnData = new bytes(100); + + // Copy return data + assembly { + returndatacopy(add(returnData, 32), 0, 100) + } + + // Revert if function reverted with REENTRANCY_ILLEGAL error + require(!REENTRANCY_ILLEGAL_REVERT_REASON.equals(returnData)); + + // Transfer will return true if function failed for any other reason + return true; + } +}
\ No newline at end of file diff --git a/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol b/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol index d9cec9edc..27187f8f8 100644 --- a/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol +++ b/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol @@ -63,11 +63,12 @@ contract TestExchangeInternals is } /// @dev Calculates partial value given a numerator and denominator. + /// Reverts if rounding error is >= 0.1% /// @param numerator Numerator. /// @param denominator Denominator. /// @param target Value to calculate partial of. /// @return Partial value of target. - function publicGetPartialAmount( + function publicSafeGetPartialAmountFloor( uint256 numerator, uint256 denominator, uint256 target @@ -76,15 +77,84 @@ contract TestExchangeInternals is pure returns (uint256 partialAmount) { - return getPartialAmount(numerator, denominator, target); + return safeGetPartialAmountFloor(numerator, denominator, target); } - /// @dev Checks if rounding error > 0.1%. + /// @dev Calculates partial value given a numerator and denominator. + /// Reverts if rounding error is >= 0.1% + /// @param numerator Numerator. + /// @param denominator Denominator. + /// @param target Value to calculate partial of. + /// @return Partial value of target. + function publicSafeGetPartialAmountCeil( + uint256 numerator, + uint256 denominator, + uint256 target + ) + public + pure + returns (uint256 partialAmount) + { + return safeGetPartialAmountCeil(numerator, denominator, target); + } + + /// @dev Calculates partial value given a numerator and denominator. + /// @param numerator Numerator. + /// @param denominator Denominator. + /// @param target Value to calculate partial of. + /// @return Partial value of target. + function publicGetPartialAmountFloor( + uint256 numerator, + uint256 denominator, + uint256 target + ) + public + pure + returns (uint256 partialAmount) + { + return getPartialAmountFloor(numerator, denominator, target); + } + + /// @dev Calculates partial value given a numerator and denominator. + /// @param numerator Numerator. + /// @param denominator Denominator. + /// @param target Value to calculate partial of. + /// @return Partial value of target. + function publicGetPartialAmountCeil( + uint256 numerator, + uint256 denominator, + uint256 target + ) + public + pure + returns (uint256 partialAmount) + { + return getPartialAmountCeil(numerator, denominator, target); + } + + /// @dev Checks if rounding error >= 0.1%. + /// @param numerator Numerator. + /// @param denominator Denominator. + /// @param target Value to multiply with numerator/denominator. + /// @return Rounding error is present. + function publicIsRoundingErrorFloor( + uint256 numerator, + uint256 denominator, + uint256 target + ) + public + pure + returns (bool isError) + { + return isRoundingErrorFloor(numerator, denominator, target); + } + + /// @dev Checks if rounding error >= 0.1%. /// @param numerator Numerator. /// @param denominator Denominator. /// @param target Value to multiply with numerator/denominator. /// @return Rounding error is present. - function publicIsRoundingError( + function publicIsRoundingErrorCeil( uint256 numerator, uint256 denominator, uint256 target @@ -93,7 +163,7 @@ contract TestExchangeInternals is pure returns (bool isError) { - return isRoundingError(numerator, denominator, target); + return isRoundingErrorCeil(numerator, denominator, target); } /// @dev Updates state with results of a fill order. diff --git a/packages/contracts/src/2.0.0/test/TestLibs/TestLibs.sol b/packages/contracts/src/2.0.0/test/TestLibs/TestLibs.sol index 4a99dd9c1..c8c58545f 100644 --- a/packages/contracts/src/2.0.0/test/TestLibs/TestLibs.sol +++ b/packages/contracts/src/2.0.0/test/TestLibs/TestLibs.sol @@ -49,7 +49,7 @@ contract TestLibs is return fillOrderCalldata; } - function publicGetPartialAmount( + function publicGetPartialAmountFloor( uint256 numerator, uint256 denominator, uint256 target @@ -58,7 +58,7 @@ contract TestLibs is pure returns (uint256 partialAmount) { - partialAmount = getPartialAmount( + partialAmount = getPartialAmountFloor( numerator, denominator, target @@ -66,7 +66,41 @@ contract TestLibs is return partialAmount; } - function publicIsRoundingError( + function publicGetPartialAmountCeil( + uint256 numerator, + uint256 denominator, + uint256 target + ) + public + pure + returns (uint256 partialAmount) + { + partialAmount = getPartialAmountCeil( + numerator, + denominator, + target + ); + return partialAmount; + } + + function publicIsRoundingErrorFloor( + uint256 numerator, + uint256 denominator, + uint256 target + ) + public + pure + returns (bool isError) + { + isError = isRoundingErrorFloor( + numerator, + denominator, + target + ); + return isError; + } + + function publicIsRoundingErrorCeil( uint256 numerator, uint256 denominator, uint256 target @@ -75,7 +109,7 @@ contract TestLibs is pure returns (bool isError) { - isError = isRoundingError( + isError = isRoundingErrorCeil( numerator, denominator, target diff --git a/packages/contracts/src/2.0.0/test/TestStaticCallReceiver/TestStaticCallReceiver.sol b/packages/contracts/src/2.0.0/test/TestStaticCallReceiver/TestStaticCallReceiver.sol new file mode 100644 index 000000000..41aab01c8 --- /dev/null +++ b/packages/contracts/src/2.0.0/test/TestStaticCallReceiver/TestStaticCallReceiver.sol @@ -0,0 +1,81 @@ +/* + + 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. + +*/ + +pragma solidity 0.4.24; + +import "../../tokens/ERC20Token/IERC20Token.sol"; + + +// solhint-disable no-unused-vars +contract TestStaticCallReceiver { + + uint256 internal state = 1; + + /// @dev Updates state and returns true. Intended to be used with `Validator` signature type. + /// @param hash Message hash that is signed. + /// @param signerAddress Address that should have signed the given hash. + /// @param signature Proof of signing. + /// @return Validity of order signature. + function isValidSignature( + bytes32 hash, + address signerAddress, + bytes signature + ) + external + returns (bool isValid) + { + updateState(); + return true; + } + + /// @dev Updates state and returns true. Intended to be used with `Wallet` signature type. + /// @param hash Message hash that is signed. + /// @param signature Proof of signing. + /// @return Validity of order signature. + function isValidSignature( + bytes32 hash, + bytes signature + ) + external + returns (bool isValid) + { + updateState(); + return true; + } + + /// @dev Approves an ERC20 token to spend tokens from this address. + /// @param token Address of ERC20 token. + /// @param spender Address that will spend tokens. + /// @param value Amount of tokens spender is approved to spend. + function approveERC20( + address token, + address spender, + uint256 value + ) + external + { + IERC20Token(token).approve(spender, value); + } + + /// @dev Increments state variable. + function updateState() + internal + { + state++; + } +} diff --git a/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol b/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol new file mode 100644 index 000000000..1dee512d4 --- /dev/null +++ b/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol @@ -0,0 +1,44 @@ +/* + + 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. + +*/ +pragma solidity 0.4.24; + + +contract ReentrancyGuard { + + // Locked state of mutex + bool private locked = false; + + /// @dev Functions with this modifer cannot be reentered. The mutex will be locked + /// before function execution and unlocked after. + modifier nonReentrant() { + // Ensure mutex is unlocked + require( + !locked, + "REENTRANCY_ILLEGAL" + ); + + // Lock mutex before function call + locked = true; + + // Perform function call + _; + + // Unlock mutex after function call + locked = false; + } +} diff --git a/packages/contracts/test/asset_proxy/proxies.ts b/packages/contracts/test/asset_proxy/proxies.ts index 76a020222..6031e554d 100644 --- a/packages/contracts/test/asset_proxy/proxies.ts +++ b/packages/contracts/test/asset_proxy/proxies.ts @@ -12,7 +12,7 @@ import { ERC20ProxyContract } from '../../generated_contract_wrappers/erc20_prox import { ERC721ProxyContract } from '../../generated_contract_wrappers/erc721_proxy'; import { IAssetProxyContract } from '../../generated_contract_wrappers/i_asset_proxy'; import { artifacts } from '../utils/artifacts'; -import { expectTransactionFailedAsync } from '../utils/assertions'; +import { expectTransactionFailedAsync, expectTransactionFailedWithoutReasonAsync } from '../utils/assertions'; import { chaiSetup } from '../utils/chai_setup'; import { constants } from '../utils/constants'; import { ERC20Wrapper } from '../utils/erc20_wrapper'; @@ -99,6 +99,17 @@ describe('Asset Transfer Proxies', () => { await blockchainLifecycle.revertAsync(); }); describe('Transfer Proxy - ERC20', () => { + it('should revert if undefined function is called', async () => { + const undefinedSelector = '0x01020304'; + await expectTransactionFailedWithoutReasonAsync( + web3Wrapper.sendTransactionAsync({ + from: owner, + to: erc20Proxy.address, + value: constants.ZERO_AMOUNT, + data: undefinedSelector, + }), + ); + }); describe('transferFrom', () => { it('should successfully transfer tokens', async () => { // Construct ERC20 asset data @@ -219,6 +230,17 @@ describe('Asset Transfer Proxies', () => { }); describe('Transfer Proxy - ERC721', () => { + it('should revert if undefined function is called', async () => { + const undefinedSelector = '0x01020304'; + await expectTransactionFailedWithoutReasonAsync( + web3Wrapper.sendTransactionAsync({ + from: owner, + to: erc721Proxy.address, + value: constants.ZERO_AMOUNT, + data: undefinedSelector, + }), + ); + }); describe('transferFrom', () => { it('should successfully transfer tokens', async () => { // Construct ERC721 asset data diff --git a/packages/contracts/test/exchange/core.ts b/packages/contracts/test/exchange/core.ts index bc2bad749..3bb71b58f 100644 --- a/packages/contracts/test/exchange/core.ts +++ b/packages/contracts/test/exchange/core.ts @@ -1,6 +1,6 @@ import { BlockchainLifecycle } from '@0xproject/dev-utils'; import { assetDataUtils, orderHashUtils } from '@0xproject/order-utils'; -import { RevertReason, SignedOrder } from '@0xproject/types'; +import { RevertReason, SignatureType, SignedOrder } from '@0xproject/types'; import { BigNumber } from '@0xproject/utils'; import { Web3Wrapper } from '@0xproject/web3-wrapper'; import * as chai from 'chai'; @@ -14,6 +14,8 @@ import { DummyNoReturnERC20TokenContract } from '../../generated_contract_wrappe import { ERC20ProxyContract } from '../../generated_contract_wrappers/erc20_proxy'; import { ERC721ProxyContract } from '../../generated_contract_wrappers/erc721_proxy'; import { ExchangeCancelEventArgs, ExchangeContract } from '../../generated_contract_wrappers/exchange'; +import { ReentrantERC20TokenContract } from '../../generated_contract_wrappers/reentrant_erc20_token'; +import { TestStaticCallReceiverContract } from '../../generated_contract_wrappers/test_static_call_receiver'; import { artifacts } from '../utils/artifacts'; import { expectTransactionFailedAsync } from '../utils/assertions'; import { getLatestBlockTimestampAsync, increaseTimeAndMineBlockAsync } from '../utils/block_timestamp'; @@ -41,9 +43,12 @@ describe('Exchange core', () => { let zrxToken: DummyERC20TokenContract; let erc721Token: DummyERC721TokenContract; let noReturnErc20Token: DummyNoReturnERC20TokenContract; + let reentrantErc20Token: ReentrantERC20TokenContract; let exchange: ExchangeContract; let erc20Proxy: ERC20ProxyContract; let erc721Proxy: ERC721ProxyContract; + let maliciousWallet: TestStaticCallReceiverContract; + let maliciousValidator: TestStaticCallReceiverContract; let signedOrder: SignedOrder; let erc20Balances: ERC20BalancesByOwner; @@ -109,6 +114,18 @@ describe('Exchange core', () => { constants.AWAIT_TRANSACTION_MINED_MS, ); + maliciousWallet = maliciousValidator = await TestStaticCallReceiverContract.deployFrom0xArtifactAsync( + artifacts.TestStaticCallReceiver, + provider, + txDefaults, + ); + reentrantErc20Token = await ReentrantERC20TokenContract.deployFrom0xArtifactAsync( + artifacts.ReentrantERC20Token, + provider, + txDefaults, + exchange.address, + ); + defaultMakerAssetAddress = erc20TokenA.address; defaultTakerAssetAddress = erc20TokenB.address; @@ -135,6 +152,26 @@ describe('Exchange core', () => { signedOrder = await orderFactory.newSignedOrderAsync(); }); + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow fillOrder to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await expectTransactionFailedAsync( + exchangeWrapper.fillOrderAsync(signedOrder, takerAddress), + RevertReason.TransferFailed, + ); + }); + }); + }; + describe('fillOrder reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should throw if signature is invalid', async () => { signedOrder = await orderFactory.newSignedOrderAsync({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), @@ -161,6 +198,51 @@ describe('Exchange core', () => { RevertReason.OrderUnfillable, ); }); + + it('should revert if `isValidSignature` tries to update state when SignatureType=Wallet', async () => { + const maliciousMakerAddress = maliciousWallet.address; + await web3Wrapper.awaitTransactionSuccessAsync( + await erc20TokenA.setBalance.sendTransactionAsync( + maliciousMakerAddress, + constants.INITIAL_ERC20_BALANCE, + ), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await web3Wrapper.awaitTransactionSuccessAsync( + await maliciousWallet.approveERC20.sendTransactionAsync( + erc20TokenA.address, + erc20Proxy.address, + constants.INITIAL_ERC20_ALLOWANCE, + ), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + signedOrder = await orderFactory.newSignedOrderAsync({ + makerAddress: maliciousMakerAddress, + makerFee: constants.ZERO_AMOUNT, + }); + signedOrder.signature = `0x0${SignatureType.Wallet}`; + await expectTransactionFailedAsync( + exchangeWrapper.fillOrderAsync(signedOrder, takerAddress), + RevertReason.WalletError, + ); + }); + + it('should revert if `isValidSignature` tries to update state when SignatureType=Validator', async () => { + const isApproved = true; + await web3Wrapper.awaitTransactionSuccessAsync( + await exchange.setSignatureValidatorApproval.sendTransactionAsync( + maliciousValidator.address, + isApproved, + { from: makerAddress }, + ), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + signedOrder.signature = `${maliciousValidator.address}0${SignatureType.Validator}`; + await expectTransactionFailedAsync( + exchangeWrapper.fillOrderAsync(signedOrder, takerAddress), + RevertReason.ValidatorError, + ); + }); }); describe('Testing exchange of ERC20 tokens with no return values', () => { @@ -448,7 +530,7 @@ describe('Exchange core', () => { // HACK(albrow): We need to hardcode the gas estimate here because // the Geth gas estimator doesn't work with the way we use // delegatecall and swallow errors. - gas: 490000, + gas: 600000, }); const newBalances = await erc20Wrapper.getBalancesAsync(); diff --git a/packages/contracts/test/exchange/internal.ts b/packages/contracts/test/exchange/internal.ts index 67d1d2d2c..dc2c5fbe0 100644 --- a/packages/contracts/test/exchange/internal.ts +++ b/packages/contracts/test/exchange/internal.ts @@ -1,6 +1,7 @@ import { BlockchainLifecycle } from '@0xproject/dev-utils'; import { Order, RevertReason, SignedOrder } from '@0xproject/types'; import { BigNumber } from '@0xproject/utils'; +import * as chai from 'chai'; import * as _ from 'lodash'; import { TestExchangeInternalsContract } from '../../generated_contract_wrappers/test_exchange_internals'; @@ -16,6 +17,8 @@ import { FillResults } from '../utils/types'; import { provider, txDefaults, web3Wrapper } from '../utils/web3_wrapper'; chaiSetup.configure(); +const expect = chai.expect; + const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); const MAX_UINT256 = new BigNumber(2).pow(256).minus(1); @@ -43,26 +46,11 @@ const emptySignedOrder: SignedOrder = { const overflowErrorForCall = new Error(RevertReason.Uint256Overflow); -async function referenceGetPartialAmountAsync( - numerator: BigNumber, - denominator: BigNumber, - target: BigNumber, -): Promise<BigNumber> { - const invalidOpcodeErrorForCall = new Error(await getInvalidOpcodeErrorMessageForCallAsync()); - const product = numerator.mul(target); - if (product.greaterThan(MAX_UINT256)) { - throw overflowErrorForCall; - } - if (denominator.eq(0)) { - throw invalidOpcodeErrorForCall; - } - return product.dividedToIntegerBy(denominator); -} - describe('Exchange core internal functions', () => { let testExchange: TestExchangeInternalsContract; - let invalidOpcodeErrorForCall: Error | undefined; let overflowErrorForSendTransaction: Error | undefined; + let divisionByZeroErrorForCall: Error | undefined; + let roundingErrorForCall: Error | undefined; before(async () => { await blockchainLifecycle.startAsync(); @@ -79,11 +67,86 @@ describe('Exchange core internal functions', () => { overflowErrorForSendTransaction = new Error( await getRevertReasonOrErrorMessageForSendTransactionAsync(RevertReason.Uint256Overflow), ); - invalidOpcodeErrorForCall = new Error(await getInvalidOpcodeErrorMessageForCallAsync()); + divisionByZeroErrorForCall = new Error(RevertReason.DivisionByZero); + roundingErrorForCall = new Error(RevertReason.RoundingError); }); // Note(albrow): Don't forget to add beforeEach and afterEach calls to reset // the blockchain state for any tests which modify it! + async function referenceIsRoundingErrorFloorAsync( + numerator: BigNumber, + denominator: BigNumber, + target: BigNumber, + ): Promise<boolean> { + if (denominator.eq(0)) { + throw divisionByZeroErrorForCall; + } + if (numerator.eq(0)) { + return false; + } + if (target.eq(0)) { + return false; + } + const product = numerator.mul(target); + const remainder = product.mod(denominator); + const remainderTimes1000 = remainder.mul('1000'); + const isError = remainderTimes1000.gte(product); + if (product.greaterThan(MAX_UINT256)) { + throw overflowErrorForCall; + } + if (remainderTimes1000.greaterThan(MAX_UINT256)) { + throw overflowErrorForCall; + } + return isError; + } + + async function referenceIsRoundingErrorCeilAsync( + numerator: BigNumber, + denominator: BigNumber, + target: BigNumber, + ): Promise<boolean> { + if (denominator.eq(0)) { + throw divisionByZeroErrorForCall; + } + if (numerator.eq(0)) { + return false; + } + if (target.eq(0)) { + return false; + } + const product = numerator.mul(target); + const remainder = product.mod(denominator); + const error = denominator.sub(remainder).mod(denominator); + const errorTimes1000 = error.mul('1000'); + const isError = errorTimes1000.gte(product); + if (product.greaterThan(MAX_UINT256)) { + throw overflowErrorForCall; + } + if (errorTimes1000.greaterThan(MAX_UINT256)) { + throw overflowErrorForCall; + } + return isError; + } + + async function referenceSafeGetPartialAmountFloorAsync( + numerator: BigNumber, + denominator: BigNumber, + target: BigNumber, + ): Promise<BigNumber> { + if (denominator.eq(0)) { + throw divisionByZeroErrorForCall; + } + const isRoundingError = await referenceIsRoundingErrorFloorAsync(numerator, denominator, target); + if (isRoundingError) { + throw roundingErrorForCall; + } + const product = numerator.mul(target); + if (product.greaterThan(MAX_UINT256)) { + throw overflowErrorForCall; + } + return product.dividedToIntegerBy(denominator); + } + describe('addFillResults', async () => { function makeFillResults(value: BigNumber): FillResults { return { @@ -158,19 +221,22 @@ describe('Exchange core internal functions', () => { // in any mathematical operation in either the reference TypeScript // implementation or the Solidity implementation of // calculateFillResults. + const makerAssetFilledAmount = await referenceSafeGetPartialAmountFloorAsync( + takerAssetFilledAmount, + orderTakerAssetAmount, + otherAmount, + ); + const order = makeOrder(otherAmount, orderTakerAssetAmount, otherAmount, otherAmount); + const orderMakerAssetAmount = order.makerAssetAmount; return { - makerAssetFilledAmount: await referenceGetPartialAmountAsync( - takerAssetFilledAmount, - orderTakerAssetAmount, - otherAmount, - ), + makerAssetFilledAmount, takerAssetFilledAmount, - makerFeePaid: await referenceGetPartialAmountAsync( - takerAssetFilledAmount, - orderTakerAssetAmount, + makerFeePaid: await referenceSafeGetPartialAmountFloorAsync( + makerAssetFilledAmount, + orderMakerAssetAmount, otherAmount, ), - takerFeePaid: await referenceGetPartialAmountAsync( + takerFeePaid: await referenceSafeGetPartialAmountFloorAsync( takerAssetFilledAmount, orderTakerAssetAmount, otherAmount, @@ -193,60 +259,158 @@ describe('Exchange core internal functions', () => { ); }); - describe('getPartialAmount', async () => { - async function testGetPartialAmountAsync( + describe('getPartialAmountFloor', async () => { + async function referenceGetPartialAmountFloorAsync( numerator: BigNumber, denominator: BigNumber, target: BigNumber, ): Promise<BigNumber> { - return testExchange.publicGetPartialAmount.callAsync(numerator, denominator, target); + if (denominator.eq(0)) { + throw divisionByZeroErrorForCall; + } + const product = numerator.mul(target); + if (product.greaterThan(MAX_UINT256)) { + throw overflowErrorForCall; + } + return product.dividedToIntegerBy(denominator); + } + async function testGetPartialAmountFloorAsync( + numerator: BigNumber, + denominator: BigNumber, + target: BigNumber, + ): Promise<BigNumber> { + return testExchange.publicGetPartialAmountFloor.callAsync(numerator, denominator, target); } await testCombinatoriallyWithReferenceFuncAsync( - 'getPartialAmount', - referenceGetPartialAmountAsync, - testGetPartialAmountAsync, + 'getPartialAmountFloor', + referenceGetPartialAmountFloorAsync, + testGetPartialAmountFloorAsync, [uint256Values, uint256Values, uint256Values], ); }); - describe('isRoundingError', async () => { - async function referenceIsRoundingErrorAsync( + describe('getPartialAmountCeil', async () => { + async function referenceGetPartialAmountCeilAsync( numerator: BigNumber, denominator: BigNumber, target: BigNumber, - ): Promise<boolean> { - const product = numerator.mul(target); + ): Promise<BigNumber> { if (denominator.eq(0)) { - throw invalidOpcodeErrorForCall; - } - const remainder = product.mod(denominator); - if (remainder.eq(0)) { - return false; + throw divisionByZeroErrorForCall; } - if (product.greaterThan(MAX_UINT256)) { + const product = numerator.mul(target); + const offset = product.add(denominator.sub(1)); + if (offset.greaterThan(MAX_UINT256)) { throw overflowErrorForCall; } - if (product.eq(0)) { - throw invalidOpcodeErrorForCall; + const result = offset.dividedToIntegerBy(denominator); + if (product.mod(denominator).eq(0)) { + expect(result.mul(denominator)).to.be.bignumber.eq(product); + } else { + expect(result.mul(denominator)).to.be.bignumber.gt(product); } - const remainderTimes1000000 = remainder.mul('1000000'); - if (remainderTimes1000000.greaterThan(MAX_UINT256)) { + return result; + } + async function testGetPartialAmountCeilAsync( + numerator: BigNumber, + denominator: BigNumber, + target: BigNumber, + ): Promise<BigNumber> { + return testExchange.publicGetPartialAmountCeil.callAsync(numerator, denominator, target); + } + await testCombinatoriallyWithReferenceFuncAsync( + 'getPartialAmountCeil', + referenceGetPartialAmountCeilAsync, + testGetPartialAmountCeilAsync, + [uint256Values, uint256Values, uint256Values], + ); + }); + + describe('safeGetPartialAmountFloor', async () => { + async function testSafeGetPartialAmountFloorAsync( + numerator: BigNumber, + denominator: BigNumber, + target: BigNumber, + ): Promise<BigNumber> { + return testExchange.publicSafeGetPartialAmountFloor.callAsync(numerator, denominator, target); + } + await testCombinatoriallyWithReferenceFuncAsync( + 'safeGetPartialAmountFloor', + referenceSafeGetPartialAmountFloorAsync, + testSafeGetPartialAmountFloorAsync, + [uint256Values, uint256Values, uint256Values], + ); + }); + + describe('safeGetPartialAmountCeil', async () => { + async function referenceSafeGetPartialAmountCeilAsync( + numerator: BigNumber, + denominator: BigNumber, + target: BigNumber, + ): Promise<BigNumber> { + if (denominator.eq(0)) { + throw divisionByZeroErrorForCall; + } + const isRoundingError = await referenceIsRoundingErrorCeilAsync(numerator, denominator, target); + if (isRoundingError) { + throw roundingErrorForCall; + } + const product = numerator.mul(target); + const offset = product.add(denominator.sub(1)); + if (offset.greaterThan(MAX_UINT256)) { throw overflowErrorForCall; } - const errPercentageTimes1000000 = remainderTimes1000000.dividedToIntegerBy(product); - return errPercentageTimes1000000.greaterThan('1000'); + const result = offset.dividedToIntegerBy(denominator); + if (product.mod(denominator).eq(0)) { + expect(result.mul(denominator)).to.be.bignumber.eq(product); + } else { + expect(result.mul(denominator)).to.be.bignumber.gt(product); + } + return result; + } + async function testSafeGetPartialAmountCeilAsync( + numerator: BigNumber, + denominator: BigNumber, + target: BigNumber, + ): Promise<BigNumber> { + return testExchange.publicSafeGetPartialAmountCeil.callAsync(numerator, denominator, target); + } + await testCombinatoriallyWithReferenceFuncAsync( + 'safeGetPartialAmountCeil', + referenceSafeGetPartialAmountCeilAsync, + testSafeGetPartialAmountCeilAsync, + [uint256Values, uint256Values, uint256Values], + ); + }); + + describe('isRoundingErrorFloor', async () => { + async function testIsRoundingErrorFloorAsync( + numerator: BigNumber, + denominator: BigNumber, + target: BigNumber, + ): Promise<boolean> { + return testExchange.publicIsRoundingErrorFloor.callAsync(numerator, denominator, target); } - async function testIsRoundingErrorAsync( + await testCombinatoriallyWithReferenceFuncAsync( + 'isRoundingErrorFloor', + referenceIsRoundingErrorFloorAsync, + testIsRoundingErrorFloorAsync, + [uint256Values, uint256Values, uint256Values], + ); + }); + + describe('isRoundingErrorCeil', async () => { + async function testIsRoundingErrorCeilAsync( numerator: BigNumber, denominator: BigNumber, target: BigNumber, ): Promise<boolean> { - return testExchange.publicIsRoundingError.callAsync(numerator, denominator, target); + return testExchange.publicIsRoundingErrorCeil.callAsync(numerator, denominator, target); } await testCombinatoriallyWithReferenceFuncAsync( - 'isRoundingError', - referenceIsRoundingErrorAsync, - testIsRoundingErrorAsync, + 'isRoundingErrorCeil', + referenceIsRoundingErrorCeilAsync, + testIsRoundingErrorCeilAsync, [uint256Values, uint256Values, uint256Values], ); }); diff --git a/packages/contracts/test/exchange/libs.ts b/packages/contracts/test/exchange/libs.ts index 6c3305d1d..37234489e 100644 --- a/packages/contracts/test/exchange/libs.ts +++ b/packages/contracts/test/exchange/libs.ts @@ -71,29 +71,57 @@ describe('Exchange libs', () => { // combinatorial tests in test/exchange/internal. They test specific edge // cases that are not covered by the combinatorial tests. describe('LibMath', () => { - it('should return false if there is a rounding error of 0.1%', async () => { - const numerator = new BigNumber(20); - const denominator = new BigNumber(999); - const target = new BigNumber(50); - // rounding error = ((20*50/999) - floor(20*50/999)) / (20*50/999) = 0.1% - const isRoundingError = await libs.publicIsRoundingError.callAsync(numerator, denominator, target); - expect(isRoundingError).to.be.false(); - }); - it('should return false if there is a rounding of 0.09%', async () => { - const numerator = new BigNumber(20); - const denominator = new BigNumber(9991); - const target = new BigNumber(500); - // rounding error = ((20*500/9991) - floor(20*500/9991)) / (20*500/9991) = 0.09% - const isRoundingError = await libs.publicIsRoundingError.callAsync(numerator, denominator, target); - expect(isRoundingError).to.be.false(); + describe('isRoundingError', () => { + it('should return true if there is a rounding error of 0.1%', async () => { + const numerator = new BigNumber(20); + const denominator = new BigNumber(999); + const target = new BigNumber(50); + // rounding error = ((20*50/999) - floor(20*50/999)) / (20*50/999) = 0.1% + const isRoundingError = await libs.publicIsRoundingErrorFloor.callAsync(numerator, denominator, target); + expect(isRoundingError).to.be.true(); + }); + it('should return false if there is a rounding of 0.09%', async () => { + const numerator = new BigNumber(20); + const denominator = new BigNumber(9991); + const target = new BigNumber(500); + // rounding error = ((20*500/9991) - floor(20*500/9991)) / (20*500/9991) = 0.09% + const isRoundingError = await libs.publicIsRoundingErrorFloor.callAsync(numerator, denominator, target); + expect(isRoundingError).to.be.false(); + }); + it('should return true if there is a rounding error of 0.11%', async () => { + const numerator = new BigNumber(20); + const denominator = new BigNumber(9989); + const target = new BigNumber(500); + // rounding error = ((20*500/9989) - floor(20*500/9989)) / (20*500/9989) = 0.011% + const isRoundingError = await libs.publicIsRoundingErrorFloor.callAsync(numerator, denominator, target); + expect(isRoundingError).to.be.true(); + }); }); - it('should return true if there is a rounding error of 0.11%', async () => { - const numerator = new BigNumber(20); - const denominator = new BigNumber(9989); - const target = new BigNumber(500); - // rounding error = ((20*500/9989) - floor(20*500/9989)) / (20*500/9989) = 0.011% - const isRoundingError = await libs.publicIsRoundingError.callAsync(numerator, denominator, target); - expect(isRoundingError).to.be.true(); + describe('isRoundingErrorCeil', () => { + it('should return true if there is a rounding error of 0.1%', async () => { + const numerator = new BigNumber(20); + const denominator = new BigNumber(1001); + const target = new BigNumber(50); + // rounding error = (ceil(20*50/1001) - (20*50/1001)) / (20*50/1001) = 0.1% + const isRoundingError = await libs.publicIsRoundingErrorCeil.callAsync(numerator, denominator, target); + expect(isRoundingError).to.be.true(); + }); + it('should return false if there is a rounding of 0.09%', async () => { + const numerator = new BigNumber(20); + const denominator = new BigNumber(10009); + const target = new BigNumber(500); + // rounding error = (ceil(20*500/10009) - (20*500/10009)) / (20*500/10009) = 0.09% + const isRoundingError = await libs.publicIsRoundingErrorCeil.callAsync(numerator, denominator, target); + expect(isRoundingError).to.be.false(); + }); + it('should return true if there is a rounding error of 0.11%', async () => { + const numerator = new BigNumber(20); + const denominator = new BigNumber(10011); + const target = new BigNumber(500); + // rounding error = (ceil(20*500/10011) - (20*500/10011)) / (20*500/10011) = 0.11% + const isRoundingError = await libs.publicIsRoundingErrorCeil.callAsync(numerator, denominator, target); + expect(isRoundingError).to.be.true(); + }); }); }); diff --git a/packages/contracts/test/exchange/match_orders.ts b/packages/contracts/test/exchange/match_orders.ts index 46b3569bd..554c456cc 100644 --- a/packages/contracts/test/exchange/match_orders.ts +++ b/packages/contracts/test/exchange/match_orders.ts @@ -11,6 +11,8 @@ import { DummyERC721TokenContract } from '../../generated_contract_wrappers/dumm import { ERC20ProxyContract } from '../../generated_contract_wrappers/erc20_proxy'; import { ERC721ProxyContract } from '../../generated_contract_wrappers/erc721_proxy'; import { ExchangeContract } from '../../generated_contract_wrappers/exchange'; +import { ReentrantERC20TokenContract } from '../../generated_contract_wrappers/reentrant_erc20_token'; +import { TestExchangeInternalsContract } from '../../generated_contract_wrappers/test_exchange_internals'; import { artifacts } from '../utils/artifacts'; import { expectTransactionFailedAsync } from '../utils/assertions'; import { chaiSetup } from '../utils/chai_setup'; @@ -20,12 +22,12 @@ import { ERC721Wrapper } from '../utils/erc721_wrapper'; import { ExchangeWrapper } from '../utils/exchange_wrapper'; import { MatchOrderTester } from '../utils/match_order_tester'; import { OrderFactory } from '../utils/order_factory'; -import { ERC20BalancesByOwner, ERC721TokenIdsByOwner, OrderInfo, OrderStatus } from '../utils/types'; +import { ERC20BalancesByOwner, ERC721TokenIdsByOwner } from '../utils/types'; import { provider, txDefaults, web3Wrapper } from '../utils/web3_wrapper'; +const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); chaiSetup.configure(); const expect = chai.expect; -const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); describe('matchOrders', () => { let makerAddressLeft: string; @@ -39,6 +41,7 @@ describe('matchOrders', () => { let erc20TokenB: DummyERC20TokenContract; let zrxToken: DummyERC20TokenContract; let erc721Token: DummyERC721TokenContract; + let reentrantErc20Token: ReentrantERC20TokenContract; let exchange: ExchangeContract; let erc20Proxy: ERC20ProxyContract; let erc721Proxy: ERC721ProxyContract; @@ -60,6 +63,8 @@ describe('matchOrders', () => { let matchOrderTester: MatchOrderTester; + let testExchange: TestExchangeInternalsContract; + before(async () => { await blockchainLifecycle.startAsync(); }); @@ -127,23 +132,46 @@ describe('matchOrders', () => { }), constants.AWAIT_TRANSACTION_MINED_MS, ); + + reentrantErc20Token = await ReentrantERC20TokenContract.deployFrom0xArtifactAsync( + artifacts.ReentrantERC20Token, + provider, + txDefaults, + exchange.address, + ); + // Set default addresses defaultERC20MakerAssetAddress = erc20TokenA.address; defaultERC20TakerAssetAddress = erc20TokenB.address; defaultERC721AssetAddress = erc721Token.address; // Create default order parameters - const defaultOrderParams = { + const defaultOrderParamsLeft = { ...constants.STATIC_ORDER_PARAMS, + makerAddress: makerAddressLeft, exchangeAddress: exchange.address, makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), + feeRecipientAddress: feeRecipientAddressLeft, + }; + const defaultOrderParamsRight = { + ...constants.STATIC_ORDER_PARAMS, + makerAddress: makerAddressRight, + exchangeAddress: exchange.address, + makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), + takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), + feeRecipientAddress: feeRecipientAddressRight, }; const privateKeyLeft = constants.TESTRPC_PRIVATE_KEYS[accounts.indexOf(makerAddressLeft)]; - orderFactoryLeft = new OrderFactory(privateKeyLeft, defaultOrderParams); + orderFactoryLeft = new OrderFactory(privateKeyLeft, defaultOrderParamsLeft); const privateKeyRight = constants.TESTRPC_PRIVATE_KEYS[accounts.indexOf(makerAddressRight)]; - orderFactoryRight = new OrderFactory(privateKeyRight, defaultOrderParams); + orderFactoryRight = new OrderFactory(privateKeyRight, defaultOrderParamsRight); // Set match order tester matchOrderTester = new MatchOrderTester(exchangeWrapper, erc20Wrapper, erc721Wrapper, zrxToken.address); + testExchange = await TestExchangeInternalsContract.deployFrom0xArtifactAsync( + artifacts.TestExchangeInternals, + provider, + txDefaults, + ); }); beforeEach(async () => { await blockchainLifecycle.startAsync(); @@ -157,263 +185,580 @@ describe('matchOrders', () => { erc721TokenIdsByOwner = await erc721Wrapper.getBalancesAsync(); }); - it('should transfer the correct amounts when orders completely fill each other', async () => { + it('Should transfer correct amounts when right order is fully filled and values pass isRoundingErrorFloor but fail isRoundingErrorCeil', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ makerAddress: makerAddressLeft, - makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), - takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(17), 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(98), 0), feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ makerAddress: makerAddressRight, makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), - makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(75), 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(13), 0), feeRecipientAddress: feeRecipientAddressRight, }); + // Assert is rounding error ceil & not rounding error floor + // These assertions are taken from MixinMatchOrders::calculateMatchedFillResults + // The rounding error is derived computating how much the left maker will sell. + const numerator = signedOrderLeft.makerAssetAmount; + const denominator = signedOrderLeft.takerAssetAmount; + const target = signedOrderRight.makerAssetAmount; + const isRoundingErrorCeil = await testExchange.publicIsRoundingErrorCeil.callAsync( + numerator, + denominator, + target, + ); + expect(isRoundingErrorCeil).to.be.true(); + const isRoundingErrorFloor = await testExchange.publicIsRoundingErrorFloor.callAsync( + numerator, + denominator, + target, + ); + expect(isRoundingErrorFloor).to.be.false(); // Match signedOrderLeft with signedOrderRight - await matchOrderTester.matchOrdersAndVerifyBalancesAsync( + // Note that the left maker received a slightly better sell price. + // This is intentional; see note in MixinMatchOrders.calculateMatchedFillResults. + // Because the left maker received a slightly more favorable sell price, the fee + // paid by the left taker is slightly higher than that paid by the left maker. + // Fees can be thought of as a tax paid by the seller, derived from the sale price. + const expectedTransferAmounts = { + // Left Maker + amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(13), 0), + amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(75), 0), + feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber('76.4705882352941176'), 16), // 76.47% + // Right Maker + amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(75), 0), + amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(13), 0), + feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Taker + amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(0), 0), + feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber('76.5306122448979591'), 16), // 76.53% + feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + }; + await matchOrderTester.matchOrdersAndAssertEffectsAsync( signedOrderLeft, signedOrderRight, takerAddress, erc20BalancesByOwner, erc721TokenIdsByOwner, + expectedTransferAmounts, ); - // Verify left order was fully filled - const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); - // Verify right order was fully filled - const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); }); - it('should transfer the correct amounts when orders completely fill each other and taker doesnt take a profit', async () => { + it('Should transfer correct amounts when left order is fully filled and values pass isRoundingErrorCeil but fail isRoundingErrorFloor', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ makerAddress: makerAddressLeft, - makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), - takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(15), 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(90), 0), feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ makerAddress: makerAddressRight, makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), - makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(97), 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(14), 0), feeRecipientAddress: feeRecipientAddressRight, }); - // Store original taker balance - const takerInitialBalances = _.cloneDeep(erc20BalancesByOwner[takerAddress][defaultERC20MakerAssetAddress]); + // Assert is rounding error floor & not rounding error ceil + // These assertions are taken from MixinMatchOrders::calculateMatchedFillResults + // The rounding error is derived computating how much the right maker will buy. + const numerator = signedOrderRight.takerAssetAmount; + const denominator = signedOrderRight.makerAssetAmount; + const target = signedOrderLeft.takerAssetAmount; + const isRoundingErrorFloor = await testExchange.publicIsRoundingErrorFloor.callAsync( + numerator, + denominator, + target, + ); + expect(isRoundingErrorFloor).to.be.true(); + const isRoundingErrorCeil = await testExchange.publicIsRoundingErrorCeil.callAsync( + numerator, + denominator, + target, + ); + expect(isRoundingErrorCeil).to.be.false(); // Match signedOrderLeft with signedOrderRight - let newERC20BalancesByOwner: ERC20BalancesByOwner; - let newERC721TokenIdsByOwner: ERC721TokenIdsByOwner; - // prettier-ignore - [ - newERC20BalancesByOwner, - // tslint:disable-next-line:trailing-comma - newERC721TokenIdsByOwner - ] = await matchOrderTester.matchOrdersAndVerifyBalancesAsync( + // Note that the right maker received a slightly better purchase price. + // This is intentional; see note in MixinMatchOrders.calculateMatchedFillResults. + // Because the right maker received a slightly more favorable buy price, the fee + // paid by the right taker is slightly higher than that paid by the right maker. + // Fees can be thought of as a tax paid by the seller, derived from the sale price. + const expectedTransferAmounts = { + // Left Maker + amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(15), 0), + amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(90), 0), + feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Right Maker + amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(90), 0), + amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(13), 0), + feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber('92.7835051546391752'), 16), // 92.78% + // Taker + amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 0), + feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber('92.8571428571428571'), 16), // 92.85% + }; + await matchOrderTester.matchOrdersAndAssertEffectsAsync( signedOrderLeft, signedOrderRight, takerAddress, erc20BalancesByOwner, erc721TokenIdsByOwner, - ); - // Verify left order was fully filled - const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); - // Verify right order was fully filled - const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); - // Verify taker did not take a profit - expect(takerInitialBalances).to.be.deep.equal( - newERC20BalancesByOwner[takerAddress][defaultERC20MakerAssetAddress], + expectedTransferAmounts, ); }); - it('should transfer the correct amounts when left order is completely filled and right order is partially filled', async () => { + it('Should give right maker a better buy price when rounding', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ makerAddress: makerAddressLeft, - makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), - takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(16), 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(22), 0), feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ makerAddress: makerAddressRight, makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), - makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(20), 18), - takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(4), 18), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(83), 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(49), 0), feeRecipientAddress: feeRecipientAddressRight, }); - // Match orders - await matchOrderTester.matchOrdersAndVerifyBalancesAsync( + // Note: + // The correct price buy price for the right maker would yield (49/83) * 22 = 12.988 units + // of the left maker asset. This gets rounded up to 13, giving the right maker a better price. + // Note: + // The maker/taker fee percentage paid on the right order differs because + // they received different sale prices. The right maker pays a + // fee slightly lower than the right taker. + const expectedTransferAmounts = { + // Left Maker + amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(16), 0), + amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(22), 0), + feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Right Maker + amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(22), 0), + amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(13), 0), + feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber('26.5060240963855421'), 16), // 26.506% + // Taker + amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(3), 0), + feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber('26.5306122448979591'), 16), // 26.531% + }; + // Match signedOrderLeft with signedOrderRight + await matchOrderTester.matchOrdersAndAssertEffectsAsync( signedOrderLeft, signedOrderRight, takerAddress, erc20BalancesByOwner, erc721TokenIdsByOwner, + expectedTransferAmounts, ); - // Verify left order was fully filled - const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); - // Verify right order was partially filled - const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FILLABLE); }); - it('should transfer the correct amounts when right order is completely filled and left order is partially filled', async () => { + 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(new BigNumber(50), 18), - takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(12), 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(97), 0), feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ makerAddress: makerAddressRight, makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), - makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(89), 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(1), 0), feeRecipientAddress: feeRecipientAddressRight, }); - // Match orders - await matchOrderTester.matchOrdersAndVerifyBalancesAsync( + // 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 + amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(11), 0), + amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(89), 0), + feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber('91.6666666666666666'), 16), // 91.6% + // Right Maker + amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(89), 0), + amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(1), 0), + feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Taker + amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 0), + feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber('91.7525773195876288'), 16), // 91.75% + feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + }; + // Match signedOrderLeft with signedOrderRight + await matchOrderTester.matchOrdersAndAssertEffectsAsync( signedOrderLeft, signedOrderRight, takerAddress, erc20BalancesByOwner, erc721TokenIdsByOwner, + expectedTransferAmounts, ); - // Verify left order was partially filled - const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FILLABLE); - // Verify right order was fully filled - const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); }); - it('should transfer the correct amounts when consecutive calls are used to completely fill the left order', async () => { + 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(new BigNumber(50), 18), - takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(1000), 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(1005), 0), feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ makerAddress: makerAddressRight, makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2126), 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(1063), 0), + feeRecipientAddress: feeRecipientAddressRight, + }); + const expectedTransferAmounts = { + // Left Maker + amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(1000), 0), + amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(1005), 0), + feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(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. + amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(1005), 0), + amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(503), 0), + feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber('47.2718720602069614'), 16), // 47.27% + // Taker + amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(497), 0), + feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber('47.3189087488240827'), 16), // 47.31% + }; + // Match signedOrderLeft with signedOrderRight + await matchOrderTester.matchOrdersAndAssertEffectsAsync( + signedOrderLeft, + signedOrderRight, + takerAddress, + erc20BalancesByOwner, + erc721TokenIdsByOwner, + expectedTransferAmounts, + ); + }); + + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow matchOrders to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + }); + const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ + makerAddress: makerAddressRight, + takerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), + feeRecipientAddress: feeRecipientAddressRight, + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await expectTransactionFailedAsync( + exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress), + RevertReason.TransferFailed, + ); + }); + }); + }; + describe('matchOrders reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + + it('should transfer the correct amounts when orders completely fill each other', async () => { + // Create orders to match + const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + }); + const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), + }); + // Match signedOrderLeft with signedOrderRight + const expectedTransferAmounts = { + // Left Maker + amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), + amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Right Maker + amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), + feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Taker + amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(3), 18), + feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + }; + await matchOrderTester.matchOrdersAndAssertEffectsAsync( + signedOrderLeft, + signedOrderRight, + takerAddress, + erc20BalancesByOwner, + erc721TokenIdsByOwner, + expectedTransferAmounts, + ); + }); + + it('should transfer the correct amounts when orders completely fill each other and taker doesnt take a profit', async () => { + // Create orders to match + const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + }); + const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), + }); + // Match signedOrderLeft with signedOrderRight + const expectedTransferAmounts = { + // Left Maker + amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), + amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Right Maker + amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), + feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Taker + amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(0), 18), + feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + }; + // Match signedOrderLeft with signedOrderRight + await matchOrderTester.matchOrdersAndAssertEffectsAsync( + signedOrderLeft, + signedOrderRight, + takerAddress, + erc20BalancesByOwner, + erc721TokenIdsByOwner, + expectedTransferAmounts, + ); + }); + + it('should transfer the correct amounts when left order is completely filled and right order is partially filled', async () => { + // Create orders to match + const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + }); + const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(20), 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(4), 18), + }); + // Match signedOrderLeft with signedOrderRight + const expectedTransferAmounts = { + // Left Maker + amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), + amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Right Maker + amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), + feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(50), 16), // 50% + // Taker + amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(3), 18), + feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber(50), 16), // 50% + }; + // Match signedOrderLeft with signedOrderRight + await matchOrderTester.matchOrdersAndAssertEffectsAsync( + signedOrderLeft, + signedOrderRight, + takerAddress, + erc20BalancesByOwner, + erc721TokenIdsByOwner, + expectedTransferAmounts, + ); + }); + + it('should transfer the correct amounts when right order is completely filled and left order is partially filled', async () => { + // Create orders to match + const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(50), 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), + }); + const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), + }); + // Match signedOrderLeft with signedOrderRight + const expectedTransferAmounts = { + // Left Maker + amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), + amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 16), // 10% + // Right Maker + amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), + feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Taker + amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(3), 18), + feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 16), // 10% + feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + }; + // Match signedOrderLeft with signedOrderRight + await matchOrderTester.matchOrdersAndAssertEffectsAsync( + signedOrderLeft, + signedOrderRight, + takerAddress, + erc20BalancesByOwner, + erc721TokenIdsByOwner, + expectedTransferAmounts, + ); + }); + + 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(new BigNumber(50), 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), + }); + const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders let newERC20BalancesByOwner: ERC20BalancesByOwner; let newERC721TokenIdsByOwner: ERC721TokenIdsByOwner; + const expectedTransferAmounts = { + // Left Maker + amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), + amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 16), // 10% + // Right Maker + amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), + feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Taker + amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(3), 18), + feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 16), // 10% + feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + }; // prettier-ignore [ newERC20BalancesByOwner, // tslint:disable-next-line:trailing-comma newERC721TokenIdsByOwner - ] = await matchOrderTester.matchOrdersAndVerifyBalancesAsync( + ] = await matchOrderTester.matchOrdersAndAssertEffectsAsync( signedOrderLeft, signedOrderRight, takerAddress, erc20BalancesByOwner, erc721TokenIdsByOwner, + expectedTransferAmounts, ); - // Verify left order was partially filled - const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FILLABLE); - // Verify right order was fully filled - const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); // 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({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(50), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match signedOrderLeft with signedOrderRight2 const leftTakerAssetFilledAmount = signedOrderRight.makerAssetAmount; const rightTakerAssetFilledAmount = new BigNumber(0); - await matchOrderTester.matchOrdersAndVerifyBalancesAsync( + const expectedTransferAmounts2 = { + // Left Maker + amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(45), 18), + amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(90), 18), + feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(90), 16), // 90% (10% paid earlier) + // Right Maker + amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(90), 18), + amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(45), 18), + feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(90), 16), // 90% + // Taker + amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(0), 18), + feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber(90), 16), // 90% (10% paid earlier) + feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber(90), 16), // 90% + }; + await matchOrderTester.matchOrdersAndAssertEffectsAsync( signedOrderLeft, signedOrderRight2, takerAddress, newERC20BalancesByOwner, - erc721TokenIdsByOwner, + newERC721TokenIdsByOwner, + expectedTransferAmounts2, leftTakerAssetFilledAmount, rightTakerAssetFilledAmount, ); - // Verify left order was fully filled - const leftOrderInfo2: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo2.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); - // Verify second right order was partially filled - const rightOrderInfo2: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight2); - expect(rightOrderInfo2.orderStatus as OrderStatus).to.be.equal(OrderStatus.FILLABLE); }); it('should transfer the correct amounts when consecutive calls are used to completely fill the right order', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(50), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders let newERC20BalancesByOwner: ERC20BalancesByOwner; let newERC721TokenIdsByOwner: ERC721TokenIdsByOwner; + const expectedTransferAmounts = { + // Left Maker + amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), + feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Right Maker + amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), + amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(4), 18), + feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(4), 16), // 4% + // Taker + amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(6), 18), + feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber(4), 16), // 4% + }; // prettier-ignore [ newERC20BalancesByOwner, // tslint:disable-next-line:trailing-comma newERC721TokenIdsByOwner - ] = await matchOrderTester.matchOrdersAndVerifyBalancesAsync( + ] = await matchOrderTester.matchOrdersAndAssertEffectsAsync( signedOrderLeft, signedOrderRight, takerAddress, erc20BalancesByOwner, erc721TokenIdsByOwner, + expectedTransferAmounts, ); - // Verify left order was partially filled - const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); - // Verify right order was fully filled - const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FILLABLE); + // Create second left order // Note: This order needs makerAssetAmount=96/takerAssetAmount=48 to fully fill the right order. // However, we use 100/50 to ensure a partial fill as we want to go down the "right fill" // branch in the contract twice for this test. const signedOrderLeft2 = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(50), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); // Match signedOrderLeft2 with signedOrderRight const leftTakerAssetFilledAmount = new BigNumber(0); @@ -421,198 +766,257 @@ describe('matchOrders', () => { erc20BalancesByOwner[takerAddress][defaultERC20MakerAssetAddress], ); const rightTakerAssetFilledAmount = signedOrderLeft.makerAssetAmount.minus(takerAmountReceived); - await matchOrderTester.matchOrdersAndVerifyBalancesAsync( + const expectedTransferAmounts2 = { + // Left Maker + amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(96), 18), + amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(48), 18), + feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(96), 16), // 96% + // Right Maker + amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(48), 18), + amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(96), 18), + feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(96), 16), // 96% + // Taker + amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(0), 18), + feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber(96), 16), // 96% + feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber(96), 16), // 96% + }; + await matchOrderTester.matchOrdersAndAssertEffectsAsync( signedOrderLeft2, signedOrderRight, takerAddress, newERC20BalancesByOwner, - erc721TokenIdsByOwner, + newERC721TokenIdsByOwner, + expectedTransferAmounts2, leftTakerAssetFilledAmount, rightTakerAssetFilledAmount, ); - // Verify second left order was partially filled - const leftOrderInfo2: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft2); - expect(leftOrderInfo2.orderStatus as OrderStatus).to.be.equal(OrderStatus.FILLABLE); - // Verify right order was fully filled - const rightOrderInfo2: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo2.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); }); it('should transfer the correct amounts if fee recipient is the same across both matched orders', async () => { const feeRecipientAddress = feeRecipientAddressLeft; const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), feeRecipientAddress, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), feeRecipientAddress, }); // Match orders - await matchOrderTester.matchOrdersAndVerifyBalancesAsync( + const expectedTransferAmounts = { + // Left Maker + amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), + amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Right Maker + amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), + feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Taker + amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(3), 18), + feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + }; + await matchOrderTester.matchOrdersAndAssertEffectsAsync( signedOrderLeft, signedOrderRight, takerAddress, erc20BalancesByOwner, erc721TokenIdsByOwner, + expectedTransferAmounts, ); }); it('should transfer the correct amounts if taker is also the left order maker', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders takerAddress = signedOrderLeft.makerAddress; - await matchOrderTester.matchOrdersAndVerifyBalancesAsync( + const expectedTransferAmounts = { + // Left Maker + amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), + amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Right Maker + amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), + feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Taker + amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(3), 18), + feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + }; + await matchOrderTester.matchOrdersAndAssertEffectsAsync( signedOrderLeft, signedOrderRight, takerAddress, erc20BalancesByOwner, erc721TokenIdsByOwner, + expectedTransferAmounts, ); }); it('should transfer the correct amounts if taker is also the right order maker', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders takerAddress = signedOrderRight.makerAddress; - await matchOrderTester.matchOrdersAndVerifyBalancesAsync( + const expectedTransferAmounts = { + // Left Maker + amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), + amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Right Maker + amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), + feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Taker + amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(3), 18), + feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + }; + await matchOrderTester.matchOrdersAndAssertEffectsAsync( signedOrderLeft, signedOrderRight, takerAddress, erc20BalancesByOwner, erc721TokenIdsByOwner, + expectedTransferAmounts, ); }); it('should transfer the correct amounts if taker is also the left fee recipient', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders takerAddress = feeRecipientAddressLeft; - await matchOrderTester.matchOrdersAndVerifyBalancesAsync( + const expectedTransferAmounts = { + // Left Maker + amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), + amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Right Maker + amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), + feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Taker + amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(3), 18), + feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + }; + await matchOrderTester.matchOrdersAndAssertEffectsAsync( signedOrderLeft, signedOrderRight, takerAddress, erc20BalancesByOwner, erc721TokenIdsByOwner, + expectedTransferAmounts, ); }); it('should transfer the correct amounts if taker is also the right fee recipient', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders takerAddress = feeRecipientAddressRight; - await matchOrderTester.matchOrdersAndVerifyBalancesAsync( + const expectedTransferAmounts = { + // Left Maker + amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), + amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Right Maker + amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), + feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Taker + amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(3), 18), + feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + }; + await matchOrderTester.matchOrdersAndAssertEffectsAsync( signedOrderLeft, signedOrderRight, takerAddress, erc20BalancesByOwner, erc721TokenIdsByOwner, + expectedTransferAmounts, ); }); it('should transfer the correct amounts if left maker is the left fee recipient and right maker is the right fee recipient', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: makerAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: makerAddressRight, }); // Match orders - await matchOrderTester.matchOrdersAndVerifyBalancesAsync( + const expectedTransferAmounts = { + // Left Maker + amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), + amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Right Maker + amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), + feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Taker + amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(3), 18), + feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + }; + await matchOrderTester.matchOrdersAndAssertEffectsAsync( signedOrderLeft, signedOrderRight, takerAddress, erc20BalancesByOwner, erc721TokenIdsByOwner, + expectedTransferAmounts, ); }); it('Should throw if left order is not fillable', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Cancel left order await exchangeWrapper.cancelOrderAsync(signedOrderLeft, signedOrderLeft.makerAddress); @@ -626,18 +1030,12 @@ describe('matchOrders', () => { it('Should throw if right order is not fillable', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Cancel right order await exchangeWrapper.cancelOrderAsync(signedOrderRight, signedOrderRight.makerAddress); @@ -651,18 +1049,12 @@ describe('matchOrders', () => { it('should throw if there is not a positive spread', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(1), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(200), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders return expectTransactionFailedAsync( @@ -674,18 +1066,13 @@ describe('matchOrders', () => { it('should throw if the left maker asset is not equal to the right taker asset ', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders return expectTransactionFailedAsync( @@ -701,20 +1088,13 @@ describe('matchOrders', () => { it('should throw if the right maker asset is not equal to the left taker asset', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders return expectTransactionFailedAsync( @@ -727,70 +1107,76 @@ describe('matchOrders', () => { // Create orders to match const erc721TokenToTransfer = erc721LeftMakerAssetIds[0]; const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetData: assetDataUtils.encodeERC721AssetData(defaultERC721AssetAddress, erc721TokenToTransfer), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), makerAssetAmount: new BigNumber(1), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), takerAssetData: assetDataUtils.encodeERC721AssetData(defaultERC721AssetAddress, erc721TokenToTransfer), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: new BigNumber(1), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders - await matchOrderTester.matchOrdersAndVerifyBalancesAsync( + const expectedTransferAmounts = { + // Left Maker + amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(1), 0), + amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Right Maker + amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(1), 0), + feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Taker + amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(0), 18), + feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 50% + }; + await matchOrderTester.matchOrdersAndAssertEffectsAsync( signedOrderLeft, signedOrderRight, takerAddress, erc20BalancesByOwner, erc721TokenIdsByOwner, + expectedTransferAmounts, ); - // Verify left order was fully filled - const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); - // Verify right order was fully filled - const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); }); it('should transfer correct amounts when right order maker asset is an ERC721 token', async () => { // Create orders to match const erc721TokenToTransfer = erc721RightMakerAssetIds[0]; const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), takerAssetData: assetDataUtils.encodeERC721AssetData(defaultERC721AssetAddress, erc721TokenToTransfer), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: new BigNumber(1), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, makerAssetData: assetDataUtils.encodeERC721AssetData(defaultERC721AssetAddress, erc721TokenToTransfer), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: new BigNumber(1), - takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressRight, + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(8), 18), }); // Match orders - await matchOrderTester.matchOrdersAndVerifyBalancesAsync( + const expectedTransferAmounts = { + // Left Maker + amountSoldByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + amountBoughtByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(1), 0), + feePaidByLeftMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Right Maker + amountSoldByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(1), 0), + amountBoughtByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(8), 18), + feePaidByRightMaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + // Taker + amountReceivedByTaker: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), + feePaidByTakerLeft: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + feePaidByTakerRight: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 16), // 100% + }; + await matchOrderTester.matchOrdersAndAssertEffectsAsync( signedOrderLeft, signedOrderRight, takerAddress, erc20BalancesByOwner, erc721TokenIdsByOwner, + expectedTransferAmounts, ); - // Verify left order was fully filled - const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); - // Verify right order was fully filled - const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); }); }); }); // tslint:disable-line:max-file-line-count diff --git a/packages/contracts/test/exchange/signature_validator.ts b/packages/contracts/test/exchange/signature_validator.ts index cd4f3d6f3..da2febfd8 100644 --- a/packages/contracts/test/exchange/signature_validator.ts +++ b/packages/contracts/test/exchange/signature_validator.ts @@ -9,11 +9,12 @@ import { TestSignatureValidatorContract, TestSignatureValidatorSignatureValidatorApprovalEventArgs, } from '../../generated_contract_wrappers/test_signature_validator'; +import { TestStaticCallReceiverContract } from '../../generated_contract_wrappers/test_static_call_receiver'; import { ValidatorContract } from '../../generated_contract_wrappers/validator'; import { WalletContract } from '../../generated_contract_wrappers/wallet'; import { addressUtils } from '../utils/address_utils'; import { artifacts } from '../utils/artifacts'; -import { expectContractCallFailed } from '../utils/assertions'; +import { expectContractCallFailed, expectContractCallFailedWithoutReasonAsync } from '../utils/assertions'; import { chaiSetup } from '../utils/chai_setup'; import { constants } from '../utils/constants'; import { LogDecoder } from '../utils/log_decoder'; @@ -31,6 +32,8 @@ describe('MixinSignatureValidator', () => { let signatureValidator: TestSignatureValidatorContract; let testWallet: WalletContract; let testValidator: ValidatorContract; + let maliciousWallet: TestStaticCallReceiverContract; + let maliciousValidator: TestStaticCallReceiverContract; let signerAddress: string; let signerPrivateKey: Buffer; let notSignerAddress: string; @@ -65,6 +68,11 @@ describe('MixinSignatureValidator', () => { txDefaults, signerAddress, ); + maliciousWallet = maliciousValidator = await TestStaticCallReceiverContract.deployFrom0xArtifactAsync( + artifacts.TestStaticCallReceiver, + provider, + txDefaults, + ); signatureValidatorLogDecoder = new LogDecoder(web3Wrapper); await web3Wrapper.awaitTransactionSuccessAsync( await signatureValidator.setSignatureValidatorApproval.sendTransactionAsync(testValidator.address, true, { @@ -72,6 +80,16 @@ describe('MixinSignatureValidator', () => { }), constants.AWAIT_TRANSACTION_MINED_MS, ); + await web3Wrapper.awaitTransactionSuccessAsync( + await signatureValidator.setSignatureValidatorApproval.sendTransactionAsync( + maliciousValidator.address, + true, + { + from: signerAddress, + }, + ), + constants.AWAIT_TRANSACTION_MINED_MS, + ); const defaultOrderParams = { ...constants.STATIC_ORDER_PARAMS, @@ -263,32 +281,6 @@ describe('MixinSignatureValidator', () => { expect(isValidSignature).to.be.false(); }); - it('should return true when SignatureType=Caller and signer is caller', async () => { - const signature = ethUtil.toBuffer(`0x${SignatureType.Caller}`); - const signatureHex = ethUtil.bufferToHex(signature); - const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); - const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - signerAddress, - signatureHex, - { from: signerAddress }, - ); - expect(isValidSignature).to.be.true(); - }); - - it('should return false when SignatureType=Caller and signer is not caller', async () => { - const signature = ethUtil.toBuffer(`0x${SignatureType.Caller}`); - const signatureHex = ethUtil.bufferToHex(signature); - const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); - const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - signerAddress, - signatureHex, - { from: notSignerAddress }, - ); - expect(isValidSignature).to.be.false(); - }); - it('should return true when SignatureType=Wallet and signature is valid', async () => { // Create EIP712 signature const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); @@ -334,6 +326,29 @@ describe('MixinSignatureValidator', () => { expect(isValidSignature).to.be.false(); }); + it('should revert when `isValidSignature` attempts to update state and SignatureType=Wallet', async () => { + // Create EIP712 signature + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); + const orderHashBuffer = ethUtil.toBuffer(orderHashHex); + const ecSignature = ethUtil.ecsign(orderHashBuffer, signerPrivateKey); + // Create 0x signature from EIP712 signature + const signature = Buffer.concat([ + ethUtil.toBuffer(ecSignature.v), + ecSignature.r, + ecSignature.s, + ethUtil.toBuffer(`0x${SignatureType.Wallet}`), + ]); + const signatureHex = ethUtil.bufferToHex(signature); + await expectContractCallFailed( + signatureValidator.publicIsValidSignature.callAsync( + orderHashHex, + maliciousWallet.address, + signatureHex, + ), + RevertReason.WalletError, + ); + }); + it('should return true when SignatureType=Validator, signature is valid and validator is approved', async () => { const validatorAddress = ethUtil.toBuffer(`${testValidator.address}`); const signatureType = ethUtil.toBuffer(`0x${SignatureType.Validator}`); @@ -364,6 +379,17 @@ describe('MixinSignatureValidator', () => { expect(isValidSignature).to.be.false(); }); + it('should revert when `isValidSignature` attempts to update state and SignatureType=Validator', async () => { + const validatorAddress = ethUtil.toBuffer(`${maliciousValidator.address}`); + const signatureType = ethUtil.toBuffer(`0x${SignatureType.Validator}`); + const signature = Buffer.concat([validatorAddress, signatureType]); + const signatureHex = ethUtil.bufferToHex(signature); + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); + await expectContractCallFailed( + signatureValidator.publicIsValidSignature.callAsync(orderHashHex, signerAddress, signatureHex), + RevertReason.ValidatorError, + ); + }); it('should return false when SignatureType=Validator, signature is valid and validator is not approved', async () => { // Set approval of signature validator to false await web3Wrapper.awaitTransactionSuccessAsync( @@ -388,53 +414,6 @@ describe('MixinSignatureValidator', () => { expect(isValidSignature).to.be.false(); }); - it('should return true when SignatureType=Trezor and signature is valid', async () => { - // Create Trezor signature - const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); - const orderHashWithTrezorPrefixHex = signatureUtils.addSignedMessagePrefix(orderHashHex, SignerType.Trezor); - const orderHashWithTrezorPrefixBuffer = ethUtil.toBuffer(orderHashWithTrezorPrefixHex); - const ecSignature = ethUtil.ecsign(orderHashWithTrezorPrefixBuffer, signerPrivateKey); - // Create 0x signature from Trezor signature - const signature = Buffer.concat([ - ethUtil.toBuffer(ecSignature.v), - ecSignature.r, - ecSignature.s, - ethUtil.toBuffer(`0x${SignatureType.Trezor}`), - ]); - const signatureHex = ethUtil.bufferToHex(signature); - // Validate signature - const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - signerAddress, - signatureHex, - ); - expect(isValidSignature).to.be.true(); - }); - - it('should return false when SignatureType=Trezor and signature is invalid', async () => { - // Create Trezor signature - const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); - const orderHashWithTrezorPrefixHex = signatureUtils.addSignedMessagePrefix(orderHashHex, SignerType.Trezor); - const orderHashWithTrezorPrefixBuffer = ethUtil.toBuffer(orderHashWithTrezorPrefixHex); - const ecSignature = ethUtil.ecsign(orderHashWithTrezorPrefixBuffer, signerPrivateKey); - // Create 0x signature from Trezor signature - const signature = Buffer.concat([ - ethUtil.toBuffer(ecSignature.v), - ecSignature.r, - ecSignature.s, - ethUtil.toBuffer(`0x${SignatureType.Trezor}`), - ]); - const signatureHex = ethUtil.bufferToHex(signature); - // Validate signature. - // This will fail because `signerAddress` signed the message, but we're passing in `notSignerAddress` - const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - notSignerAddress, - signatureHex, - ); - expect(isValidSignature).to.be.false(); - }); - it('should return true when SignatureType=Presigned and signer has presigned hash', async () => { // Presign hash const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); @@ -468,6 +447,42 @@ describe('MixinSignatureValidator', () => { ); expect(isValidSignature).to.be.false(); }); + + it('should return true when message was signed by a Trezor One (firmware version 1.6.2)', async () => { + // messageHash translates to 0x2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b + const messageHash = ethUtil.bufferToHex(ethUtil.toBuffer('++++++++++++++++++++++++++++++++')); + const signer = '0xc28b145f10f0bcf0fc000e778615f8fd73490bad'; + const v = ethUtil.toBuffer('0x1c'); + const r = ethUtil.toBuffer('0x7b888b596ccf87f0bacab0dcb483124973f7420f169b4824d7a12534ac1e9832'); + const s = ethUtil.toBuffer('0x0c8e14f7edc01459e13965f1da56e0c23ed11e2cca932571eee1292178f90424'); + const trezorSignatureType = ethUtil.toBuffer(`0x${SignatureType.EthSign}`); + const signature = Buffer.concat([v, r, s, trezorSignatureType]); + const signatureHex = ethUtil.bufferToHex(signature); + const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync( + messageHash, + signer, + signatureHex, + ); + expect(isValidSignature).to.be.true(); + }); + + it('should return true when message was signed by a Trezor Model T (firmware version 2.0.7)', async () => { + // messageHash translates to 0x2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b + const messageHash = ethUtil.bufferToHex(ethUtil.toBuffer('++++++++++++++++++++++++++++++++')); + const signer = '0x98ce6d9345e8ffa7d99ee0822272fae9d2c0e895'; + const v = ethUtil.toBuffer('0x1c'); + const r = ethUtil.toBuffer('0x423b71062c327f0ec4fe199b8da0f34185e59b4c1cb4cc23df86cac4a601fb3f'); + const s = ethUtil.toBuffer('0x53810d6591b5348b7ee08ee812c874b0fdfb942c9849d59512c90e295221091f'); + const trezorSignatureType = ethUtil.toBuffer(`0x${SignatureType.EthSign}`); + const signature = Buffer.concat([v, r, s, trezorSignatureType]); + const signatureHex = ethUtil.bufferToHex(signature); + const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync( + messageHash, + signer, + signatureHex, + ); + expect(isValidSignature).to.be.true(); + }); }); describe('setSignatureValidatorApproval', () => { diff --git a/packages/contracts/test/exchange/wrapper.ts b/packages/contracts/test/exchange/wrapper.ts index d48441dca..aadb5ab59 100644 --- a/packages/contracts/test/exchange/wrapper.ts +++ b/packages/contracts/test/exchange/wrapper.ts @@ -11,6 +11,7 @@ import { DummyERC721TokenContract } from '../../generated_contract_wrappers/dumm import { ERC20ProxyContract } from '../../generated_contract_wrappers/erc20_proxy'; import { ERC721ProxyContract } from '../../generated_contract_wrappers/erc721_proxy'; import { ExchangeContract } from '../../generated_contract_wrappers/exchange'; +import { ReentrantERC20TokenContract } from '../../generated_contract_wrappers/reentrant_erc20_token'; import { artifacts } from '../utils/artifacts'; import { expectTransactionFailedAsync } from '../utils/assertions'; import { getLatestBlockTimestampAsync, increaseTimeAndMineBlockAsync } from '../utils/block_timestamp'; @@ -40,6 +41,7 @@ describe('Exchange wrappers', () => { let exchange: ExchangeContract; let erc20Proxy: ERC20ProxyContract; let erc721Proxy: ERC721ProxyContract; + let reentrantErc20Token: ReentrantERC20TokenContract; let exchangeWrapper: ExchangeWrapper; let erc20Wrapper: ERC20Wrapper; @@ -104,6 +106,13 @@ describe('Exchange wrappers', () => { constants.AWAIT_TRANSACTION_MINED_MS, ); + reentrantErc20Token = await ReentrantERC20TokenContract.deployFrom0xArtifactAsync( + artifacts.ReentrantERC20Token, + provider, + txDefaults, + exchange.address, + ); + defaultMakerAssetAddress = erc20TokenA.address; defaultTakerAssetAddress = erc20TokenB.address; @@ -126,6 +135,26 @@ describe('Exchange wrappers', () => { await blockchainLifecycle.revertAsync(); }); describe('fillOrKillOrder', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow fillOrKillOrder to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await expectTransactionFailedAsync( + exchangeWrapper.fillOrKillOrderAsync(signedOrder, takerAddress), + RevertReason.TransferFailed, + ); + }); + }); + }; + describe('fillOrKillOrder reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should transfer the correct amounts', async () => { const signedOrder = await orderFactory.newSignedOrderAsync({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), @@ -197,6 +226,25 @@ describe('Exchange wrappers', () => { }); describe('fillOrderNoThrow', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow fillOrderNoThrow to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await exchangeWrapper.fillOrderNoThrowAsync(signedOrder, takerAddress); + const newBalances = await erc20Wrapper.getBalancesAsync(); + expect(erc20Balances).to.deep.equal(newBalances); + }); + }); + }; + describe('fillOrderNoThrow reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should transfer the correct amounts', async () => { const signedOrder = await orderFactory.newSignedOrderAsync({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), @@ -397,6 +445,26 @@ describe('Exchange wrappers', () => { }); describe('batchFillOrders', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow batchFillOrders to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await expectTransactionFailedAsync( + exchangeWrapper.batchFillOrdersAsync([signedOrder], takerAddress), + RevertReason.TransferFailed, + ); + }); + }); + }; + describe('batchFillOrders reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should transfer the correct amounts', async () => { const takerAssetFillAmounts: BigNumber[] = []; const makerAssetAddress = erc20TokenA.address; @@ -446,6 +514,26 @@ describe('Exchange wrappers', () => { }); describe('batchFillOrKillOrders', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow batchFillOrKillOrders to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await expectTransactionFailedAsync( + exchangeWrapper.batchFillOrKillOrdersAsync([signedOrder], takerAddress), + RevertReason.TransferFailed, + ); + }); + }); + }; + describe('batchFillOrKillOrders reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should transfer the correct amounts', async () => { const takerAssetFillAmounts: BigNumber[] = []; const makerAssetAddress = erc20TokenA.address; @@ -512,6 +600,25 @@ describe('Exchange wrappers', () => { }); describe('batchFillOrdersNoThrow', async () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow batchFillOrdersNoThrow to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await exchangeWrapper.batchFillOrdersNoThrowAsync([signedOrder], takerAddress); + const newBalances = await erc20Wrapper.getBalancesAsync(); + expect(erc20Balances).to.deep.equal(newBalances); + }); + }); + }; + describe('batchFillOrdersNoThrow reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should transfer the correct amounts', async () => { const takerAssetFillAmounts: BigNumber[] = []; const makerAssetAddress = erc20TokenA.address; @@ -625,6 +732,28 @@ describe('Exchange wrappers', () => { }); describe('marketSellOrders', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow marketSellOrders to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await expectTransactionFailedAsync( + exchangeWrapper.marketSellOrdersAsync([signedOrder], takerAddress, { + takerAssetFillAmount: signedOrder.takerAssetAmount, + }), + RevertReason.TransferFailed, + ); + }); + }); + }; + describe('marketSellOrders reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should stop when the entire takerAssetFillAmount is filled', async () => { const takerAssetFillAmount = signedOrders[0].takerAssetAmount.plus( signedOrders[1].takerAssetAmount.div(2), @@ -717,6 +846,27 @@ describe('Exchange wrappers', () => { }); describe('marketSellOrdersNoThrow', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow marketSellOrdersNoThrow to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await exchangeWrapper.marketSellOrdersNoThrowAsync([signedOrder], takerAddress, { + takerAssetFillAmount: signedOrder.takerAssetAmount, + }); + const newBalances = await erc20Wrapper.getBalancesAsync(); + expect(erc20Balances).to.deep.equal(newBalances); + }); + }); + }; + describe('marketSellOrdersNoThrow reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should stop when the entire takerAssetFillAmount is filled', async () => { const takerAssetFillAmount = signedOrders[0].takerAssetAmount.plus( signedOrders[1].takerAssetAmount.div(2), @@ -843,6 +993,28 @@ describe('Exchange wrappers', () => { }); describe('marketBuyOrders', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow marketBuyOrders to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await expectTransactionFailedAsync( + exchangeWrapper.marketBuyOrdersAsync([signedOrder], takerAddress, { + makerAssetFillAmount: signedOrder.makerAssetAmount, + }), + RevertReason.TransferFailed, + ); + }); + }); + }; + describe('marketBuyOrders reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should stop when the entire makerAssetFillAmount is filled', async () => { const makerAssetFillAmount = signedOrders[0].makerAssetAmount.plus( signedOrders[1].makerAssetAmount.div(2), @@ -933,6 +1105,27 @@ describe('Exchange wrappers', () => { }); describe('marketBuyOrdersNoThrow', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow marketBuyOrdersNoThrow to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await exchangeWrapper.marketBuyOrdersNoThrowAsync([signedOrder], takerAddress, { + makerAssetFillAmount: signedOrder.makerAssetAmount, + }); + const newBalances = await erc20Wrapper.getBalancesAsync(); + expect(erc20Balances).to.deep.equal(newBalances); + }); + }); + }; + describe('marketBuyOrdersNoThrow reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should stop when the entire makerAssetFillAmount is filled', async () => { const makerAssetFillAmount = signedOrders[0].makerAssetAmount.plus( signedOrders[1].makerAssetAmount.div(2), diff --git a/packages/contracts/test/utils/artifacts.ts b/packages/contracts/test/utils/artifacts.ts index e8a7585ac..5ddb5cc7f 100644 --- a/packages/contracts/test/utils/artifacts.ts +++ b/packages/contracts/test/utils/artifacts.ts @@ -16,6 +16,7 @@ import * as MixinAuthorizable from '../../artifacts/MixinAuthorizable.json'; import * as MultiSigWallet from '../../artifacts/MultiSigWallet.json'; import * as MultiSigWalletWithTimeLock from '../../artifacts/MultiSigWalletWithTimeLock.json'; import * as OrderValidator from '../../artifacts/OrderValidator.json'; +import * as ReentrantERC20Token from '../../artifacts/ReentrantERC20Token.json'; import * as TestAssetProxyDispatcher from '../../artifacts/TestAssetProxyDispatcher.json'; import * as TestAssetProxyOwner from '../../artifacts/TestAssetProxyOwner.json'; import * as TestConstants from '../../artifacts/TestConstants.json'; @@ -23,6 +24,7 @@ import * as TestExchangeInternals from '../../artifacts/TestExchangeInternals.js import * as TestLibBytes from '../../artifacts/TestLibBytes.json'; import * as TestLibs from '../../artifacts/TestLibs.json'; import * as TestSignatureValidator from '../../artifacts/TestSignatureValidator.json'; +import * as TestStaticCallReceiver from '../../artifacts/TestStaticCallReceiver.json'; import * as TokenRegistry from '../../artifacts/TokenRegistry.json'; import * as Validator from '../../artifacts/Validator.json'; import * as Wallet from '../../artifacts/Wallet.json'; @@ -48,6 +50,7 @@ export const artifacts = { MultiSigWallet: (MultiSigWallet as any) as ContractArtifact, MultiSigWalletWithTimeLock: (MultiSigWalletWithTimeLock as any) as ContractArtifact, OrderValidator: (OrderValidator as any) as ContractArtifact, + ReentrantERC20Token: (ReentrantERC20Token as any) as ContractArtifact, TestAssetProxyOwner: (TestAssetProxyOwner as any) as ContractArtifact, TestAssetProxyDispatcher: (TestAssetProxyDispatcher as any) as ContractArtifact, TestConstants: (TestConstants as any) as ContractArtifact, @@ -55,6 +58,7 @@ export const artifacts = { TestLibs: (TestLibs as any) as ContractArtifact, TestExchangeInternals: (TestExchangeInternals as any) as ContractArtifact, TestSignatureValidator: (TestSignatureValidator as any) as ContractArtifact, + TestStaticCallReceiver: (TestStaticCallReceiver as any) as ContractArtifact, Validator: (Validator as any) as ContractArtifact, Wallet: (Wallet as any) as ContractArtifact, TokenRegistry: (TokenRegistry as any) as ContractArtifact, diff --git a/packages/contracts/test/utils/constants.ts b/packages/contracts/test/utils/constants.ts index 65eaee398..ee4378d2e 100644 --- a/packages/contracts/test/utils/constants.ts +++ b/packages/contracts/test/utils/constants.ts @@ -51,4 +51,16 @@ export const constants = { WORD_LENGTH: 32, ZERO_AMOUNT: new BigNumber(0), PERCENTAGE_DENOMINATOR: new BigNumber(10).pow(18), + FUNCTIONS_WITH_MUTEX: [ + 'FILL_ORDER', + 'FILL_OR_KILL_ORDER', + 'BATCH_FILL_ORDERS', + 'BATCH_FILL_OR_KILL_ORDERS', + 'MARKET_BUY_ORDERS', + 'MARKET_SELL_ORDERS', + 'MATCH_ORDERS', + 'CANCEL_ORDER', + 'CANCEL_ORDERS_UP_TO', + 'SET_SIGNATURE_VALIDATOR_APPROVAL', + ], }; diff --git a/packages/contracts/test/utils/fill_order_combinatorial_utils.ts b/packages/contracts/test/utils/fill_order_combinatorial_utils.ts index a9318571c..92d0f4003 100644 --- a/packages/contracts/test/utils/fill_order_combinatorial_utils.ts +++ b/packages/contracts/test/utils/fill_order_combinatorial_utils.ts @@ -467,17 +467,17 @@ export class FillOrderCombinatorialUtils { ? remainingTakerAmountToFill : alreadyFilledTakerAmount.add(takerAssetFillAmount); - const expFilledMakerAmount = orderUtils.getPartialAmount( + const expFilledMakerAmount = orderUtils.getPartialAmountFloor( expFilledTakerAmount, signedOrder.takerAssetAmount, signedOrder.makerAssetAmount, ); - const expMakerFeePaid = orderUtils.getPartialAmount( + const expMakerFeePaid = orderUtils.getPartialAmountFloor( expFilledTakerAmount, signedOrder.takerAssetAmount, signedOrder.makerFee, ); - const expTakerFeePaid = orderUtils.getPartialAmount( + const expTakerFeePaid = orderUtils.getPartialAmountFloor( expFilledTakerAmount, signedOrder.takerAssetAmount, signedOrder.takerFee, @@ -668,7 +668,7 @@ export class FillOrderCombinatorialUtils { signedOrder: SignedOrder, takerAssetFillAmount: BigNumber, ): Promise<void> { - const makerAssetFillAmount = orderUtils.getPartialAmount( + const makerAssetFillAmount = orderUtils.getPartialAmountFloor( takerAssetFillAmount, signedOrder.takerAssetAmount, signedOrder.makerAssetAmount, @@ -705,7 +705,7 @@ export class FillOrderCombinatorialUtils { ); } - const makerFee = orderUtils.getPartialAmount( + const makerFee = orderUtils.getPartialAmountFloor( takerAssetFillAmount, signedOrder.takerAssetAmount, signedOrder.makerFee, @@ -829,7 +829,7 @@ export class FillOrderCombinatorialUtils { ); } - const takerFee = orderUtils.getPartialAmount( + const takerFee = orderUtils.getPartialAmountFloor( takerAssetFillAmount, signedOrder.takerAssetAmount, signedOrder.takerFee, diff --git a/packages/contracts/test/utils/match_order_tester.ts b/packages/contracts/test/utils/match_order_tester.ts index fa2eabc12..e0c55b834 100644 --- a/packages/contracts/test/utils/match_order_tester.ts +++ b/packages/contracts/test/utils/match_order_tester.ts @@ -4,11 +4,20 @@ import { BigNumber } from '@0xproject/utils'; import * as chai from 'chai'; import * as _ from 'lodash'; +import { TransactionReceiptWithDecodedLogs } from '../../../../node_modules/ethereum-types'; + import { chaiSetup } from './chai_setup'; import { ERC20Wrapper } from './erc20_wrapper'; import { ERC721Wrapper } from './erc721_wrapper'; import { ExchangeWrapper } from './exchange_wrapper'; -import { ERC20BalancesByOwner, ERC721TokenIdsByOwner, TransferAmountsByMatchOrders as TransferAmounts } from './types'; +import { + ERC20BalancesByOwner, + ERC721TokenIdsByOwner, + OrderInfo, + OrderStatus, + TransferAmountsByMatchOrders as TransferAmounts, + TransferAmountsLoggedByMatchOrders as LoggedTransferAmounts, +} from './types'; chaiSetup.configure(); const expect = chai.expect; @@ -18,43 +27,107 @@ export class MatchOrderTester { private readonly _erc20Wrapper: ERC20Wrapper; private readonly _erc721Wrapper: ERC721Wrapper; private readonly _feeTokenAddress: string; - - /// @dev Compares a pair of ERC20 balances and a pair of ERC721 token owners. - /// @param expectedNewERC20BalancesByOwner Expected ERC20 balances. - /// @param realERC20BalancesByOwner Actual ERC20 balances. - /// @param expectedNewERC721TokenIdsByOwner Expected ERC721 token owners. - /// @param realERC721TokenIdsByOwner Actual ERC20 token owners. - /// @return True only if ERC20 balances match and ERC721 token owners match. - private static _compareExpectedAndRealBalances( - expectedNewERC20BalancesByOwner: ERC20BalancesByOwner, + /// @dev Checks values from the logs produced by Exchange.matchOrders against the expected transfer amounts. + /// Values include the amounts transferred from the left/right makers and taker, along with + /// the fees paid on each matched order. These are also the return values of MatchOrders. + /// @param signedOrderLeft First matched order. + /// @param signedOrderRight Second matched order. + /// @param transactionReceipt Transaction receipt and logs produced by Exchange.matchOrders. + /// @param takerAddress Address of taker (account that called Exchange.matchOrders) + /// @param expectedTransferAmounts Expected amounts transferred as a result of order matching. + private static async _assertLogsAsync( + signedOrderLeft: SignedOrder, + signedOrderRight: SignedOrder, + transactionReceipt: TransactionReceiptWithDecodedLogs, + takerAddress: string, + expectedTransferAmounts: TransferAmounts, + ): Promise<void> { + // Should have two fill event logs -- one for each order. + const transactionFillLogs = _.filter(transactionReceipt.logs, ['event', 'Fill']); + expect(transactionFillLogs.length, 'Checking number of logs').to.be.equal(2); + // First log is for left fill + const leftLog = (transactionFillLogs[0] as any).args as LoggedTransferAmounts; + expect(leftLog.makerAddress, 'Checking logged maker address of left order').to.be.equal( + signedOrderLeft.makerAddress, + ); + expect(leftLog.takerAddress, 'Checking logged taker address of right order').to.be.equal(takerAddress); + const amountBoughtByLeftMaker = new BigNumber(leftLog.takerAssetFilledAmount); + const amountSoldByLeftMaker = new BigNumber(leftLog.makerAssetFilledAmount); + const feePaidByLeftMaker = new BigNumber(leftLog.makerFeePaid); + const feePaidByTakerLeft = new BigNumber(leftLog.takerFeePaid); + // Second log is for right fill + const rightLog = (transactionFillLogs[1] as any).args as LoggedTransferAmounts; + expect(rightLog.makerAddress, 'Checking logged maker address of right order').to.be.equal( + signedOrderRight.makerAddress, + ); + expect(rightLog.takerAddress, 'Checking loggerd taker address of right order').to.be.equal(takerAddress); + const amountBoughtByRightMaker = new BigNumber(rightLog.takerAssetFilledAmount); + const amountSoldByRightMaker = new BigNumber(rightLog.makerAssetFilledAmount); + const feePaidByRightMaker = new BigNumber(rightLog.makerFeePaid); + const feePaidByTakerRight = new BigNumber(rightLog.takerFeePaid); + // Derive amount received by taker + const amountReceivedByTaker = amountSoldByLeftMaker.sub(amountBoughtByRightMaker); + // Assert log values - left order + expect(amountBoughtByLeftMaker, 'Checking logged amount bought by left maker').to.be.bignumber.equal( + expectedTransferAmounts.amountBoughtByLeftMaker, + ); + expect(amountSoldByLeftMaker, 'Checking logged amount sold by left maker').to.be.bignumber.equal( + expectedTransferAmounts.amountSoldByLeftMaker, + ); + expect(feePaidByLeftMaker, 'Checking logged fee paid by left maker').to.be.bignumber.equal( + expectedTransferAmounts.feePaidByLeftMaker, + ); + expect(feePaidByTakerLeft, 'Checking logged fee paid on left order by taker').to.be.bignumber.equal( + expectedTransferAmounts.feePaidByTakerLeft, + ); + // Assert log values - right order + expect(amountBoughtByRightMaker, 'Checking logged amount bought by right maker').to.be.bignumber.equal( + expectedTransferAmounts.amountBoughtByRightMaker, + ); + expect(amountSoldByRightMaker, 'Checking logged amount sold by right maker').to.be.bignumber.equal( + expectedTransferAmounts.amountSoldByRightMaker, + ); + expect(feePaidByRightMaker, 'Checking logged fee paid by right maker').to.be.bignumber.equal( + expectedTransferAmounts.feePaidByRightMaker, + ); + expect(feePaidByTakerRight, 'Checking logged fee paid on right order by taker').to.be.bignumber.equal( + expectedTransferAmounts.feePaidByTakerRight, + ); + // Assert derived amount received by taker + expect(amountReceivedByTaker, 'Checking logged amount received by taker').to.be.bignumber.equal( + expectedTransferAmounts.amountReceivedByTaker, + ); + } + /// @dev Asserts all expected ERC20 and ERC721 account holdings match the real holdings. + /// @param expectedERC20BalancesByOwner Expected ERC20 balances. + /// @param realERC20BalancesByOwner Real ERC20 balances. + /// @param expectedERC721TokenIdsByOwner Expected ERC721 token owners. + /// @param realERC721TokenIdsByOwner Real ERC20 token owners. + private static async _assertAllKnownBalancesAsync( + expectedERC20BalancesByOwner: ERC20BalancesByOwner, realERC20BalancesByOwner: ERC20BalancesByOwner, - expectedNewERC721TokenIdsByOwner: ERC721TokenIdsByOwner, + expectedERC721TokenIdsByOwner: ERC721TokenIdsByOwner, realERC721TokenIdsByOwner: ERC721TokenIdsByOwner, - ): boolean { + ): Promise<void> { // ERC20 Balances - const doesErc20BalancesMatch = _.isEqual(expectedNewERC20BalancesByOwner, realERC20BalancesByOwner); - if (!doesErc20BalancesMatch) { - return false; - } + const areERC20BalancesEqual = _.isEqual(expectedERC20BalancesByOwner, realERC20BalancesByOwner); + expect(areERC20BalancesEqual, 'Checking all known ERC20 account balances').to.be.true(); // ERC721 Token Ids - const sortedExpectedNewERC721TokenIdsByOwner = _.mapValues( - expectedNewERC721TokenIdsByOwner, - tokenIdsByOwner => { - _.mapValues(tokenIdsByOwner, tokenIds => { - _.sortBy(tokenIds); - }); - }, - ); + const sortedExpectedNewERC721TokenIdsByOwner = _.mapValues(expectedERC721TokenIdsByOwner, tokenIdsByOwner => { + _.mapValues(tokenIdsByOwner, tokenIds => { + _.sortBy(tokenIds); + }); + }); const sortedNewERC721TokenIdsByOwner = _.mapValues(realERC721TokenIdsByOwner, tokenIdsByOwner => { _.mapValues(tokenIdsByOwner, tokenIds => { _.sortBy(tokenIds); }); }); - const doesErc721TokenIdsMatch = _.isEqual( + const areERC721TokenIdsEqual = _.isEqual( sortedExpectedNewERC721TokenIdsByOwner, sortedNewERC721TokenIdsByOwner, ); - return doesErc721TokenIdsMatch; + expect(areERC721TokenIdsEqual, 'Checking all known ERC721 account balances').to.be.true(); } /// @dev Constructs new MatchOrderTester. /// @param exchangeWrapper Used to call to the Exchange. @@ -72,150 +145,199 @@ export class MatchOrderTester { this._erc721Wrapper = erc721Wrapper; this._feeTokenAddress = feeTokenAddress; } - /// @dev Matches two complementary orders and validates results. - /// Validation either succeeds or throws. + /// @dev Matches two complementary orders and asserts results. /// @param signedOrderLeft First matched order. /// @param signedOrderRight Second matched order. /// @param takerAddress Address of taker (the address who matched the two orders) /// @param erc20BalancesByOwner Current ERC20 balances. /// @param erc721TokenIdsByOwner Current ERC721 token owners. - /// @param initialTakerAssetFilledAmountLeft Current amount the left order has been filled. - /// @param initialTakerAssetFilledAmountRight Current amount the right order has been filled. + /// @param expectedTransferAmounts Expected amounts transferred as a result of order matching. + /// @param initialLeftOrderFilledAmount How much left order has been filled, prior to matching orders. + /// @param initialRightOrderFilledAmount How much the right order has been filled, prior to matching orders. /// @return New ERC20 balances & ERC721 token owners. - public async matchOrdersAndVerifyBalancesAsync( + public async matchOrdersAndAssertEffectsAsync( signedOrderLeft: SignedOrder, signedOrderRight: SignedOrder, takerAddress: string, erc20BalancesByOwner: ERC20BalancesByOwner, erc721TokenIdsByOwner: ERC721TokenIdsByOwner, - initialTakerAssetFilledAmountLeft?: BigNumber, - initialTakerAssetFilledAmountRight?: BigNumber, + expectedTransferAmounts: TransferAmounts, + initialLeftOrderFilledAmount: BigNumber = new BigNumber(0), + initialRightOrderFilledAmount: BigNumber = new BigNumber(0), ): Promise<[ERC20BalancesByOwner, ERC721TokenIdsByOwner]> { - // Verify Left order preconditions - const orderTakerAssetFilledAmountLeft = await this._exchangeWrapper.getTakerAssetFilledAmountAsync( - orderHashUtils.getOrderHashHex(signedOrderLeft), - ); - const expectedOrderFilledAmountLeft = initialTakerAssetFilledAmountLeft - ? initialTakerAssetFilledAmountLeft - : new BigNumber(0); - expect(expectedOrderFilledAmountLeft).to.be.bignumber.equal(orderTakerAssetFilledAmountLeft); - // Verify Right order preconditions - const orderTakerAssetFilledAmountRight = await this._exchangeWrapper.getTakerAssetFilledAmountAsync( - orderHashUtils.getOrderHashHex(signedOrderRight), + // Assert initial order states + await this._assertInitialOrderStatesAsync( + signedOrderLeft, + signedOrderRight, + initialLeftOrderFilledAmount, + initialRightOrderFilledAmount, ); - const expectedOrderFilledAmountRight = initialTakerAssetFilledAmountRight - ? initialTakerAssetFilledAmountRight - : new BigNumber(0); - expect(expectedOrderFilledAmountRight).to.be.bignumber.equal(orderTakerAssetFilledAmountRight); // Match left & right orders - await this._exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress); + const transactionReceipt = await this._exchangeWrapper.matchOrdersAsync( + signedOrderLeft, + signedOrderRight, + takerAddress, + ); const newERC20BalancesByOwner = await this._erc20Wrapper.getBalancesAsync(); const newERC721TokenIdsByOwner = await this._erc721Wrapper.getBalancesAsync(); - // Calculate expected balance changes - const expectedTransferAmounts = await this._calculateExpectedTransferAmountsAsync( + // Assert logs + await MatchOrderTester._assertLogsAsync( signedOrderLeft, signedOrderRight, - orderTakerAssetFilledAmountLeft, - orderTakerAssetFilledAmountRight, + transactionReceipt, + takerAddress, + expectedTransferAmounts, ); - let expectedERC20BalancesByOwner: ERC20BalancesByOwner; - let expectedERC721TokenIdsByOwner: ERC721TokenIdsByOwner; - [expectedERC20BalancesByOwner, expectedERC721TokenIdsByOwner] = this._calculateExpectedBalances( + // Assert exchange state + await this._assertExchangeStateAsync( signedOrderLeft, signedOrderRight, - takerAddress, - erc20BalancesByOwner, - erc721TokenIdsByOwner, + initialLeftOrderFilledAmount, + initialRightOrderFilledAmount, expectedTransferAmounts, ); - // Assert our expected balances are equal to the actual balances - const didExpectedBalancesMatchRealBalances = MatchOrderTester._compareExpectedAndRealBalances( - expectedERC20BalancesByOwner, + // Assert balances of makers, taker, and fee recipients + await this._assertBalancesAsync( + signedOrderLeft, + signedOrderRight, + erc20BalancesByOwner, + erc721TokenIdsByOwner, newERC20BalancesByOwner, - expectedERC721TokenIdsByOwner, newERC721TokenIdsByOwner, + expectedTransferAmounts, + takerAddress, ); - expect(didExpectedBalancesMatchRealBalances).to.be.true(); return [newERC20BalancesByOwner, newERC721TokenIdsByOwner]; } - /// @dev Calculates expected transfer amounts between order makers, fee recipients, and - /// the taker when two orders are matched. + /// @dev Asserts initial exchange state for the left and right orders. + /// @param signedOrderLeft First matched order. + /// @param signedOrderRight Second matched order. + /// @param expectedOrderFilledAmountLeft How much left order has been filled, prior to matching orders. + /// @param expectedOrderFilledAmountRight How much the right order has been filled, prior to matching orders. + private async _assertInitialOrderStatesAsync( + signedOrderLeft: SignedOrder, + signedOrderRight: SignedOrder, + expectedOrderFilledAmountLeft: BigNumber, + expectedOrderFilledAmountRight: BigNumber, + ): Promise<void> { + // Assert left order initial state + const orderTakerAssetFilledAmountLeft = await this._exchangeWrapper.getTakerAssetFilledAmountAsync( + orderHashUtils.getOrderHashHex(signedOrderLeft), + ); + expect(orderTakerAssetFilledAmountLeft, 'Checking inital state of left order').to.be.bignumber.equal( + expectedOrderFilledAmountLeft, + ); + // Assert right order initial state + const orderTakerAssetFilledAmountRight = await this._exchangeWrapper.getTakerAssetFilledAmountAsync( + orderHashUtils.getOrderHashHex(signedOrderRight), + ); + expect(orderTakerAssetFilledAmountRight, 'Checking inital state of right order').to.be.bignumber.equal( + expectedOrderFilledAmountRight, + ); + } + /// @dev Asserts the exchange state against the expected amounts transferred by from matching orders. /// @param signedOrderLeft First matched order. /// @param signedOrderRight Second matched order. - /// @param orderTakerAssetFilledAmountLeft How much left order has been filled, prior to matching orders. - /// @param orderTakerAssetFilledAmountRight How much the right order has been filled, prior to matching orders. + /// @param initialLeftOrderFilledAmount How much left order has been filled, prior to matching orders. + /// @param initialRightOrderFilledAmount How much the right order has been filled, prior to matching orders. /// @return TransferAmounts A struct containing the expected transfer amounts. - private async _calculateExpectedTransferAmountsAsync( + private async _assertExchangeStateAsync( signedOrderLeft: SignedOrder, signedOrderRight: SignedOrder, - orderTakerAssetFilledAmountLeft: BigNumber, - orderTakerAssetFilledAmountRight: BigNumber, - ): Promise<TransferAmounts> { + initialLeftOrderFilledAmount: BigNumber, + initialRightOrderFilledAmount: BigNumber, + expectedTransferAmounts: TransferAmounts, + ): Promise<void> { + // Assert state for left order: amount bought by left maker let amountBoughtByLeftMaker = await this._exchangeWrapper.getTakerAssetFilledAmountAsync( orderHashUtils.getOrderHashHex(signedOrderLeft), ); - amountBoughtByLeftMaker = amountBoughtByLeftMaker.minus(orderTakerAssetFilledAmountLeft); - const amountSoldByLeftMaker = amountBoughtByLeftMaker - .times(signedOrderLeft.makerAssetAmount) - .dividedToIntegerBy(signedOrderLeft.takerAssetAmount); - const amountReceivedByRightMaker = amountBoughtByLeftMaker - .times(signedOrderRight.takerAssetAmount) - .dividedToIntegerBy(signedOrderRight.makerAssetAmount); - const amountReceivedByTaker = amountSoldByLeftMaker.minus(amountReceivedByRightMaker); + amountBoughtByLeftMaker = amountBoughtByLeftMaker.minus(initialLeftOrderFilledAmount); + expect(amountBoughtByLeftMaker, 'Checking exchange state for left order').to.be.bignumber.equal( + expectedTransferAmounts.amountBoughtByLeftMaker, + ); + // Assert state for right order: amount bought by right maker let amountBoughtByRightMaker = await this._exchangeWrapper.getTakerAssetFilledAmountAsync( orderHashUtils.getOrderHashHex(signedOrderRight), ); - amountBoughtByRightMaker = amountBoughtByRightMaker.minus(orderTakerAssetFilledAmountRight); - const amountSoldByRightMaker = amountBoughtByRightMaker - .times(signedOrderRight.makerAssetAmount) - .dividedToIntegerBy(signedOrderRight.takerAssetAmount); - const amountReceivedByLeftMaker = amountSoldByRightMaker; - const feePaidByLeftMaker = signedOrderLeft.makerFee - .times(amountSoldByLeftMaker) - .dividedToIntegerBy(signedOrderLeft.makerAssetAmount); - const feePaidByRightMaker = signedOrderRight.makerFee - .times(amountSoldByRightMaker) - .dividedToIntegerBy(signedOrderRight.makerAssetAmount); - const feePaidByTakerLeft = signedOrderLeft.takerFee - .times(amountSoldByLeftMaker) - .dividedToIntegerBy(signedOrderLeft.makerAssetAmount); - const feePaidByTakerRight = signedOrderRight.takerFee - .times(amountSoldByRightMaker) - .dividedToIntegerBy(signedOrderRight.makerAssetAmount); - const totalFeePaidByTaker = feePaidByTakerLeft.add(feePaidByTakerRight); - const feeReceivedLeft = feePaidByLeftMaker.add(feePaidByTakerLeft); - const feeReceivedRight = feePaidByRightMaker.add(feePaidByTakerRight); - // Return values - const expectedTransferAmounts = { - // Left Maker - amountBoughtByLeftMaker, - amountSoldByLeftMaker, - amountReceivedByLeftMaker, - feePaidByLeftMaker, - // Right Maker - amountBoughtByRightMaker, - amountSoldByRightMaker, - amountReceivedByRightMaker, - feePaidByRightMaker, - // Taker - amountReceivedByTaker, - feePaidByTakerLeft, - feePaidByTakerRight, - totalFeePaidByTaker, - // Fee Recipients - feeReceivedLeft, - feeReceivedRight, - }; - return expectedTransferAmounts; + amountBoughtByRightMaker = amountBoughtByRightMaker.minus(initialRightOrderFilledAmount); + expect(amountBoughtByRightMaker, 'Checking exchange state for right order').to.be.bignumber.equal( + expectedTransferAmounts.amountBoughtByRightMaker, + ); + // Assert left order status + const maxAmountBoughtByLeftMaker = signedOrderLeft.takerAssetAmount.minus(initialLeftOrderFilledAmount); + const leftOrderInfo: OrderInfo = await this._exchangeWrapper.getOrderInfoAsync(signedOrderLeft); + const leftExpectedStatus = expectedTransferAmounts.amountBoughtByLeftMaker.equals(maxAmountBoughtByLeftMaker) + ? OrderStatus.FULLY_FILLED + : OrderStatus.FILLABLE; + expect(leftOrderInfo.orderStatus as OrderStatus, 'Checking exchange status for left order').to.be.equal( + leftExpectedStatus, + ); + // Assert right order status + const maxAmountBoughtByRightMaker = signedOrderRight.takerAssetAmount.minus(initialRightOrderFilledAmount); + const rightOrderInfo: OrderInfo = await this._exchangeWrapper.getOrderInfoAsync(signedOrderRight); + const rightExpectedStatus = expectedTransferAmounts.amountBoughtByRightMaker.equals(maxAmountBoughtByRightMaker) + ? OrderStatus.FULLY_FILLED + : OrderStatus.FILLABLE; + expect(rightOrderInfo.orderStatus as OrderStatus, 'Checking exchange status for right order').to.be.equal( + rightExpectedStatus, + ); + } + /// @dev Asserts account balances after matching orders. + /// @param signedOrderLeft First matched order. + /// @param signedOrderRight Second matched order. + /// @param initialERC20BalancesByOwner ERC20 balances prior to order matching. + /// @param initialERC721TokenIdsByOwner ERC721 token owners prior to order matching. + /// @param finalERC20BalancesByOwner ERC20 balances after order matching. + /// @param finalERC721TokenIdsByOwner ERC721 token owners after order matching. + /// @param expectedTransferAmounts Expected amounts transferred as a result of order matching. + /// @param takerAddress Address of taker (account that called Exchange.matchOrders). + private async _assertBalancesAsync( + signedOrderLeft: SignedOrder, + signedOrderRight: SignedOrder, + initialERC20BalancesByOwner: ERC20BalancesByOwner, + initialERC721TokenIdsByOwner: ERC721TokenIdsByOwner, + finalERC20BalancesByOwner: ERC20BalancesByOwner, + finalERC721TokenIdsByOwner: ERC721TokenIdsByOwner, + expectedTransferAmounts: TransferAmounts, + takerAddress: string, + ): Promise<void> { + let expectedERC20BalancesByOwner: ERC20BalancesByOwner; + let expectedERC721TokenIdsByOwner: ERC721TokenIdsByOwner; + [expectedERC20BalancesByOwner, expectedERC721TokenIdsByOwner] = this._calculateExpectedBalances( + signedOrderLeft, + signedOrderRight, + takerAddress, + initialERC20BalancesByOwner, + initialERC721TokenIdsByOwner, + expectedTransferAmounts, + ); + // Assert balances of makers, taker, and fee recipients + await this._assertMakerTakerAndFeeRecipientBalancesAsync( + signedOrderLeft, + signedOrderRight, + expectedERC20BalancesByOwner, + finalERC20BalancesByOwner, + expectedERC721TokenIdsByOwner, + finalERC721TokenIdsByOwner, + takerAddress, + ); + // Assert balances for all known accounts + await MatchOrderTester._assertAllKnownBalancesAsync( + expectedERC20BalancesByOwner, + finalERC20BalancesByOwner, + expectedERC721TokenIdsByOwner, + finalERC721TokenIdsByOwner, + ); } /// @dev Calculates the expected balances of order makers, fee recipients, and the taker, /// as a result of matching two orders. - /// @param signedOrderLeft First matched order. + /// @param signedOrderRight First matched order. /// @param signedOrderRight Second matched order. /// @param takerAddress Address of taker (the address who matched the two orders) /// @param erc20BalancesByOwner Current ERC20 balances. /// @param erc721TokenIdsByOwner Current ERC721 token owners. - /// @param expectedTransferAmounts A struct containing the expected transfer amounts. + /// @param expectedTransferAmounts Expected amounts transferred as a result of order matching. /// @return Expected ERC20 balances & ERC721 token owners after orders have been matched. private _calculateExpectedBalances( signedOrderLeft: SignedOrder, @@ -247,7 +369,7 @@ export class MatchOrderTester { expectedNewERC20BalancesByOwner[makerAddressRight][ takerAssetAddressRight ] = expectedNewERC20BalancesByOwner[makerAddressRight][takerAssetAddressRight].add( - expectedTransferAmounts.amountReceivedByRightMaker, + expectedTransferAmounts.amountBoughtByRightMaker, ); // Taker expectedNewERC20BalancesByOwner[takerAddress][makerAssetAddressLeft] = expectedNewERC20BalancesByOwner[ @@ -277,7 +399,7 @@ export class MatchOrderTester { // Left Maker expectedNewERC20BalancesByOwner[makerAddressLeft][takerAssetAddressLeft] = expectedNewERC20BalancesByOwner[ makerAddressLeft - ][takerAssetAddressLeft].add(expectedTransferAmounts.amountReceivedByLeftMaker); + ][takerAssetAddressLeft].add(expectedTransferAmounts.amountBoughtByLeftMaker); // Right Maker expectedNewERC20BalancesByOwner[makerAddressRight][ makerAssetAddressRight @@ -307,20 +429,138 @@ export class MatchOrderTester { // Taker Fees expectedNewERC20BalancesByOwner[takerAddress][this._feeTokenAddress] = expectedNewERC20BalancesByOwner[ takerAddress - ][this._feeTokenAddress].minus(expectedTransferAmounts.totalFeePaidByTaker); + ][this._feeTokenAddress].minus( + expectedTransferAmounts.feePaidByTakerLeft.add(expectedTransferAmounts.feePaidByTakerRight), + ); // Left Fee Recipient Fees expectedNewERC20BalancesByOwner[feeRecipientAddressLeft][ this._feeTokenAddress ] = expectedNewERC20BalancesByOwner[feeRecipientAddressLeft][this._feeTokenAddress].add( - expectedTransferAmounts.feeReceivedLeft, + expectedTransferAmounts.feePaidByLeftMaker.add(expectedTransferAmounts.feePaidByTakerLeft), ); // Right Fee Recipient Fees expectedNewERC20BalancesByOwner[feeRecipientAddressRight][ this._feeTokenAddress ] = expectedNewERC20BalancesByOwner[feeRecipientAddressRight][this._feeTokenAddress].add( - expectedTransferAmounts.feeReceivedRight, + expectedTransferAmounts.feePaidByRightMaker.add(expectedTransferAmounts.feePaidByTakerRight), ); return [expectedNewERC20BalancesByOwner, expectedNewERC721TokenIdsByOwner]; } -} + /// @dev Asserts ERC20 account balances and ERC721 token holdings that result from order matching. + /// Specifically checks balances of makers, taker and fee recipients. + /// @param signedOrderLeft First matched order. + /// @param signedOrderRight Second matched order. + /// @param expectedERC20BalancesByOwner Expected ERC20 balances. + /// @param realERC20BalancesByOwner Real ERC20 balances. + /// @param expectedERC721TokenIdsByOwner Expected ERC721 token owners. + /// @param realERC721TokenIdsByOwner Real ERC20 token owners. + /// @param takerAddress Address of taker (account that called Exchange.matchOrders). + private async _assertMakerTakerAndFeeRecipientBalancesAsync( + signedOrderLeft: SignedOrder, + signedOrderRight: SignedOrder, + expectedERC20BalancesByOwner: ERC20BalancesByOwner, + realERC20BalancesByOwner: ERC20BalancesByOwner, + expectedERC721TokenIdsByOwner: ERC721TokenIdsByOwner, + realERC721TokenIdsByOwner: ERC721TokenIdsByOwner, + takerAddress: string, + ): Promise<void> { + // Individual balance comparisons + const makerAssetProxyIdLeft = assetDataUtils.decodeAssetProxyId(signedOrderLeft.makerAssetData); + const makerERC20AssetDataLeft = + makerAssetProxyIdLeft === AssetProxyId.ERC20 + ? assetDataUtils.decodeERC20AssetData(signedOrderLeft.makerAssetData) + : assetDataUtils.decodeERC721AssetData(signedOrderLeft.makerAssetData); + const makerAssetAddressLeft = makerERC20AssetDataLeft.tokenAddress; + const makerAssetProxyIdRight = assetDataUtils.decodeAssetProxyId(signedOrderRight.makerAssetData); + const makerERC20AssetDataRight = + makerAssetProxyIdRight === AssetProxyId.ERC20 + ? assetDataUtils.decodeERC20AssetData(signedOrderRight.makerAssetData) + : assetDataUtils.decodeERC721AssetData(signedOrderRight.makerAssetData); + const makerAssetAddressRight = makerERC20AssetDataRight.tokenAddress; + if (makerAssetProxyIdLeft === AssetProxyId.ERC20) { + expect( + realERC20BalancesByOwner[signedOrderLeft.makerAddress][makerAssetAddressLeft], + 'Checking left maker egress ERC20 account balance', + ).to.be.bignumber.equal(expectedERC20BalancesByOwner[signedOrderLeft.makerAddress][makerAssetAddressLeft]); + expect( + realERC20BalancesByOwner[signedOrderRight.makerAddress][makerAssetAddressLeft], + 'Checking right maker ingress ERC20 account balance', + ).to.be.bignumber.equal(expectedERC20BalancesByOwner[signedOrderRight.makerAddress][makerAssetAddressLeft]); + expect( + realERC20BalancesByOwner[takerAddress][makerAssetAddressLeft], + 'Checking taker ingress ERC20 account balance', + ).to.be.bignumber.equal(expectedERC20BalancesByOwner[takerAddress][makerAssetAddressLeft]); + } else if (makerAssetProxyIdLeft === AssetProxyId.ERC721) { + expect( + realERC721TokenIdsByOwner[signedOrderLeft.makerAddress][makerAssetAddressLeft].sort(), + 'Checking left maker egress ERC721 account holdings', + ).to.be.deep.equal( + expectedERC721TokenIdsByOwner[signedOrderLeft.makerAddress][makerAssetAddressLeft].sort(), + ); + expect( + realERC721TokenIdsByOwner[signedOrderRight.makerAddress][makerAssetAddressLeft].sort(), + 'Checking right maker ERC721 account holdings', + ).to.be.deep.equal( + expectedERC721TokenIdsByOwner[signedOrderRight.makerAddress][makerAssetAddressLeft].sort(), + ); + expect( + realERC721TokenIdsByOwner[takerAddress][makerAssetAddressLeft].sort(), + 'Checking taker ingress ERC721 account holdings', + ).to.be.deep.equal(expectedERC721TokenIdsByOwner[takerAddress][makerAssetAddressLeft].sort()); + } else { + throw new Error(`Unhandled Asset Proxy ID: ${makerAssetProxyIdLeft}`); + } + if (makerAssetProxyIdRight === AssetProxyId.ERC20) { + expect( + realERC20BalancesByOwner[signedOrderLeft.makerAddress][makerAssetAddressRight], + 'Checking left maker ingress ERC20 account balance', + ).to.be.bignumber.equal(expectedERC20BalancesByOwner[signedOrderLeft.makerAddress][makerAssetAddressRight]); + expect( + realERC20BalancesByOwner[signedOrderRight.makerAddress][makerAssetAddressRight], + 'Checking right maker egress ERC20 account balance', + ).to.be.bignumber.equal( + expectedERC20BalancesByOwner[signedOrderRight.makerAddress][makerAssetAddressRight], + ); + } else if (makerAssetProxyIdRight === AssetProxyId.ERC721) { + expect( + realERC721TokenIdsByOwner[signedOrderLeft.makerAddress][makerAssetAddressRight].sort(), + 'Checking left maker ingress ERC721 account holdings', + ).to.be.deep.equal( + expectedERC721TokenIdsByOwner[signedOrderLeft.makerAddress][makerAssetAddressRight].sort(), + ); + expect( + realERC721TokenIdsByOwner[signedOrderRight.makerAddress][makerAssetAddressRight], + 'Checking right maker agress ERC721 account holdings', + ).to.be.deep.equal(expectedERC721TokenIdsByOwner[signedOrderRight.makerAddress][makerAssetAddressRight]); + } else { + throw new Error(`Unhandled Asset Proxy ID: ${makerAssetProxyIdRight}`); + } + // Paid fees + expect( + realERC20BalancesByOwner[signedOrderLeft.makerAddress][this._feeTokenAddress], + 'Checking left maker egress ERC20 account fees', + ).to.be.bignumber.equal(expectedERC20BalancesByOwner[signedOrderLeft.makerAddress][this._feeTokenAddress]); + expect( + realERC20BalancesByOwner[signedOrderRight.makerAddress][this._feeTokenAddress], + 'Checking right maker egress ERC20 account fees', + ).to.be.bignumber.equal(expectedERC20BalancesByOwner[signedOrderRight.makerAddress][this._feeTokenAddress]); + expect( + realERC20BalancesByOwner[takerAddress][this._feeTokenAddress], + 'Checking taker egress ERC20 account fees', + ).to.be.bignumber.equal(expectedERC20BalancesByOwner[takerAddress][this._feeTokenAddress]); + // Received fees + expect( + realERC20BalancesByOwner[signedOrderLeft.feeRecipientAddress][this._feeTokenAddress], + 'Checking left fee recipient ingress ERC20 account fees', + ).to.be.bignumber.equal( + expectedERC20BalancesByOwner[signedOrderLeft.feeRecipientAddress][this._feeTokenAddress], + ); + expect( + realERC20BalancesByOwner[signedOrderRight.feeRecipientAddress][this._feeTokenAddress], + 'Checking right fee receipient ingress ERC20 account fees', + ).to.be.bignumber.equal( + expectedERC20BalancesByOwner[signedOrderRight.feeRecipientAddress][this._feeTokenAddress], + ); + } +} // tslint:disable-line:max-file-line-count diff --git a/packages/contracts/test/utils/order_utils.ts b/packages/contracts/test/utils/order_utils.ts index 019f6e74b..444e27c44 100644 --- a/packages/contracts/test/utils/order_utils.ts +++ b/packages/contracts/test/utils/order_utils.ts @@ -5,7 +5,7 @@ import { constants } from './constants'; import { CancelOrder, MatchOrder } from './types'; export const orderUtils = { - getPartialAmount(numerator: BigNumber, denominator: BigNumber, target: BigNumber): BigNumber { + getPartialAmountFloor(numerator: BigNumber, denominator: BigNumber, target: BigNumber): BigNumber { const partialAmount = numerator .mul(target) .div(denominator) diff --git a/packages/contracts/test/utils/test_with_reference.ts b/packages/contracts/test/utils/test_with_reference.ts index 599b1eed4..b80be4a6c 100644 --- a/packages/contracts/test/utils/test_with_reference.ts +++ b/packages/contracts/test/utils/test_with_reference.ts @@ -6,6 +6,34 @@ import { chaiSetup } from './chai_setup'; chaiSetup.configure(); const expect = chai.expect; +class Value<T> { + public value: T; + constructor(value: T) { + this.value = value; + } +} + +// tslint:disable-next-line: max-classes-per-file +class ErrorMessage { + public error: string; + constructor(message: string) { + this.error = message; + } +} + +type PromiseResult<T> = Value<T> | ErrorMessage; + +// TODO(albrow): This seems like a generic utility function that could exist in +// lodash. We should replace it by a library implementation, or move it to our +// own. +async function evaluatePromise<T>(promise: Promise<T>): Promise<PromiseResult<T>> { + try { + return new Value<T>(await promise); + } catch (e) { + return new ErrorMessage(e.message); + } +} + export async function testWithReferenceFuncAsync<P0, R>( referenceFunc: (p0: P0) => Promise<R>, testFunc: (p0: P0) => Promise<R>, @@ -64,39 +92,31 @@ export async function testWithReferenceFuncAsync( testFuncAsync: (...args: any[]) => Promise<any>, values: any[], ): Promise<void> { - let expectedResult: any; - let expectedErr: string | undefined; - try { - expectedResult = await referenceFuncAsync(...values); - } catch (e) { - expectedErr = e.message; - } - let actualResult: any | undefined; - try { - actualResult = await testFuncAsync(...values); - if (!_.isUndefined(expectedErr)) { + // Measure correct behaviour + const expected = await evaluatePromise(referenceFuncAsync(...values)); + + // Measure actual behaviour + const actual = await evaluatePromise(testFuncAsync(...values)); + + // Compare behaviour + if (expected instanceof ErrorMessage) { + // If we expected an error, check if the actual error message contains the + // expected error message. + if (!(actual instanceof ErrorMessage)) { throw new Error( - `Expected error containing ${expectedErr} but got no error\n\tTest case: ${_getTestCaseString( + `Expected error containing ${expected.error} but got no error\n\tTest case: ${_getTestCaseString( referenceFuncAsync, values, )}`, ); } - } catch (e) { - if (_.isUndefined(expectedErr)) { - throw new Error(`${e.message}\n\tTest case: ${_getTestCaseString(referenceFuncAsync, values)}`); - } else { - expect(e.message).to.contain( - expectedErr, - `${e.message}\n\tTest case: ${_getTestCaseString(referenceFuncAsync, values)}`, - ); - } - } - if (!_.isUndefined(actualResult) && !_.isUndefined(expectedResult)) { - expect(actualResult).to.deep.equal( - expectedResult, - `Test case: ${_getTestCaseString(referenceFuncAsync, values)}`, + expect(actual.error).to.contain( + expected.error, + `${actual.error}\n\tTest case: ${_getTestCaseString(referenceFuncAsync, values)}`, ); + } else { + // If we do not expect an error, compare actual and expected directly. + expect(actual).to.deep.equal(expected, `Test case ${_getTestCaseString(referenceFuncAsync, values)}`); } } diff --git a/packages/contracts/test/utils/types.ts b/packages/contracts/test/utils/types.ts index 481ee87d6..598fb5393 100644 --- a/packages/contracts/test/utils/types.ts +++ b/packages/contracts/test/utils/types.ts @@ -117,21 +117,24 @@ export interface TransferAmountsByMatchOrders { // Left Maker amountBoughtByLeftMaker: BigNumber; amountSoldByLeftMaker: BigNumber; - amountReceivedByLeftMaker: BigNumber; feePaidByLeftMaker: BigNumber; // Right Maker amountBoughtByRightMaker: BigNumber; amountSoldByRightMaker: BigNumber; - amountReceivedByRightMaker: BigNumber; feePaidByRightMaker: BigNumber; // Taker amountReceivedByTaker: BigNumber; feePaidByTakerLeft: BigNumber; feePaidByTakerRight: BigNumber; - totalFeePaidByTaker: BigNumber; - // Fee Recipients - feeReceivedLeft: BigNumber; - feeReceivedRight: BigNumber; +} + +export interface TransferAmountsLoggedByMatchOrders { + makerAddress: string; + takerAddress: string; + makerAssetFilledAmount: string; + takerAssetFilledAmount: string; + makerFeePaid: string; + takerFeePaid: string; } export interface OrderInfo { diff --git a/packages/contracts/test/utils_test/test_with_reference.ts b/packages/contracts/test/utils_test/test_with_reference.ts new file mode 100644 index 000000000..8d633cd1f --- /dev/null +++ b/packages/contracts/test/utils_test/test_with_reference.ts @@ -0,0 +1,63 @@ +import * as chai from 'chai'; + +import { chaiSetup } from '../utils/chai_setup'; +import { testWithReferenceFuncAsync } from '../utils/test_with_reference'; + +chaiSetup.configure(); +const expect = chai.expect; + +async function divAsync(x: number, y: number): Promise<number> { + if (y === 0) { + throw new Error('MathError: divide by zero'); + } + return x / y; +} + +// returns an async function that always returns the given value. +function alwaysValueFunc(value: number): (x: number, y: number) => Promise<number> { + return async (x: number, y: number) => value; +} + +// returns an async function which always throws/rejects with the given error +// message. +function alwaysFailFunc(errMessage: string): (x: number, y: number) => Promise<number> { + return async (x: number, y: number) => { + throw new Error(errMessage); + }; +} + +describe('testWithReferenceFuncAsync', () => { + it('passes when both succeed and actual === expected', async () => { + await testWithReferenceFuncAsync(alwaysValueFunc(0.5), divAsync, [1, 2]); + }); + + it('passes when both fail and actual error contains expected error', async () => { + await testWithReferenceFuncAsync(alwaysFailFunc('divide by zero'), divAsync, [1, 0]); + }); + + it('fails when both succeed and actual !== expected', async () => { + expect(testWithReferenceFuncAsync(alwaysValueFunc(3), divAsync, [1, 2])).to.be.rejectedWith( + 'Test case {"x":1,"y":2}: expected { value: 0.5 } to deeply equal { value: 3 }', + ); + }); + + it('fails when both fail and actual error does not contain expected error', async () => { + expect( + testWithReferenceFuncAsync(alwaysFailFunc('Unexpected math error'), divAsync, [1, 0]), + ).to.be.rejectedWith( + 'MathError: divide by zero\n\tTest case: {"x":1,"y":0}: expected \'MathError: divide by zero\' to include \'Unexpected math error\'', + ); + }); + + it('fails when referenceFunc succeeds and testFunc fails', async () => { + expect(testWithReferenceFuncAsync(alwaysValueFunc(0), divAsync, [1, 0])).to.be.rejectedWith( + 'Test case {"x":1,"y":0}: expected { error: \'MathError: divide by zero\' } to deeply equal { value: 0 }', + ); + }); + + it('fails when referenceFunc fails and testFunc succeeds', async () => { + expect(testWithReferenceFuncAsync(alwaysFailFunc('divide by zero'), divAsync, [1, 2])).to.be.rejectedWith( + 'Expected error containing divide by zero but got no error\n\tTest case: {"x":1,"y":2}', + ); + }); +}); |