From e7c4120d24278416f56c0a64851bebfa3833c448 Mon Sep 17 00:00:00 2001
From: Greg Hysen <greg.hysen@gmail.com>
Date: Tue, 4 Jun 2019 14:09:22 -0700
Subject: [PATCH] Validation checks for the length of asset data

---
 .../contracts/src/ERC1155Proxy.sol            | 12 +++++++
 contracts/asset-proxy/test/erc1155_proxy.ts   | 32 ++++++++++++++++++-
 packages/types/src/index.ts                   |  1 +
 3 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/contracts/asset-proxy/contracts/src/ERC1155Proxy.sol b/contracts/asset-proxy/contracts/src/ERC1155Proxy.sol
index 024e1cba06..5dcb64ada4 100644
--- a/contracts/asset-proxy/contracts/src/ERC1155Proxy.sol
+++ b/contracts/asset-proxy/contracts/src/ERC1155Proxy.sol
@@ -163,6 +163,18 @@ contract ERC1155Proxy is
                 // Load length in bytes of `assetData`
                 let assetDataLength := calldataload(assetDataOffset)
 
+                // Assert that the length of asset data:
+                // 1. Must be at least 132 bytes (Table #2)
+                // 2. Must be a multiple of 32 (excluding the 4-byte selector)
+                if or(lt(assetDataLength, 100), 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
                 // +32 for length field
                 let assetDataEnd := add(assetDataOffset, add(assetDataLength, 32))
diff --git a/contracts/asset-proxy/test/erc1155_proxy.ts b/contracts/asset-proxy/test/erc1155_proxy.ts
index 59152a944b..73836a1bdc 100644
--- a/contracts/asset-proxy/test/erc1155_proxy.ts
+++ b/contracts/asset-proxy/test/erc1155_proxy.ts
@@ -643,7 +643,7 @@ describe('ERC1155Proxy', () => {
                 valuesToTransfer,
                 receiverCallbackData,
             );
-            const extraData = '0102030405060708';
+            const extraData = '0102030405060708091001020304050607080910010203040506070809100102';
             const assetDataWithExtraData = `${assetData}${extraData}`;
             // check balances before transfer
             const expectedInitialBalances = [spenderInitialFungibleBalance, receiverContractInitialFungibleBalance];
@@ -1352,6 +1352,36 @@ describe('ERC1155Proxy', () => {
                 RevertReason.InvalidDataOffset,
             );
         });
+        it('should revert if length of assetData, excluding the selector, is not a multiple of 32', async () => {
+            // setup test parameters
+            const tokensToTransfer = fungibleTokens.slice(0, 1);
+            const valuesToTransfer = [fungibleValueToTransferLarge];
+            const valueMultiplier = valueMultiplierSmall;
+            const erc1155ContractAddress = erc1155Wrapper.getContract().address;
+            const assetData = assetDataUtils.encodeERC1155AssetData(
+                erc1155ContractAddress,
+                tokensToTransfer,
+                valuesToTransfer,
+                receiverCallbackData,
+            );
+            const extraData = '01';
+            const assetDataWithExtraData = `${assetData}${extraData}`;
+            // execute transfer
+            await expectTransactionFailedAsync(
+                erc1155ProxyWrapper.transferFromWithLogsAsync(
+                    spender,
+                    receiverContract,
+                    erc1155Contract.address,
+                    tokensToTransfer,
+                    valuesToTransfer,
+                    valueMultiplier,
+                    receiverCallbackData,
+                    authorized,
+                    assetDataWithExtraData,
+                ),
+                RevertReason.InvalidAssetDataLength
+            );
+        });
         it('should transfer nothing if value is zero', async () => {
             // setup test parameters
             const tokenHolders = [spender, receiver];
diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts
index d501f859a2..560ebf3c61 100644
--- a/packages/types/src/index.ts
+++ b/packages/types/src/index.ts
@@ -315,6 +315,7 @@ export enum RevertReason {
     InvalidIdsOffset = 'INVALID_IDS_OFFSET',
     InvalidValuesOffset = 'INVALID_VALUES_OFFSET',
     InvalidDataOffset = 'INVALID_DATA_OFFSET',
+    InvalidAssetDataLength = 'INVALID_ASSET_DATA_LENGTH',
 }
 
 export enum StatusCodes {