From 1c3b2b7141138f8da82197dfbdbdbfd76e7780b7 Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Wed, 29 Aug 2018 11:39:52 -0700 Subject: Updated readBytes4 to match spec + added unit tests. These are 3.5/3.6 from audit --- .../protocol/AssetProxyOwner/AssetProxyOwner.sol | 13 +++++++++-- .../src/2.0.0/utils/LibBytes/LibBytes.sol | 7 +++++- packages/contracts/test/libraries/lib_bytes.ts | 26 +++++++++++++++++++++- 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/packages/contracts/src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol b/packages/contracts/src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol index 8b7333646..e661f2427 100644 --- a/packages/contracts/src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol +++ b/packages/contracts/src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol @@ -105,11 +105,20 @@ contract AssetProxyOwner is uint256 index ) internal + pure returns (bytes4 result) { - require(b.length >= index + 4); + require( + b.length >= index + 4, + "GREATER_OR_EQUAL_TO_4_LENGTH_REQUIRED" + ); + + // Arrays are prefixed by a 32 byte length field + index += 32; + + // Read the bytes4 from array memory assembly { - result := mload(add(b, 32)) + result := mload(add(b, index)) // Solidity does not require us to clean the trailing bytes. // We do it anyway result := and(result, 0xFFFFFFFF00000000000000000000000000000000000000000000000000000000) diff --git a/packages/contracts/src/2.0.0/utils/LibBytes/LibBytes.sol b/packages/contracts/src/2.0.0/utils/LibBytes/LibBytes.sol index 504e950a8..93873cbcc 100644 --- a/packages/contracts/src/2.0.0/utils/LibBytes/LibBytes.sol +++ b/packages/contracts/src/2.0.0/utils/LibBytes/LibBytes.sol @@ -467,8 +467,13 @@ library LibBytes { b.length >= index + 4, "GREATER_OR_EQUAL_TO_4_LENGTH_REQUIRED" ); + + // Arrays are prefixed by a 32 byte length field + index += 32; + + // Read the bytes4 from array memory assembly { - result := mload(add(b, 32)) + result := mload(add(b, index)) // Solidity does not require us to clean the trailing bytes. // We do it anyway result := and(result, 0xFFFFFFFF00000000000000000000000000000000000000000000000000000000) diff --git a/packages/contracts/test/libraries/lib_bytes.ts b/packages/contracts/test/libraries/lib_bytes.ts index 1c497a226..6fda9dc97 100644 --- a/packages/contracts/test/libraries/lib_bytes.ts +++ b/packages/contracts/test/libraries/lib_bytes.ts @@ -41,6 +41,8 @@ describe('LibBytes', () => { const testBytes32B = '0x534877abd8443578526845cdfef020047528759477fedef87346527659aced32'; const testUint256 = new BigNumber(testBytes32, 16); const testUint256B = new BigNumber(testBytes32B, 16); + const testBytes4 = '0xabcdef12'; + const testBytes4B = new BigNumber(testBytes4, 16); let shortData: string; let shortTestBytes: string; let shortTestBytesAsBuffer: Buffer; @@ -462,8 +464,9 @@ describe('LibBytes', () => { // AssertionError: expected promise to be rejected with an error including 'revert' but it was fulfilled with '0x08c379a0' it('should revert if byte array has a length < 4', async () => { const byteArrayLessThan4Bytes = '0x010101'; + const offset = new BigNumber(0); return expectContractCallFailed( - libBytes.publicReadBytes4.callAsync(byteArrayLessThan4Bytes, new BigNumber(0)), + libBytes.publicReadBytes4.callAsync(byteArrayLessThan4Bytes, offset), RevertReason.LibBytesGreaterOrEqualTo4LengthRequired, ); }); @@ -472,6 +475,27 @@ describe('LibBytes', () => { const expectedFirst4Bytes = byteArrayLongerThan32Bytes.slice(0, 10); expect(first4Bytes).to.equal(expectedFirst4Bytes); }); + it('should successfully read bytes4 when the bytes4 takes up the whole array', async () => { + const testBytes4Offset = new BigNumber(0); + const bytes4 = await libBytes.publicReadBytes4.callAsync(testBytes4, testBytes4Offset); + return expect(bytes4).to.be.equal(testBytes4); + }); + it('should successfully read bytes4 when it is offset in the array)', async () => { + const bytes4ByteArrayBuffer = ethUtil.toBuffer(testBytes4); + const prefixByteArrayBuffer = ethUtil.toBuffer('0xabcdef'); + const combinedByteArrayBuffer = Buffer.concat([prefixByteArrayBuffer, bytes4ByteArrayBuffer]); + const combinedByteArray = ethUtil.bufferToHex(combinedByteArrayBuffer); + const testBytes4Offset = new BigNumber(prefixByteArrayBuffer.byteLength); + const bytes4 = await libBytes.publicReadBytes4.callAsync(combinedByteArray, testBytes4Offset); + return expect(bytes4).to.be.equal(testBytes4); + }); + it('should fail if the length between the offset and end of the byte array is too short to hold a bytes4', async () => { + const badOffset = new BigNumber(ethUtil.toBuffer(testBytes4).byteLength); + return expectContractCallFailed( + libBytes.publicReadBytes4.callAsync(testBytes4, badOffset), + RevertReason.LibBytesGreaterOrEqualTo4LengthRequired, + ); + }); }); describe('readBytesWithLength', () => { -- cgit v1.2.3 From 8c803ab232d094c2f69d3b2d44cae11f153d2d5f Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Wed, 29 Aug 2018 12:25:06 -0700 Subject: Updated comments for ERC20 proxy to clarify how we load the token address from calldata (3.18 from audit) --- .../src/2.0.0/protocol/AssetProxy/ERC20Proxy.sol | 67 +++++++++++++++++++--- .../src/2.0.0/protocol/AssetProxy/ERC721Proxy.sol | 2 + 2 files changed, 60 insertions(+), 9 deletions(-) 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 004c3892d..785f3aba9 100644 --- a/packages/contracts/src/2.0.0/protocol/AssetProxy/ERC20Proxy.sol +++ b/packages/contracts/src/2.0.0/protocol/AssetProxy/ERC20Proxy.sol @@ -59,15 +59,64 @@ contract ERC20Proxy is mstore(96, 0) revert(0, 100) } - - /////// Token contract address /////// - // The token address is found as follows: - // * It is stored at offset 4 in `assetData` contents. - // * This is stored at offset 32 from `assetData`. - // * The offset to `assetData` from Params is stored at offset - // 4 in calldata. - // * The offset of Params in calldata is 4. - // So we read location 4 and add 32 + 4 + 4 to it. + + // `transferFrom`. + // The function is marked `external`, so no abi decodeding is done for + // us. Instead, we expect the `calldata` memory to contain the + // following: + // + // | Area | Offset | Length | Contents | + // |----------|--------|---------|-------------------------------------| + // | Header | 0 | 4 | function selector | + // | Params | | 4 * 32 | function parameters: | + // | | 4 | | 1. offset to assetData (*) | + // | | 36 | | 2. from | + // | | 68 | | 3. to | + // | | 100 | | 4. amount | + // | Data | | | assetData: | + // | | 132 | 32 | assetData Length | + // | | 164 | ** | assetData Contents | + // + // (*): offset is computed from start of function parameters, so offset + // by an additional 4 bytes in the calldata. + // + // (**): see table below to compute length of assetData Contents + // + // WARNING: The ABIv2 specification allows additional padding between + // the Params and Data section. This will result in a larger + // offset to assetData. + + // Asset data itself is encoded as follows: + // + // | Area | Offset | Length | Contents | + // |----------|--------|---------|-------------------------------------| + // | Header | 0 | 4 | function selector | + // | Params | | 1 * 32 | function parameters: | + // | | 4 | 12 + 20 | 1. token address | + + // We construct calldata for the `token.transferFrom` ABI. + // The layout of this calldata is in the table below. + // + // | Area | Offset | Length | Contents | + // |----------|--------|---------|-------------------------------------| + // | Header | 0 | 4 | function selector | + // | Params | | 3 * 32 | function parameters: | + // | | 4 | | 1. from | + // | | 36 | | 2. to | + // | | 68 | | 3. amount | + + /////// Read token address from calldata /////// + // * The token address is stored in `assetData`. + // + // * The "offset to assetData" is stored at offset 4 in the calldata (table 1). + // [assetDataOffsetFromParams = calldataload(4)] + // + // * Notes that the "offset to assetData" is relative to the "Params" area of calldata; + // add 4 bytes to account for the length of the "Header" area (table 1). + // [assetDataOffsetFromHeader = assetDataOffsetFromParams + 4] + // + // * The "token address" is offset 32+4=36 bytes into "assetData" (tables 1 & 2). + // [tokenOffset = assetDataOffsetFromHeader + 36 = calldataload(4) + 4 + 36] let token := calldataload(add(calldataload(4), 40)) /////// Setup Header Area /////// 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 9d0bc0f74..caf05b0dd 100644 --- a/packages/contracts/src/2.0.0/protocol/AssetProxy/ERC721Proxy.sol +++ b/packages/contracts/src/2.0.0/protocol/AssetProxy/ERC721Proxy.sol @@ -80,6 +80,8 @@ contract ERC721Proxy is // (*): offset is computed from start of function parameters, so offset // by an additional 4 bytes in the calldata. // + // (**): see table below to compute length of assetData Contents + // // WARNING: The ABIv2 specification allows additional padding between // the Params and Data section. This will result in a larger // offset to assetData. -- cgit v1.2.3 From 62b93cf2ebf242ea5b432daddc030b24da46421f Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Wed, 29 Aug 2018 13:14:51 -0700 Subject: More tests for LibBytes --- packages/contracts/test/libraries/lib_bytes.ts | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/packages/contracts/test/libraries/lib_bytes.ts b/packages/contracts/test/libraries/lib_bytes.ts index 6fda9dc97..646875f1e 100644 --- a/packages/contracts/test/libraries/lib_bytes.ts +++ b/packages/contracts/test/libraries/lib_bytes.ts @@ -42,7 +42,7 @@ describe('LibBytes', () => { const testUint256 = new BigNumber(testBytes32, 16); const testUint256B = new BigNumber(testBytes32B, 16); const testBytes4 = '0xabcdef12'; - const testBytes4B = new BigNumber(testBytes4, 16); + const testByte = '0xab'; let shortData: string; let shortTestBytes: string; let shortTestBytesAsBuffer: Buffer; @@ -108,13 +108,19 @@ describe('LibBytes', () => { RevertReason.LibBytesGreaterThanZeroLengthRequired, ); }); - it('should pop the last byte from the input and return it', async () => { + it('should pop the last byte from the input and return it when array holds more than 1 byte', async () => { const [newBytes, poppedByte] = await libBytes.publicPopLastByte.callAsync(byteArrayLongerThan32Bytes); const expectedNewBytes = byteArrayLongerThan32Bytes.slice(0, -2); const expectedPoppedByte = `0x${byteArrayLongerThan32Bytes.slice(-2)}`; expect(newBytes).to.equal(expectedNewBytes); expect(poppedByte).to.equal(expectedPoppedByte); }); + it('should pop the last byte from the input and return it when array is exactly one', async () => { + const [newBytes, poppedByte] = await libBytes.publicPopLastByte.callAsync(testByte); + const expectedNewBytes = '0x'; + expect(newBytes).to.equal(expectedNewBytes); + return expect(poppedByte).to.be.equal(testByte); + }); }); describe('popLast20Bytes', () => { @@ -124,13 +130,20 @@ describe('LibBytes', () => { RevertReason.LibBytesGreaterOrEqualTo20LengthRequired, ); }); - it('should pop the last 20 bytes from the input and return it', async () => { + it('should pop the last 20 bytes from the input and return it when array holds more than one byte', async () => { const [newBytes, poppedAddress] = await libBytes.publicPopLast20Bytes.callAsync(byteArrayLongerThan32Bytes); const expectedNewBytes = byteArrayLongerThan32Bytes.slice(0, -40); const expectedPoppedAddress = `0x${byteArrayLongerThan32Bytes.slice(-40)}`; expect(newBytes).to.equal(expectedNewBytes); expect(poppedAddress).to.equal(expectedPoppedAddress); }); + it('should pop the last 20 bytes from the input and return it when array is exactly 20 bytes', async () => { + const [newBytes, poppedAddress] = await libBytes.publicPopLast20Bytes.callAsync(testAddress); + const expectedNewBytes = '0x'; + const expectedPoppedAddress = testAddress; + expect(newBytes).to.equal(expectedNewBytes); + expect(poppedAddress).to.equal(expectedPoppedAddress); + }); }); describe('equals', () => { -- cgit v1.2.3 From 5f1c9cfee503bd3fb53fe12bb21f42f3f7981dc7 Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Wed, 29 Aug 2018 13:15:40 -0700 Subject: Reverted syntax used by readBytes4 in AssetProxyOwner to be compatible with Solidity v0.4.10 --- .../src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/contracts/src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol b/packages/contracts/src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol index e661f2427..7da3d87e2 100644 --- a/packages/contracts/src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol +++ b/packages/contracts/src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol @@ -105,13 +105,9 @@ contract AssetProxyOwner is uint256 index ) internal - pure returns (bytes4 result) { - require( - b.length >= index + 4, - "GREATER_OR_EQUAL_TO_4_LENGTH_REQUIRED" - ); + require(b.length >= index + 4); // Arrays are prefixed by a 32 byte length field index += 32; -- cgit v1.2.3 From aa833ef074b5492dec876b2991175cbd9eae599c Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Wed, 29 Aug 2018 13:17:13 -0700 Subject: Typos in LibBytes tests --- packages/contracts/test/libraries/lib_bytes.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/contracts/test/libraries/lib_bytes.ts b/packages/contracts/test/libraries/lib_bytes.ts index 646875f1e..efdfa13a2 100644 --- a/packages/contracts/test/libraries/lib_bytes.ts +++ b/packages/contracts/test/libraries/lib_bytes.ts @@ -115,7 +115,7 @@ describe('LibBytes', () => { expect(newBytes).to.equal(expectedNewBytes); expect(poppedByte).to.equal(expectedPoppedByte); }); - it('should pop the last byte from the input and return it when array is exactly one', async () => { + it('should pop the last byte from the input and return it when array is exactly 1 byte', async () => { const [newBytes, poppedByte] = await libBytes.publicPopLastByte.callAsync(testByte); const expectedNewBytes = '0x'; expect(newBytes).to.equal(expectedNewBytes); @@ -130,7 +130,7 @@ describe('LibBytes', () => { RevertReason.LibBytesGreaterOrEqualTo20LengthRequired, ); }); - it('should pop the last 20 bytes from the input and return it when array holds more than one byte', async () => { + it('should pop the last 20 bytes from the input and return it when array holds more than 20 bytes', async () => { const [newBytes, poppedAddress] = await libBytes.publicPopLast20Bytes.callAsync(byteArrayLongerThan32Bytes); const expectedNewBytes = byteArrayLongerThan32Bytes.slice(0, -40); const expectedPoppedAddress = `0x${byteArrayLongerThan32Bytes.slice(-40)}`; @@ -318,7 +318,7 @@ describe('LibBytes', () => { const bytes32 = await libBytes.publicReadBytes32.callAsync(testBytes32, testBytes32Offset); return expect(bytes32).to.be.equal(testBytes32); }); - it('should successfully read bytes32 when it is offset in the array)', async () => { + it('should successfully read bytes32 when it is offset in the array', async () => { const bytes32ByteArrayBuffer = ethUtil.toBuffer(testBytes32); const prefixByteArrayBuffer = ethUtil.toBuffer('0xabcdef'); const combinedByteArrayBuffer = Buffer.concat([prefixByteArrayBuffer, bytes32ByteArrayBuffer]); @@ -493,7 +493,7 @@ describe('LibBytes', () => { const bytes4 = await libBytes.publicReadBytes4.callAsync(testBytes4, testBytes4Offset); return expect(bytes4).to.be.equal(testBytes4); }); - it('should successfully read bytes4 when it is offset in the array)', async () => { + it('should successfully read bytes4 when it is offset in the array', async () => { const bytes4ByteArrayBuffer = ethUtil.toBuffer(testBytes4); const prefixByteArrayBuffer = ethUtil.toBuffer('0xabcdef'); const combinedByteArrayBuffer = Buffer.concat([prefixByteArrayBuffer, bytes4ByteArrayBuffer]); @@ -583,7 +583,7 @@ describe('LibBytes', () => { }); describe('writeBytesWithLength', () => { - it('should successfully write short, nested array of bytes when it takes up the whole array)', async () => { + it('should successfully write short, nested array of bytes when it takes up the whole array', async () => { const testBytesOffset = new BigNumber(0); const emptyByteArray = ethUtil.bufferToHex(new Buffer(shortTestBytesAsBuffer.byteLength)); const bytesWritten = await libBytes.publicWriteBytesWithLength.callAsync( @@ -692,7 +692,7 @@ describe('LibBytes', () => { RevertReason.LibBytesGreaterOrEqualToNestedBytesLengthRequired, ); }); - it('should fail if the length between the offset and end of the byte array is too short to hold the length of a nested byte array)', async () => { + it('should fail if the length between the offset and end of the byte array is too short to hold the length of a nested byte array', async () => { const emptyByteArray = ethUtil.bufferToHex(new Buffer(shortTestBytesAsBuffer.byteLength)); const badOffset = new BigNumber(ethUtil.toBuffer(shortTestBytesAsBuffer).byteLength); return expectContractCallFailed( -- cgit v1.2.3