Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

Commit

Permalink
Merge pull request #1964 from 0xProject/feature/contracts/consistentE…
Browse files Browse the repository at this point in the history 
…rrorCodes

Update MAP + add validation to assetDataUtils
  • Loading branch information
@abandeali1
abandeali1 authored Jul 17, 2019
2 parents 37bce53 + 05d50b6 commit c4d9ef9
Show file tree
Hide file tree
Showing 5 changed files with 227 additions and 25 deletions.
61 changes: 45 additions & 16 deletions contracts/asset-proxy/contracts/src/MultiAssetProxy.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,10 @@ contract MultiAssetProxy is
// offset to assetData.

// Load offset to `assetData`
let assetDataOffset := calldataload(4)
let assetDataOffset := add(calldataload(4), 4)

// Load length in bytes of `assetData`
let assetDataLength := calldataload(assetDataOffset)

// Asset data itself is encoded as follows:
//
Expand All @@ -108,41 +111,62 @@ contract MultiAssetProxy is
// | | 132 + a | b | nestedAssetData Contents (offsets) |
// | | 132 + a + b | | nestedAssetData[0, ..., len] |

// Assert that the length of asset data:
// 1. Must be at least 68 bytes (see table above)
// 2. Must be a multiple of 32 (excluding the 4-byte selector)
if or(lt(assetDataLength, 68), mod(sub(assetDataLength, 4), 32)) {
// Revert with `Error("INVALID_ASSET_DATA_LENGTH")`
mstore(0, 0x08c379a000000000000000000000000000000000000000000000000000000000)
mstore(32, 0x0000002000000000000000000000000000000000000000000000000000000000)
mstore(64, 0x00000019494e56414c49445f41535345545f444154415f4c454e475448000000)
mstore(96, 0)
revert(0, 100)
}

// End of asset data in calldata
// assetDataOffset
// + 32 (assetData len)
let assetDataEnd := add(assetDataOffset, add(assetDataLength, 32))
if gt(assetDataEnd, calldatasize()) {
// Revert with `Error("INVALID_ASSET_DATA_END")`
mstore(0, 0x08c379a000000000000000000000000000000000000000000000000000000000)
mstore(32, 0x0000002000000000000000000000000000000000000000000000000000000000)
mstore(64, 0x00000016494e56414c49445f41535345545f444154415f454e44000000000000)
mstore(96, 0)
revert(0, 100)
}

// In order to find the offset to `amounts`, we must add:
// 4 (function selector)
// + assetDataOffset
// assetDataOffset
// + 32 (assetData len)
// + 4 (assetProxyId)
let amountsOffset := calldataload(add(assetDataOffset, 40))
let amountsOffset := calldataload(add(assetDataOffset, 36))

// In order to find the offset to `nestedAssetData`, we must add:
// 4 (function selector)
// + assetDataOffset
// assetDataOffset
// + 32 (assetData len)
// + 4 (assetProxyId)
// + 32 (amounts offset)
let nestedAssetDataOffset := calldataload(add(assetDataOffset, 72))
let nestedAssetDataOffset := calldataload(add(assetDataOffset, 68))

// In order to find the start of the `amounts` contents, we must add:
// 4 (function selector)
// + assetDataOffset
// assetDataOffset
// + 32 (assetData len)
// + 4 (assetProxyId)
// + amountsOffset
// + 32 (amounts len)
let amountsContentsStart := add(assetDataOffset, add(amountsOffset, 72))
let amountsContentsStart := add(assetDataOffset, add(amountsOffset, 68))

// Load number of elements in `amounts`
let amountsLen := calldataload(sub(amountsContentsStart, 32))

// In order to find the start of the `nestedAssetData` contents, we must add:
// 4 (function selector)
// + assetDataOffset
// assetDataOffset
// + 32 (assetData len)
// + 4 (assetProxyId)
// + nestedAssetDataOffset
// + 32 (nestedAssetData len)
let nestedAssetDataContentsStart := add(assetDataOffset, add(nestedAssetDataOffset, 72))
let nestedAssetDataContentsStart := add(assetDataOffset, add(nestedAssetDataOffset, 68))

// Load number of elements in `nestedAssetData`
let nestedAssetDataLen := calldataload(sub(nestedAssetDataContentsStart, 32))
Expand Down Expand Up @@ -204,15 +228,20 @@ contract MultiAssetProxy is
let nestedAssetDataElementOffset := calldataload(add(nestedAssetDataContentsStart, i))

