diff --git a/contracts/asset-proxy/contracts/src/MultiAssetProxy.sol b/contracts/asset-proxy/contracts/src/MultiAssetProxy.sol index a0261f872c..dfda0544a5 100644 --- a/contracts/asset-proxy/contracts/src/MultiAssetProxy.sol +++ b/contracts/asset-proxy/contracts/src/MultiAssetProxy.sol @@ -90,35 +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(add(assetDataOffset, 4)) - - // Assert that the length of asset data: - // 1. Must be at least 132 bytes (see table above) - // 2. Must be a multiple of 32 (excluding the 4-byte selector) - if or(lt(assetDataLength, 132), 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 - // +4 for selector - // +32 for length field - let assetDataEnd := add(assetDataOffset, add(assetDataLength, 36)) - 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) - } + let assetDataLength := calldataload(assetDataOffset) // Asset data itself is encoded as follows: // @@ -136,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)) @@ -232,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) diff --git a/contracts/asset-proxy/test/proxies.ts b/contracts/asset-proxy/test/proxies.ts index 8003b1af58..ffca38a77f 100644 --- a/contracts/asset-proxy/test/proxies.ts +++ b/contracts/asset-proxy/test/proxies.ts @@ -1712,18 +1712,18 @@ describe('Asset Transfer Proxies', () => { RevertReason.InvalidAssetDataLength, ); }); - it('should revert if length of assetData is less than 132 bytes', async () => { + 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 - // 96 byte payload. This results in asset data that is 100 bytes + // 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 zeros96Bytes = '0'.repeat(188); - const assetData131Bytes = `${AssetProxyId.MultiAsset}${zeros96Bytes}`; + const zeros32Bytes = '0'.repeat(64); + const assetData36Bytes = `${AssetProxyId.MultiAsset}${zeros32Bytes}`; const badData = assetProxyInterface.transferFrom.getABIEncodedTransactionData( - assetData131Bytes, + assetData36Bytes, fromAddress, toAddress, inputAmount,