From 64401f10318bc12641f32239763326ba478fa417 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Thu, 7 Feb 2019 14:00:04 -0800 Subject: [PATCH] Fix bug in slice functions that disallow slicing the last byte of a byte array --- contracts/utils/contracts/src/LibBytes.sol | 4 +- .../utils/contracts/test/TestLibBytes.sol | 37 +++++++++ contracts/utils/test/lib_bytes.ts | 80 +++++++++++++++++++ packages/types/src/index.ts | 2 + 4 files changed, 121 insertions(+), 2 deletions(-) diff --git a/contracts/utils/contracts/src/LibBytes.sol b/contracts/utils/contracts/src/LibBytes.sol index 1fa56786da..bc82fa519e 100644 --- a/contracts/utils/contracts/src/LibBytes.sol +++ b/contracts/utils/contracts/src/LibBytes.sol @@ -179,7 +179,7 @@ library LibBytes { "FROM_LESS_THAN_TO_REQUIRED" ); require( - to < b.length, + to <= b.length, "TO_LESS_THAN_LENGTH_REQUIRED" ); @@ -213,7 +213,7 @@ library LibBytes { "FROM_LESS_THAN_TO_REQUIRED" ); require( - to < b.length, + to <= b.length, "TO_LESS_THAN_LENGTH_REQUIRED" ); diff --git a/contracts/utils/contracts/test/TestLibBytes.sol b/contracts/utils/contracts/test/TestLibBytes.sol index 82ad314fb5..4743be66e6 100644 --- a/contracts/utils/contracts/test/TestLibBytes.sol +++ b/contracts/utils/contracts/test/TestLibBytes.sol @@ -266,4 +266,41 @@ contract TestLibBytes { // Return modified memory contents return mem; } + + /// @dev Returns a slices from a byte array. + /// @param b The byte array to take a slice from. + /// @param from The starting index for the slice (inclusive). + /// @param to The final index for the slice (exclusive). + /// @return result The slice containing bytes at indices [from, to) + function publicSlice( + bytes memory b, + uint256 from, + uint256 to + ) + public + pure + returns (bytes memory result) + { + result = LibBytes.slice(b, from, to); + return result; + } + + /// @dev Returns a slice from a byte array without preserving the input. + /// @param b The byte array to take a slice from. Will be destroyed in the process. + /// @param from The starting index for the slice (inclusive). + /// @param to The final index for the slice (exclusive). + /// @return result The slice containing bytes at indices [from, to) + /// @dev When `from == 0`, the original array will match the slice. In other cases its state will be corrupted. + function publicSliceDestructive( + bytes memory b, + uint256 from, + uint256 to + ) + public + pure + returns (bytes memory result) + { + result = LibBytes.sliceDestructive(b, from, to); + return result; + } } diff --git a/contracts/utils/test/lib_bytes.ts b/contracts/utils/test/lib_bytes.ts index daad287299..282fa768c6 100644 --- a/contracts/utils/test/lib_bytes.ts +++ b/contracts/utils/test/lib_bytes.ts @@ -870,5 +870,85 @@ describe('LibBytes', () => { describe('copies forward within one word and one byte overlap', () => test([[0, 0, 1, 'one byte'], [0, 10, 11, 'eleven bytes'], [0, 15, 16, 'sixteen bytes']])); }); + + describe('slice', () => { + it('should revert if from > to', async () => { + const from = new BigNumber(1); + const to = new BigNumber(0); + expectContractCallFailedAsync( + libBytes.publicSlice.callAsync(byteArrayLongerThan32Bytes, from, to), + RevertReason.FromLessThanToRequired, + ); + }); + it('should return a byte array of length 0 if from == to', async () => { + const from = new BigNumber(0); + const to = from; + const result = await libBytes.publicSlice.callAsync(byteArrayLongerThan32Bytes, from, to); + expect(result).to.eq(constants.NULL_BYTES); + }); + it('should revert if to > input.length', async () => { + const byteLen = (byteArrayLongerThan32Bytes.length - 2) / 2; + const from = new BigNumber(0); + const to = new BigNumber(byteLen).plus(1); + expectContractCallFailedAsync( + libBytes.publicSlice.callAsync(byteArrayLongerThan32Bytes, from, to), + RevertReason.ToLessThanLengthRequired, + ); + }); + it('should slice a section of the input', async () => { + const from = new BigNumber(1); + const to = new BigNumber(2); + const result = await libBytes.publicSlice.callAsync(byteArrayLongerThan32Bytes, from, to); + const expectedResult = `0x${byteArrayLongerThan32Bytes.slice(4, 6)}`; + expect(result).to.eq(expectedResult); + }); + it('should copy the entire input if from = 0 and to = input.length', async () => { + const byteLen = (byteArrayLongerThan32Bytes.length - 2) / 2; + const from = new BigNumber(0); + const to = new BigNumber(byteLen); + const result = await libBytes.publicSlice.callAsync(byteArrayLongerThan32Bytes, from, to); + expect(result).to.eq(byteArrayLongerThan32Bytes); + }); + }); + + describe('sliceDestructive', () => { + it('should revert if from > to', async () => { + const from = new BigNumber(1); + const to = new BigNumber(0); + expectContractCallFailedAsync( + libBytes.publicSliceDestructive.callAsync(byteArrayLongerThan32Bytes, from, to), + RevertReason.FromLessThanToRequired, + ); + }); + it('should return a byte array of length 0 if from == to', async () => { + const from = new BigNumber(0); + const to = from; + const result = await libBytes.publicSliceDestructive.callAsync(byteArrayLongerThan32Bytes, from, to); + expect(result).to.eq(constants.NULL_BYTES); + }); + it('should revert if to > input.length', async () => { + const byteLen = (byteArrayLongerThan32Bytes.length - 2) / 2; + const from = new BigNumber(0); + const to = new BigNumber(byteLen).plus(1); + expectContractCallFailedAsync( + libBytes.publicSliceDestructive.callAsync(byteArrayLongerThan32Bytes, from, to), + RevertReason.ToLessThanLengthRequired, + ); + }); + it('should slice a section of the input', async () => { + const from = new BigNumber(1); + const to = new BigNumber(2); + const result = await libBytes.publicSliceDestructive.callAsync(byteArrayLongerThan32Bytes, from, to); + const expectedResult = `0x${byteArrayLongerThan32Bytes.slice(4, 6)}`; + expect(result).to.eq(expectedResult); + }); + it('should copy the entire input if from = 0 and to = input.length', async () => { + const byteLen = (byteArrayLongerThan32Bytes.length - 2) / 2; + const from = new BigNumber(0); + const to = new BigNumber(byteLen); + const result = await libBytes.publicSliceDestructive.callAsync(byteArrayLongerThan32Bytes, from, to); + expect(result).to.eq(byteArrayLongerThan32Bytes); + }); + }); }); // tslint:disable:max-file-line-count diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index b3a0839994..ead0543a71 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -269,6 +269,8 @@ export enum RevertReason { InvalidOrBlockedExchangeSelector = 'INVALID_OR_BLOCKED_EXCHANGE_SELECTOR', BalanceQueryFailed = 'BALANCE_QUERY_FAILED', AtLeastOneAddressDoesNotMeetBalanceThreshold = 'AT_LEAST_ONE_ADDRESS_DOES_NOT_MEET_BALANCE_THRESHOLD', + FromLessThanToRequired = 'FROM_LESS_THAN_TO_REQUIRED', + ToLessThanLengthRequired = 'TO_LESS_THAN_LENGTH_REQUIRED', } export enum StatusCodes {