crowetic/protocol
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix regression in DevUtils (#2449)

Browse Source 
* fix bug in OrderTransferSimulationUtils causing failures for 721 assets

* Patched the regression and added tests

* Added regression test for fillable order

* Created a test for in and out of process ganache

* Split up DevUtils into two contracts

* Updated migration

* Remove the in and out of process ganache test

* Fixed contract addresses

* Appease linter

* Addressed review comments and updated artifacts, wrappers, and snapshots

* Fixed regression after refactor

* Update DevUtils and libTransactionDecoder contracts on mainnet and testnets

* Addressed @mzhu's review feedback

* Addressed @hysz's review feedback

* Updated devUtils address on testnets and mainnet after deployment

Co-authored-by: mzhu25 <mchl.zhu.96@gmail.com>
Co-authored-by: Fabio B <kandinsky454@protonmail.ch>
This commit is contained in:
James Towle
2020-01-21 18:54:10 -08:00
committed by Jacob Evans
parent 4e46bf4697
commit ce04d3ce41
19 changed files with 625 additions and 244 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
contracts/dev-utils/compiler.json
View File

@@ -7,7 +7,7 @@
"evmVersion": "istanbul",
"optimizer": {
"enabled": true,
"runs": 200,
"runs": 5000,
"details": { "yul": true, "deduplicate": true, "cse": true, "constantOptimizer": true }
},
"outputSelection": {

2
contracts/dev-utils/contracts/src/DevUtils.sol
View File

@@ -26,14 +26,12 @@ import "@0x/contracts-utils/contracts/src/LibEIP712.sol";
import "@0x/contracts-utils/contracts/src/LibBytes.sol";
import "./OrderValidationUtils.sol";
import "./OrderTransferSimulationUtils.sol";
import "./LibTransactionDecoder.sol";
import "./EthBalanceChecker.sol";
// solhint-disable no-empty-blocks
contract DevUtils is
OrderValidationUtils,
LibTransactionDecoder,
LibEIP712ExchangeDomain,
EthBalanceChecker
{

14
contracts/dev-utils/contracts/src/OrderTransferSimulationUtils.sol
View File

@@ -41,6 +41,10 @@ contract OrderTransferSimulationUtils is
TransfersSuccessful // All transfers in the order were successful
}
// NOTE(jalextowle): This is a random address that we use to avoid issues that addresses like `address(1)`
// may cause later.
address constant internal UNUSED_ADDRESS = address(0x377f698C4c287018D09b516F415317aEC5919332);
// keccak256(abi.encodeWithSignature("Error(string)", "TRANSFERS_SUCCESSFUL"));
bytes32 constant internal _TRANSFERS_SUCCESSFUL_RESULT_HASH = 0xf43f26ea5a94b478394a975e856464913dc1a8a1ca70939d974aa7c238aa0ce0;
@@ -82,13 +86,13 @@ contract OrderTransferSimulationUtils is
// Transfer `makerAsset` from maker to taker
assetData[0] = order.makerAssetData;
fromAddresses[0] = order.makerAddress;
toAddresses[0] = takerAddress;
toAddresses[0] = takerAddress == address(0) ? UNUSED_ADDRESS : takerAddress;
amounts[0] = fillResults.makerAssetFilledAmount;
// Transfer `makerFeeAsset` from maker to feeRecipient
assetData[1] = order.makerFeeAssetData;
fromAddresses[1] = order.makerAddress;
toAddresses[1] = order.feeRecipientAddress;
toAddresses[1] = order.feeRecipientAddress == address(0) ? UNUSED_ADDRESS : order.feeRecipientAddress;
amounts[1] = fillResults.makerFeePaid;
return _simulateTransferFromCalls(
@@ -134,19 +138,19 @@ contract OrderTransferSimulationUtils is
// Transfer `makerAsset` from maker to taker
assetData[1] = order.makerAssetData;
fromAddresses[1] = order.makerAddress;
toAddresses[1] = takerAddress;
toAddresses[1] = takerAddress == address(0) ? UNUSED_ADDRESS : takerAddress;
amounts[1] = fillResults.makerAssetFilledAmount;
// Transfer `takerFeeAsset` from taker to feeRecipient
assetData[2] = order.takerFeeAssetData;
fromAddresses[2] = takerAddress;
toAddresses[2] = order.feeRecipientAddress;
toAddresses[2] = order.feeRecipientAddress == address(0) ? UNUSED_ADDRESS : order.feeRecipientAddress;
amounts[2] = fillResults.takerFeePaid;
// Transfer `makerFeeAsset` from maker to feeRecipient
assetData[3] = order.makerFeeAssetData;
fromAddresses[3] = order.makerAddress;
toAddresses[3] = order.feeRecipientAddress;
toAddresses[3] = order.feeRecipientAddress == address(0) ? UNUSED_ADDRESS : order.feeRecipientAddress;
amounts[3] = fillResults.makerFeePaid;
return _simulateTransferFromCalls(

71
contracts/dev-utils/contracts/src/OrderValidationUtils.sol
View File

@@ -133,6 +133,18 @@ contract OrderValidationUtils is
fillableTakerAssetAmount
) == OrderTransferResults.TransfersSuccessful ? fillableTakerAssetAmount : 0;
if (!_isAssetDataValid(order.takerAssetData)) {
fillableTakerAssetAmount = 0;
}
if (order.takerFee != 0 && !_isAssetDataValid(order.takerFeeAssetData)) {
fillableTakerAssetAmount = 0;
}
if (orderInfo.orderStatus != LibOrder.OrderStatus.FILLABLE) {
fillableTakerAssetAmount = 0;
}
return (orderInfo, fillableTakerAssetAmount, isValidSignature);
}
@@ -185,4 +197,63 @@ contract OrderValidationUtils is
transferableAssetAmount = LibSafeMath.min256(balance, allowance);
return transferableAssetAmount;
}
/// @dev This function handles the edge cases around taker validation. This function
/// currently attempts to find duplicate ERC721 token's in the taker
/// multiAssetData.
/// @param assetData The asset data that should be validated.
/// @return Whether or not the order should be considered valid.
function _isAssetDataValid(bytes memory assetData)
internal
pure
returns (bool)
{
// Asset data must be composed of an asset proxy Id and a bytes segment with
// a length divisible by 32.
if (assetData.length % 32 != 4) {
return false;
}
// Only process the taker asset data if it is multiAssetData.
bytes4 assetProxyId = assetData.readBytes4(0);
if (assetProxyId != IAssetData(address(0)).MultiAsset.selector) {
return true;
}
// Get array of values and array of assetDatas
(, uint256[] memory assetAmounts, bytes[] memory nestedAssetData) = decodeMultiAssetData(assetData);
uint256 length = nestedAssetData.length;
for (uint256 i = 0; i != length; i++) {
// TODO(jalextowle): Implement similar validation for non-fungible ERC1155 asset data.
bytes4 nestedAssetProxyId = nestedAssetData[i].readBytes4(0);
if (nestedAssetProxyId == IAssetData(address(0)).ERC721Token.selector) {
if (_isAssetDataDuplicated(nestedAssetData, i)) {
return false;
}
}
}
return true;
}
/// Determines whether or not asset data is duplicated later in the nested asset data.
/// @param nestedAssetData The asset data to scan for duplication.
/// @param startIdx The index where the scan should begin.
/// @return A boolean reflecting whether or not the starting asset data was duplicated.
function _isAssetDataDuplicated(
bytes[] memory nestedAssetData,
uint256 startIdx
)
internal
pure
returns (bool)
{
uint256 length = nestedAssetData.length;
for (uint256 i = startIdx + 1; i < length; i++) {
if (nestedAssetData[startIdx].equals(nestedAssetData[i])) {
return true;
}
}
}
}

