From c62f26d66d54877ef08b381364fa3ac645c328e8 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 15 Jul 2025 11:17:13 +0200 Subject: [PATCH 1/8] =?UTF-8?q?Fix=20bug=20in=20Bytes.lastIndexOf=20when?= =?UTF-8?q?=20array=20is=20empty=20and=20position=20is=20not=202=C2=B2?= =?UTF-8?q?=E2=81=B5=E2=81=B6-1?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- contracts/utils/Bytes.sol | 10 ++++++---- test/utils/Bytes.t.sol | 31 ++++++++++++++++++++++++++++++ test/utils/Bytes.test.js | 40 +++++++++++++++++++++++++++------------ 3 files changed, 65 insertions(+), 16 deletions(-) diff --git a/contracts/utils/Bytes.sol b/contracts/utils/Bytes.sol index b76198e8032..e06d23d42b6 100644 --- a/contracts/utils/Bytes.sol +++ b/contracts/utils/Bytes.sol @@ -58,10 +58,12 @@ library Bytes { function lastIndexOf(bytes memory buffer, bytes1 s, uint256 pos) internal pure returns (uint256) { unchecked { uint256 length = buffer.length; - // NOTE here we cannot do `i = Math.min(pos + 1, length)` because `pos + 1` could overflow - for (uint256 i = Math.min(pos, length - 1) + 1; i > 0; --i) { - if (bytes1(_unsafeReadBytesOffset(buffer, i - 1)) == s) { - return i - 1; + if (length > 0) { + // NOTE here we cannot do `i = Math.min(pos + 1, length)` because `pos + 1` could overflow + for (uint256 i = Math.min(pos, length - 1) + 1; i > 0; --i) { + if (bytes1(_unsafeReadBytesOffset(buffer, i - 1)) == s) { + return i - 1; + } } } return type(uint256).max; diff --git a/test/utils/Bytes.t.sol b/test/utils/Bytes.t.sol index be0bf783e95..dbcc62898f4 100644 --- a/test/utils/Bytes.t.sol +++ b/test/utils/Bytes.t.sol @@ -9,6 +9,37 @@ import {Bytes} from "@openzeppelin/contracts/utils/Bytes.sol"; contract BytesTest is Test { using Bytes for bytes; + // INDEX OF + function testIndexOf(bytes memory buffer, bytes1 s) public pure { + testIndexOf(buffer, s, 0); + } + + function testIndexOf(bytes memory buffer, bytes1 s, uint256 pos) public pure { + uint256 result = Bytes.indexOf(buffer, s, pos); + + // The search value should not me present between `pos` (included) and `result` excluded. + // Do not search after the end of the buffer + for (uint256 i = pos; i < Math.min(result, buffer.length); ++i) assertNotEq(buffer[i], s); + if (result != type(uint256).max) assertEq(buffer[result], s); + } + + function testLastIndexOf(bytes memory buffer, bytes1 s) public pure { + testLastIndexOf(buffer, s, 0); + } + + function testLastIndexOf(bytes memory buffer, bytes1 s, uint256 pos) public pure { + uint256 result = Bytes.lastIndexOf(buffer, s, pos); + + // Case found: the search value should not be present between `result` (excluded) and `pos` (included) + // Case not found: the search value should not be present anywhere before `pos` (included) + unchecked { + // using unchecked gives us `result + 1 == 0` in the "not found" case + for (uint256 i = result + 1; i < Math.min(pos + 1, buffer.length); ++i) assertNotEq(buffer[i], s); + } + if (result != type(uint256).max) assertEq(buffer[result], s); + } + + // SLICES function testSliceWithStartOnly(bytes memory buffer, uint256 start) public pure { bytes memory originalBuffer = bytes.concat(buffer); bytes memory result = buffer.slice(start); diff --git a/test/utils/Bytes.test.js b/test/utils/Bytes.test.js index 0357cac90d1..55c0be9178f 100644 --- a/test/utils/Bytes.test.js +++ b/test/utils/Bytes.test.js @@ -28,39 +28,55 @@ describe('Bytes', function () { describe('indexOf', function () { it('first', async function () { - expect(await this.mock.$indexOf(lorem, ethers.toBeHex(present))).to.equal(lorem.indexOf(present)); + await expect(this.mock.$indexOf(lorem, ethers.toBeHex(present))).to.eventually.equal(lorem.indexOf(present)); }); it('from index', async function () { for (const start in Array(lorem.length + 10).fill()) { const index = lorem.indexOf(present, start); const result = index === -1 ? ethers.MaxUint256 : index; - expect(await this.mock.$indexOf(lorem, ethers.toBeHex(present), ethers.Typed.uint256(start))).to.equal(result); + await expect( + this.mock.$indexOf(lorem, ethers.toBeHex(present), ethers.Typed.uint256(start)), + ).to.eventually.equal(result); } }); it('absent', async function () { - expect(await this.mock.$indexOf(lorem, ethers.toBeHex(absent))).to.equal(ethers.MaxUint256); + await expect(this.mock.$indexOf(lorem, ethers.toBeHex(absent))).to.eventually.equal(ethers.MaxUint256); + }); + + it('empty buffer', async function () { + await expect(this.mock.$indexOf('0x', '0x00')).to.eventually.equal(ethers.MaxUint256); + await expect(this.mock.$indexOf('0x', '0x00', ethers.Typed.uint256(17))).to.eventually.equal(ethers.MaxUint256); }); }); describe('lastIndexOf', function () { it('first', async function () { - expect(await this.mock.$lastIndexOf(lorem, ethers.toBeHex(present))).to.equal(lorem.lastIndexOf(present)); + await expect(this.mock.$lastIndexOf(lorem, ethers.toBeHex(present))).to.eventually.equal( + lorem.lastIndexOf(present), + ); }); it('from index', async function () { for (const start in Array(lorem.length + 10).fill()) { const index = lorem.lastIndexOf(present, start); const result = index === -1 ? ethers.MaxUint256 : index; - expect(await this.mock.$lastIndexOf(lorem, ethers.toBeHex(present), ethers.Typed.uint256(start))).to.equal( - result, - ); + await expect( + this.mock.$lastIndexOf(lorem, ethers.toBeHex(present), ethers.Typed.uint256(start)), + ).to.eventually.equal(result); } }); it('absent', async function () { - expect(await this.mock.$lastIndexOf(lorem, ethers.toBeHex(absent))).to.equal(ethers.MaxUint256); + await expect(this.mock.$lastIndexOf(lorem, ethers.toBeHex(absent))).to.eventually.equal(ethers.MaxUint256); + }); + + it('empty buffer', async function () { + await expect(this.mock.$lastIndexOf('0x', '0x00')).to.eventually.equal(ethers.MaxUint256); + await expect(this.mock.$lastIndexOf('0x', '0x00', ethers.Typed.uint256(17))).to.eventually.equal( + ethers.MaxUint256, + ); }); }); @@ -73,8 +89,8 @@ describe('Bytes', function () { })) { it(descr, async function () { const result = ethers.hexlify(lorem.slice(start)); - expect(await this.mock.$slice(lorem, start)).to.equal(result); - expect(await this.mock.$splice(lorem, start)).to.equal(result); + await expect(this.mock.$slice(lorem, start)).to.eventually.equal(result); + await expect(this.mock.$splice(lorem, start)).to.eventually.equal(result); }); } }); @@ -89,8 +105,8 @@ describe('Bytes', function () { })) { it(descr, async function () { const result = ethers.hexlify(lorem.slice(start, end)); - expect(await this.mock.$slice(lorem, start, ethers.Typed.uint256(end))).to.equal(result); - expect(await this.mock.$splice(lorem, start, ethers.Typed.uint256(end))).to.equal(result); + await expect(this.mock.$slice(lorem, start, ethers.Typed.uint256(end))).to.eventually.equal(result); + await expect(this.mock.$splice(lorem, start, ethers.Typed.uint256(end))).to.eventually.equal(result); }); } }); From aa1cdb84d2104269ebb35813811305799793f4e3 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 15 Jul 2025 11:20:49 +0200 Subject: [PATCH 2/8] add changeset --- .changeset/witty-hats-flow.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/witty-hats-flow.md diff --git a/.changeset/witty-hats-flow.md b/.changeset/witty-hats-flow.md new file mode 100644 index 00000000000..1c567f5526e --- /dev/null +++ b/.changeset/witty-hats-flow.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`Bytes`: Fix an issue when calling `lastIndexOf(bytes,byte,uint256)` with an empty buffer and a lookup position that is not 2²⁵⁶-1. From 29b1b35926889714333a7f644bf544b1a8e508f6 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 15 Jul 2025 11:24:34 +0200 Subject: [PATCH 3/8] branchless --- contracts/utils/Bytes.sol | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/contracts/utils/Bytes.sol b/contracts/utils/Bytes.sol index e06d23d42b6..b2f33d80511 100644 --- a/contracts/utils/Bytes.sol +++ b/contracts/utils/Bytes.sol @@ -58,12 +58,10 @@ library Bytes { function lastIndexOf(bytes memory buffer, bytes1 s, uint256 pos) internal pure returns (uint256) { unchecked { uint256 length = buffer.length; - if (length > 0) { - // NOTE here we cannot do `i = Math.min(pos + 1, length)` because `pos + 1` could overflow - for (uint256 i = Math.min(pos, length - 1) + 1; i > 0; --i) { - if (bytes1(_unsafeReadBytesOffset(buffer, i - 1)) == s) { - return i - 1; - } + uint256 end = Math.ternary(pos == type(uint256).max, length, Math.min(pos + 1, length)); + for (uint256 i = end; i > 0; --i) { + if (bytes1(_unsafeReadBytesOffset(buffer, i - 1)) == s) { + return i - 1; } } return type(uint256).max; From edf974ea12e36ed4d73addf2870523b103fb6947 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 15 Jul 2025 12:35:09 +0200 Subject: [PATCH 4/8] simplify fuzz cases --- test/utils/Bytes.t.sol | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/test/utils/Bytes.t.sol b/test/utils/Bytes.t.sol index dbcc62898f4..24b012a3fe7 100644 --- a/test/utils/Bytes.t.sol +++ b/test/utils/Bytes.t.sol @@ -17,10 +17,18 @@ contract BytesTest is Test { function testIndexOf(bytes memory buffer, bytes1 s, uint256 pos) public pure { uint256 result = Bytes.indexOf(buffer, s, pos); - // The search value should not me present between `pos` (included) and `result` excluded. - // Do not search after the end of the buffer - for (uint256 i = pos; i < Math.min(result, buffer.length); ++i) assertNotEq(buffer[i], s); - if (result != type(uint256).max) assertEq(buffer[result], s); + if (buffer.length == 0) { + // Case 0: buffer is empty + assertEq(result, type(uint256).max); + } else if (result == type(uint256).max) { + // Case 1: search value could not be found + for (uint256 i = pos; i < buffer.length; ++i) assertNotEq(buffer[i], s); + } else { + // Case 2: search value was found + assertEq(buffer[result], s); + // search value is not present anywhere before the found location + for (uint256 i = pos; i < result; ++i) assertNotEq(buffer[i], s); + } } function testLastIndexOf(bytes memory buffer, bytes1 s) public pure { @@ -30,13 +38,18 @@ contract BytesTest is Test { function testLastIndexOf(bytes memory buffer, bytes1 s, uint256 pos) public pure { uint256 result = Bytes.lastIndexOf(buffer, s, pos); - // Case found: the search value should not be present between `result` (excluded) and `pos` (included) - // Case not found: the search value should not be present anywhere before `pos` (included) - unchecked { - // using unchecked gives us `result + 1 == 0` in the "not found" case - for (uint256 i = result + 1; i < Math.min(pos + 1, buffer.length); ++i) assertNotEq(buffer[i], s); + if (buffer.length == 0) { + // Case 0: buffer is empty + assertEq(result, type(uint256).max); + } else if (result == type(uint256).max) { + // Case 1: search value could not be found + for (uint256 i = 0; i <= Math.min(pos, buffer.length - 1); ++i) assertNotEq(buffer[i], s); + } else { + // Case 2: search value was found + assertEq(buffer[result], s); + // search value is not present anywhere after the found location + for (uint256 i = result + 1; i <= Math.min(pos, buffer.length - 1); ++i) assertNotEq(buffer[i], s); } - if (result != type(uint256).max) assertEq(buffer[result], s); } // SLICES From 655c3c013824badbe61fd25e72a7afa681479bc9 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 15 Jul 2025 12:39:53 +0200 Subject: [PATCH 5/8] independant fuzz tests of and without pos --- test/utils/Bytes.t.sol | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/test/utils/Bytes.t.sol b/test/utils/Bytes.t.sol index 24b012a3fe7..ec2edc7564e 100644 --- a/test/utils/Bytes.t.sol +++ b/test/utils/Bytes.t.sol @@ -11,7 +11,20 @@ contract BytesTest is Test { // INDEX OF function testIndexOf(bytes memory buffer, bytes1 s) public pure { - testIndexOf(buffer, s, 0); + uint256 result = Bytes.indexOf(buffer, s); + + if (buffer.length == 0) { + // Case 0: buffer is empty + assertEq(result, type(uint256).max); + } else if (result == type(uint256).max) { + // Case 1: search value could not be found + for (uint256 i = 0; i < buffer.length; ++i) assertNotEq(buffer[i], s); + } else { + // Case 2: search value was found + assertEq(buffer[result], s); + // search value is not present anywhere before the found location + for (uint256 i = 0; i < result; ++i) assertNotEq(buffer[i], s); + } } function testIndexOf(bytes memory buffer, bytes1 s, uint256 pos) public pure { @@ -32,7 +45,20 @@ contract BytesTest is Test { } function testLastIndexOf(bytes memory buffer, bytes1 s) public pure { - testLastIndexOf(buffer, s, 0); + uint256 result = Bytes.lastIndexOf(buffer, s); + + if (buffer.length == 0) { + // Case 0: buffer is empty + assertEq(result, type(uint256).max); + } else if (result == type(uint256).max) { + // Case 1: search value could not be found + for (uint256 i = 0; i < buffer.length; ++i) assertNotEq(buffer[i], s); + } else { + // Case 2: search value was found + assertEq(buffer[result], s); + // search value is not present anywhere after the found location + for (uint256 i = result + 1; i < buffer.length; ++i) assertNotEq(buffer[i], s); + } } function testLastIndexOf(bytes memory buffer, bytes1 s, uint256 pos) public pure { From cfbb2e6493dcae8f150448ccf20ddff1a6d8f7c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Tue, 15 Jul 2025 09:27:56 -0600 Subject: [PATCH 6/8] Update .changeset/witty-hats-flow.md Co-authored-by: Hadrien Croubois --- .changeset/witty-hats-flow.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/witty-hats-flow.md b/.changeset/witty-hats-flow.md index 1c567f5526e..757b8af24de 100644 --- a/.changeset/witty-hats-flow.md +++ b/.changeset/witty-hats-flow.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': patch --- -`Bytes`: Fix an issue when calling `lastIndexOf(bytes,byte,uint256)` with an empty buffer and a lookup position that is not 2²⁵⁶-1. +`Bytes`: Fix `lastIndexOf(bytes,byte,uint256)` with empty buffers and finite position to correctly return `type(uint256).max` instead of accessing uninitialized memory sections. From df473afe8ca89300a573216951329df5ad0669c8 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 16 Jul 2025 22:04:11 +0200 Subject: [PATCH 7/8] use Math.saturatingAdd to more closelly match original syntax --- contracts/utils/Bytes.sol | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/contracts/utils/Bytes.sol b/contracts/utils/Bytes.sol index b2f33d80511..3631f685ef1 100644 --- a/contracts/utils/Bytes.sol +++ b/contracts/utils/Bytes.sol @@ -57,9 +57,7 @@ library Bytes { */ function lastIndexOf(bytes memory buffer, bytes1 s, uint256 pos) internal pure returns (uint256) { unchecked { - uint256 length = buffer.length; - uint256 end = Math.ternary(pos == type(uint256).max, length, Math.min(pos + 1, length)); - for (uint256 i = end; i > 0; --i) { + for (uint256 i = Math.min(Math.saturatingAdd(pos, 1), buffer.length); i > 0; --i) { if (bytes1(_unsafeReadBytesOffset(buffer, i - 1)) == s) { return i - 1; } From 6eb86f48fb1267b7569987ef5ca851bf2b4d5ae6 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 16 Jul 2025 22:09:55 +0200 Subject: [PATCH 8/8] minimize changes --- contracts/utils/Bytes.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/utils/Bytes.sol b/contracts/utils/Bytes.sol index 3631f685ef1..6e03c7b8eed 100644 --- a/contracts/utils/Bytes.sol +++ b/contracts/utils/Bytes.sol @@ -57,7 +57,8 @@ library Bytes { */ function lastIndexOf(bytes memory buffer, bytes1 s, uint256 pos) internal pure returns (uint256) { unchecked { - for (uint256 i = Math.min(Math.saturatingAdd(pos, 1), buffer.length); i > 0; --i) { + uint256 length = buffer.length; + for (uint256 i = Math.min(Math.saturatingAdd(pos, 1), length); i > 0; --i) { if (bytes1(_unsafeReadBytesOffset(buffer, i - 1)) == s) { return i - 1; }