aboutsummaryrefslogtreecommitdiffstats
path: root/packages/contracts
diff options
context:
space:
mode:
authorGreg Hysen <hysz@users.noreply.github.com>2018-08-30 04:53:49 +0800
committerGitHub <noreply@github.com>2018-08-30 04:53:49 +0800
commit6cedf5362bb95863660ced59e0af04f97e525619 (patch)
tree3f89f13eafb8ed796c8dd813a41d8d3785332eb4 /packages/contracts
parente7d5ceb9c5487a5851dbfc1f8bdbe0182fedaef2 (diff)
parentaa833ef074b5492dec876b2991175cbd9eae599c (diff)
downloaddexon-sol-tools-6cedf5362bb95863660ced59e0af04f97e525619.tar
dexon-sol-tools-6cedf5362bb95863660ced59e0af04f97e525619.tar.gz
dexon-sol-tools-6cedf5362bb95863660ced59e0af04f97e525619.tar.bz2
dexon-sol-tools-6cedf5362bb95863660ced59e0af04f97e525619.tar.lz
dexon-sol-tools-6cedf5362bb95863660ced59e0af04f97e525619.tar.xz
dexon-sol-tools-6cedf5362bb95863660ced59e0af04f97e525619.tar.zst
dexon-sol-tools-6cedf5362bb95863660ced59e0af04f97e525619.zip
Merge pull request #1039 from 0xProject/fix/contracts/audit2Fixes
Fixes as-per Audit for LibBytes + ERC20 Proxy Comments
Diffstat (limited to 'packages/contracts')
-rw-r--r--packages/contracts/src/2.0.0/protocol/AssetProxy/ERC20Proxy.sol67
-rw-r--r--packages/contracts/src/2.0.0/protocol/AssetProxy/ERC721Proxy.sol2
-rw-r--r--packages/contracts/src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol7
-rw-r--r--packages/contracts/src/2.0.0/utils/LibBytes/LibBytes.sol7
-rw-r--r--packages/contracts/test/libraries/lib_bytes.ts49
5 files changed, 115 insertions, 17 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.
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..7da3d87e2 100644
--- a/packages/contracts/src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol
+++ b/packages/contracts/src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol
@@ -108,8 +108,13 @@ contract AssetProxyOwner is
returns (bytes4 result)
{
require(b.length >= index + 4);
+
+ // 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..efdfa13a2 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 testByte = '0xab';
let shortData: string;
let shortTestBytes: string;
let shortTestBytesAsBuffer: Buffer;
@@ -106,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 1 byte', 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', () => {
@@ -122,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 20 bytes', 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', () => {
@@ -303,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]);
@@ -462,8 +477,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 +488,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', () => {
@@ -546,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(
@@ -655,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(