// In order to find the start of the `nestedAssetData[i]` contents, we must add:
// 4 (function selector)
// + assetDataOffset
// assetDataOffset
// + 32 (assetData len)
// + 4 (assetProxyId)
// + nestedAssetDataOffset
// + 32 (nestedAssetData len)
// + nestedAssetDataElementOffset
// + 32 (nestedAssetDataElement len)
let nestedAssetDataElementContentsStart := add(assetDataOffset, add(nestedAssetDataOffset, add(nestedAssetDataElementOffset, 104)))
let nestedAssetDataElementContentsStart := add(
assetDataOffset,
add(
nestedAssetDataOffset,
add(nestedAssetDataElementOffset, 100)
)
)

// Load length of `nestedAssetData[i]`
let nestedAssetDataElementLenStart := sub(nestedAssetDataElementContentsStart, 32)
Expand Down
118 changes: 116 additions & 2 deletions contracts/asset-proxy/test/proxies.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {
} from '@0x/contracts-test-utils';
import { BlockchainLifecycle } from '@0x/dev-utils';
import { assetDataUtils } from '@0x/order-utils';
import { RevertReason } from '@0x/types';
import { AssetProxyId, RevertReason } from '@0x/types';
import { BigNumber } from '@0x/utils';
import * as chai from 'chai';
import { LogWithDecodedArgs } from 'ethereum-types';
Expand Down Expand Up @@ -1329,7 +1329,7 @@ describe('Asset Transfer Proxies', () => {
const erc721AssetData = assetDataUtils.encodeERC721AssetData(erc721TokenA.address, erc721AFromTokenId);
const amounts = [erc20Amount, erc721Amount];
const nestedAssetData = [erc20AssetData, erc721AssetData];
const extraData = '0102030405060708';
const extraData = '0102030405060708090001020304050607080900010203040506070809000102';
const assetData = `${assetDataUtils.encodeMultiAssetData(amounts, nestedAssetData)}${extraData}`;
const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData(
assetData,
Expand Down Expand Up @@ -1624,6 +1624,120 @@ describe('Asset Transfer Proxies', () => {
RevertReason.SenderNotAuthorized,
);
});
it('should revert if asset data overflows beyond the bounds of calldata', async () => {
const inputAmount = new BigNumber(1);
const erc20Amount = new BigNumber(10);
const erc20AssetData = assetDataUtils.encodeERC20AssetData(erc20TokenA.address);
const erc721Amount = new BigNumber(1);
const erc721AssetData = assetDataUtils.encodeERC721AssetData(erc721TokenA.address, erc721AFromTokenId);
const amounts = [erc20Amount, erc721Amount];
const nestedAssetData = [erc20AssetData, erc721AssetData];
const assetData = assetDataUtils.encodeMultiAssetData(amounts, nestedAssetData);
const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData(
assetData,
fromAddress,
toAddress,
inputAmount,
);
// append asset data to end of tx data with a length of 0x300 bytes, which will extend past actual calldata.
const offsetToAssetData = '0000000000000000000000000000000000000000000000000000000000000080';
const invalidOffsetToAssetData = '00000000000000000000000000000000000000000000000000000000000002a0';
const newAssetData = '0000000000000000000000000000000000000000000000000000000000000304';
const badData = `${data.replace(offsetToAssetData, invalidOffsetToAssetData)}${newAssetData}`;
// execute transfer
await expectTransactionFailedAsync(
web3Wrapper.sendTransactionAsync({
to: multiAssetProxy.address,
data: badData,
from: authorized,
}),
RevertReason.InvalidAssetDataEnd,
);
});
it('should revert if asset data resolves to a location beyond the bounds of calldata', async () => {
const inputAmount = new BigNumber(1);
const erc20Amount = new BigNumber(10);
const erc20AssetData = assetDataUtils.encodeERC20AssetData(erc20TokenA.address);
const erc721Amount = new BigNumber(1);
const erc721AssetData = assetDataUtils.encodeERC721AssetData(erc721TokenA.address, erc721AFromTokenId);
const amounts = [erc20Amount, erc721Amount];
const nestedAssetData = [erc20AssetData, erc721AssetData];
const assetData = assetDataUtils.encodeMultiAssetData(amounts, nestedAssetData);
const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData(
assetData,
fromAddress,
toAddress,
inputAmount,
);
const offsetToAssetData = '0000000000000000000000000000000000000000000000000000000000000080';
const invalidOffsetToAssetData = '0000000000000000000000000000000000000000000000000000000000000400';
const badData = data.replace(offsetToAssetData, invalidOffsetToAssetData);
// execute transfer
// note that this triggers `InvalidAssetDataLength` because the length is zero, otherwise it would
// trigger `InvalidAssetDataEnd`.
await expectTransactionFailedAsync(
web3Wrapper.sendTransactionAsync({
to: multiAssetProxy.address,
data: badData,
from: authorized,
}),
RevertReason.InvalidAssetDataLength,
);
});
it('should revert if length of assetData, excluding the selector, is not a multiple of 32', async () => {
// setup test parameters
const inputAmount = new BigNumber(1);
const erc20Amount = new BigNumber(10);
const erc20AssetData = assetDataUtils.encodeERC20AssetData(erc20TokenA.address);
const erc721Amount = new BigNumber(1);
const erc721AssetData = assetDataUtils.encodeERC721AssetData(erc721TokenA.address, erc721AFromTokenId);
const amounts = [erc20Amount, erc721Amount];
const nestedAssetData = [erc20AssetData, erc721AssetData];
const assetData = assetDataUtils.encodeMultiAssetData(amounts, nestedAssetData);
const extraData = '01';
const assetDataWithExtraData = `${assetData}${extraData}`;
const badData = assetProxyInterface.transferFrom.getABIEncodedTransactionData(
assetDataWithExtraData,
fromAddress,
toAddress,
inputAmount,
);
// execute transfer
await expectTransactionFailedAsync(
web3Wrapper.sendTransactionAsync({
to: multiAssetProxy.address,
data: badData,
from: authorized,
}),
RevertReason.InvalidAssetDataLength,
);
});
it('should revert if length of assetData is less than 68 bytes', async () => {
// setup test parameters
const inputAmount = new BigNumber(1);
// we'll construct asset data that has a 4 byte selector plus
// 32 byte payload. This results in asset data that is 36 bytes
// long and will trigger the `invalid length` error.
// we must be sure to use a # of bytes that is still %32
// so that we know the error is not triggered by another check in the code.
const zeros32Bytes = '0'.repeat(64);
const assetData36Bytes = `${AssetProxyId.MultiAsset}${zeros32Bytes}`;
const badData = assetProxyInterface.transferFrom.getABIEncodedTransactionData(
assetData36Bytes,
fromAddress,
toAddress,
inputAmount,
);
// execute transfer
await expectTransactionFailedAsync(
web3Wrapper.sendTransactionAsync({
to: multiAssetProxy.address,
data: badData,
from: authorized,
}),
RevertReason.InvalidAssetDataLength,
);
});
});
});
});
Expand Down
9 changes: 9 additions & 0 deletions packages/order-utils/CHANGELOG.json
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,13 @@
[
{
"version": "8.2.3",
"changes": [
{
"note": "Ensure `assetData` is word aligned",
"pr": 1964
}
]
},
{
"timestamp": 1563193019,
"version": "8.2.2",
Expand Down
54 changes: 51 additions & 3 deletions packages/order-utils/src/asset_data_utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,7 @@ export const assetDataUtils = {
}. Got ${assetData.length}`,
);
}
assetDataUtils.assertWordAlignedAssetData(assetData);
const assetProxyId = assetDataUtils.decodeAssetProxyId(assetData);
if (assetProxyId !== AssetProxyId.ERC20) {
throw new Error(
Expand All @@ -379,6 +380,7 @@ export const assetDataUtils = {
}. Got ${assetData.length}`,
);
}
assetDataUtils.assertWordAlignedAssetData(assetData);
const assetProxyId = assetDataUtils.decodeAssetProxyId(assetData);
if (assetProxyId !== AssetProxyId.ERC721) {
throw new Error(
Expand All @@ -393,8 +395,22 @@ export const assetDataUtils = {
* @param assetData Hex encoded assetData string
*/
assertIsERC1155AssetData(assetData: string): void {
// If the asset data is correctly decoded then it is valid.
assetDataUtils.decodeERC1155AssetData(assetData);
if (assetData.length < constants.ERC1155_ASSET_DATA_MIN_CHAR_LENGTH_WITH_PREFIX) {
throw new Error(
`Could not decode ERC1155 Proxy Data. Expected length of encoded data to be at least ${
constants.ERC1155_ASSET_DATA_MIN_CHAR_LENGTH_WITH_PREFIX
}. Got ${assetData.length}`,
);
}
assetDataUtils.assertWordAlignedAssetData(assetData);
const assetProxyId = assetDataUtils.decodeAssetProxyId(assetData);
if (assetProxyId !== AssetProxyId.ERC1155) {
throw new Error(
`Could not decode ERC1155 assetData. Expected assetProxyId to be ERC1155 (${
AssetProxyId.ERC1155
}), but got ${assetProxyId}`,
);
}
},
/**
* Throws if the length or assetProxyId are invalid for the MultiAssetProxy.
Expand All @@ -408,6 +424,7 @@ export const assetDataUtils = {
}. Got ${assetData.length}`,
);
}
assetDataUtils.assertWordAlignedAssetData(assetData);
const assetProxyId = assetDataUtils.decodeAssetProxyId(assetData);
if (assetProxyId !== AssetProxyId.MultiAsset) {
throw new Error(
Expand All @@ -422,7 +439,34 @@ export const assetDataUtils = {
* @param assetData Hex encoded assetData string
*/
assertIsStaticCallAssetData(assetData: string): void {
assetDataUtils.decodeStaticCallAssetData(assetData);
if (assetData.length < constants.STATIC_CALL_ASSET_DATA_MIN_CHAR_LENGTH_WITH_PREFIX) {
throw new Error(
`Could not decode StaticCall Proxy Data. Expected length of encoded data to be at least ${
constants.STATIC_CALL_ASSET_DATA_MIN_CHAR_LENGTH_WITH_PREFIX
}. Got ${assetData.length}`,
);
}
assetDataUtils.assertWordAlignedAssetData(assetData);
const assetProxyId = assetDataUtils.decodeAssetProxyId(assetData);
if (assetProxyId !== AssetProxyId.StaticCall) {
throw new Error(
`Could not decode StaticCall assetData. Expected assetProxyId to be StaticCall (${
AssetProxyId.StaticCall
}), but got ${assetProxyId}`,
);
}
},
/**
* Throws if the assetData is not padded to 32 bytes.
* @param assetData Hex encoded assetData string
*/
assertWordAlignedAssetData(assetData: string): void {
const charsIn32Bytes = 64;
if ((assetData.length - constants.SELECTOR_CHAR_LENGTH_WITH_PREFIX) % charsIn32Bytes !== 0) {
throw new Error(
`assetData must be word aligned. ${(assetData.length - 2) / 2} is not a valid byte length.`,
);
}
},
/**
* Throws if the length or assetProxyId are invalid for the corresponding AssetProxy.
Expand Down Expand Up @@ -470,8 +514,12 @@ export const assetDataUtils = {
case AssetProxyId.MultiAsset:
const multiAssetData = assetDataUtils.decodeMultiAssetData(assetData);
return multiAssetData;
case AssetProxyId.StaticCall:
const staticCallData = assetDataUtils.decodeStaticCallAssetData(assetData);
return staticCallData;
default:
throw new Error(`Unrecognized asset proxy id: ${assetProxyId}`);
}
},
};
// tslint:disable:max-file-line-count
10 changes: 6 additions & 4 deletions packages/order-utils/src/constants.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,12 @@ export const constants = {
UNLIMITED_ALLOWANCE_IN_BASE_UNITS: new BigNumber(2).pow(256).minus(1),
TESTRPC_NETWORK_ID: 50,
ADDRESS_LENGTH: 20,
ERC20_ASSET_DATA_MIN_CHAR_LENGTH_WITH_PREFIX: 74,
ERC721_ASSET_DATA_MIN_CHAR_LENGTH_WITH_PREFIX: 136,
MULTI_ASSET_DATA_MIN_CHAR_LENGTH_WITH_PREFIX: 266,
SELECTOR_CHAR_LENGTH_WITH_PREFIX: 10,
ERC20_ASSET_DATA_MIN_CHAR_LENGTH_WITH_PREFIX: 74, // 36 bytes
ERC721_ASSET_DATA_MIN_CHAR_LENGTH_WITH_PREFIX: 138, // 68 bytes
ERC1155_ASSET_DATA_MIN_CHAR_LENGTH_WITH_PREFIX: 266, // 132 bytes
MULTI_ASSET_DATA_MIN_CHAR_LENGTH_WITH_PREFIX: 138, // 68 bytes
STATIC_CALL_ASSET_DATA_MIN_CHAR_LENGTH_WITH_PREFIX: 202, // 100 bytes
SELECTOR_CHAR_LENGTH_WITH_PREFIX: 10, // 4 bytes
INFINITE_TIMESTAMP_SEC: new BigNumber(2524604400), // Close to infinite
ZERO_AMOUNT: new BigNumber(0),
EXCHANGE_DOMAIN_NAME: '0x Protocol',
Expand Down

0 comments on commit c4d9ef9

Please sign in to comment.