41
contracts/integrations/test/dev-utils/order_validation_utils.ts
View File

@@ -155,6 +155,19 @@ blockchainTests.resets('OrderValidationUtils/OrderTransferSimulatorUtils', env =
.callAsync();
expect(fillableTakerAssetAmount).to.bignumber.equal(constants.ZERO_AMOUNT);
});
it('should correctly validate fillable order', async () => {
signedOrder = await maker.signOrderAsync({
makerAssetData: erc721AssetData,
makerAssetAmount: new BigNumber(1),
makerFee: constants.ZERO_AMOUNT,
takerFee: constants.ZERO_AMOUNT,
});
await taker.configureERC20TokenAsync(erc20Token2);
const [, fillableTakerAssetAmount] = await devUtils
.getOrderRelevantState(signedOrder, signedOrder.signature)
.callAsync();
expect(fillableTakerAssetAmount).to.bignumber.greaterThan(constants.ZERO_AMOUNT);
});
it('should return a fillableTakerAssetAmount of 0 when balances/allowances of one asset within a multiAssetData are insufficient (ERC20)', async () => {
const multiAssetData = await devUtils
.encodeMultiAssetData([new BigNumber(1), new BigNumber(1)], [erc20AssetData, erc20AssetData2])
@@ -222,6 +235,34 @@ blockchainTests.resets('OrderValidationUtils/OrderTransferSimulatorUtils', env =
.callAsync();
expect(fillableTakerAssetAmount).to.bignumber.equal(constants.ZERO_AMOUNT);
});
it('should return a fillableTakerAssetAmount of 0 when an erc721 asset is duplicated in the maker fee side of a multi-asset proxy order', async () => {
const multiAssetData = await devUtils
.encodeMultiAssetData([new BigNumber(1), new BigNumber(1)], [erc721AssetData, erc721AssetData])
.callAsync();
signedOrder = await maker.signOrderAsync({
makerFeeAssetData: multiAssetData,
makerFee: new BigNumber(1),
takerFee: constants.ZERO_AMOUNT,
});
const [, fillableTakerAssetAmount] = await devUtils
.getOrderRelevantState(signedOrder, signedOrder.signature)
.callAsync();
expect(fillableTakerAssetAmount).to.bignumber.equal(constants.ZERO_AMOUNT);
});
it('should return a fillableTakerAssetAmount of 0 when an erc721 asset is duplicated in the taker fee side of a multi-asset proxy order', async () => {
const multiAssetData = await devUtils
.encodeMultiAssetData([new BigNumber(1), new BigNumber(1)], [erc721AssetData, erc721AssetData])
.callAsync();
signedOrder = await maker.signOrderAsync({
makerFee: constants.ZERO_AMOUNT,
takerFeeAssetData: multiAssetData,
takerFee: new BigNumber(1),
});
const [, fillableTakerAssetAmount] = await devUtils
.getOrderRelevantState(signedOrder, signedOrder.signature)
.callAsync();
expect(fillableTakerAssetAmount).to.bignumber.equal(constants.ZERO_AMOUNT);
});
it('should return the correct fillableTakerAssetAmount when fee balances/allowances are partially sufficient', async () => {
await erc20Token.setBalance(maker.address, signedOrder.makerAssetAmount).awaitTransactionSuccessAsync();
await erc20Token.approve(erc20Proxy.address, signedOrder.makerAssetAmount).awaitTransactionSuccessAsync({

1
contracts/integrations/test/framework/deployment_manager.ts
View File

@@ -196,7 +196,6 @@ export class DeploymentManager {
exchange,
staking.stakingProxy,
]);
const devUtils = await DevUtilsContract.deployFrom0xArtifactAsync(
devUtilsArtifacts.DevUtils,
environment.provider,