Fix bug in Bytes.lastIndexOf when array is empty and position is not 2²⁵⁶-1 (#5797)

Amxx · ernestognw · Amxx · commit 84a600ba7228 · 2025-07-16T22:45:46.000+02:00
Co-authored-by: Ernesto García &lt;ernestognw@gmail.com&gt;
Signed-off-by: Hadrien Croubois &lt;hadrien.croubois@gmail.com&gt;
diff --git a/.changeset/witty-hats-flow.md b/.changeset/witty-hats-flow.md
@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': patch
+---
+
+`Bytes`: Fix `lastIndexOf(bytes,byte,uint256)` with empty buffers and finite position to correctly return `type(uint256).max` instead of accessing uninitialized memory sections.
diff --git a/contracts/utils/Bytes.sol b/contracts/utils/Bytes.sol
@@ -58,8 +58,7 @@ 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) {
+            for (uint256 i = Math.min(Math.saturatingAdd(pos, 1), length); i > 0; --i) {
                 if (bytes1(_unsafeReadBytesOffset(buffer, i - 1)) == s) {
                     return i - 1;
                 }
diff --git a/test/utils/Bytes.t.sol b/test/utils/Bytes.t.sol
@@ -0,0 +1,219 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.20;
+
+import {Test} from "forge-std/Test.sol";
+import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";
+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 {
+        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 {
+        uint256 result = Bytes.indexOf(buffer, s, pos);
+
+        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 {
+        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 {
+        uint256 result = Bytes.lastIndexOf(buffer, s, pos);
+
+        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);
+        }
+    }
+
+    // SLICES
+    function testSliceWithStartOnly(bytes memory buffer, uint256 start) public pure {
+        bytes memory originalBuffer = bytes.concat(buffer);
+        bytes memory result = buffer.slice(start);
+
+        // Original buffer was not modified
+        assertEq(buffer, originalBuffer);
+
+        // Should return bytes from start to end
+        assertEq(result.length, Math.saturatingSub(buffer.length, start));
+
+        // Verify content matches
+        for (uint256 i = 0; i < result.length; ++i) {
+            assertEq(result[i], buffer[start + i]);
+        }
+    }
+
+    function testSlice(bytes memory buffer, uint256 start, uint256 end) public pure {
+        bytes memory originalBuffer = bytes.concat(buffer);
+        bytes memory result = buffer.slice(start, end);
+
+        // Original buffer was not modified
+        assertEq(buffer, originalBuffer);
+
+        // Calculate expected bounds after sanitization
+        uint256 sanitizedEnd = Math.min(end, buffer.length);
+        uint256 sanitizedStart = Math.min(start, sanitizedEnd);
+        uint256 expectedLength = sanitizedEnd - sanitizedStart;
+
+        assertEq(result.length, expectedLength);
+
+        // Verify content matches when there's content to verify
+        for (uint256 i = 0; i < result.length; ++i) {
+            assertEq(result[i], buffer[sanitizedStart + i]);
+        }
+    }
+
+    function testSpliceWithStartOnly(bytes memory buffer, uint256 start) public pure {
+        bytes memory originalBuffer = bytes.concat(buffer);
+        bytes memory result = buffer.splice(start);
+
+        // Result should be the same object as input (modified in place)
+        assertEq(result, buffer);
+
+        // Should contain bytes from start to end, moved to beginning
+        assertEq(result.length, Math.saturatingSub(originalBuffer.length, start));
+
+        // Verify content matches moved content
+        for (uint256 i = 0; i < result.length; ++i) {
+            assertEq(result[i], originalBuffer[start + i]);
+        }
+    }
+
+    function testSplice(bytes memory buffer, uint256 start, uint256 end) public pure {
+        bytes memory originalBuffer = bytes.concat(buffer);
+        bytes memory result = buffer.splice(start, end);
+
+        // Result should be the same object as input (modified in place)
+        assertEq(result, buffer);
+
+        // Calculate expected bounds after sanitization
+        uint256 sanitizedEnd = Math.min(end, originalBuffer.length);
+        uint256 sanitizedStart = Math.min(start, sanitizedEnd);
+        uint256 expectedLength = sanitizedEnd - sanitizedStart;
+
+        assertEq(result.length, expectedLength);
+
+        // Verify content matches moved content
+        for (uint256 i = 0; i < result.length; ++i) {
+            assertEq(result[i], originalBuffer[sanitizedStart + i]);
+        }
+    }
+
+    // REVERSE BITS
+    function testSymbolicReverseBytes32(bytes32 value) public pure {
+        assertEq(Bytes.reverseBytes32(Bytes.reverseBytes32(value)), value);
+    }
+
+    function testSymbolicReverseBytes16(bytes16 value) public pure {
+        assertEq(Bytes.reverseBytes16(Bytes.reverseBytes16(value)), value);
+    }
+
+    function testSymbolicReverseBytes16Dirty(bytes16 value) public pure {
+        assertEq(Bytes.reverseBytes16(Bytes.reverseBytes16(_dirtyBytes16(value))), value);
+        assertEq(Bytes.reverseBytes16(_dirtyBytes16(Bytes.reverseBytes16(value))), value);
+    }
+
+    function testSymbolicReverseBytes8(bytes8 value) public pure {
+        assertEq(Bytes.reverseBytes8(Bytes.reverseBytes8(value)), value);
+    }
+
+    function testSymbolicReverseBytes8Dirty(bytes8 value) public pure {
+        assertEq(Bytes.reverseBytes8(Bytes.reverseBytes8(_dirtyBytes8(value))), value);
+        assertEq(Bytes.reverseBytes8(_dirtyBytes8(Bytes.reverseBytes8(value))), value);
+    }
+
+    function testSymbolicReverseBytes4(bytes4 value) public pure {
+        assertEq(Bytes.reverseBytes4(Bytes.reverseBytes4(value)), value);
+    }
+
+    function testSymbolicReverseBytes4Dirty(bytes4 value) public pure {
+        assertEq(Bytes.reverseBytes4(Bytes.reverseBytes4(_dirtyBytes4(value))), value);
+        assertEq(Bytes.reverseBytes4(_dirtyBytes4(Bytes.reverseBytes4(value))), value);
+    }
+
+    function testSymbolicReverseBytes2(bytes2 value) public pure {
+        assertEq(Bytes.reverseBytes2(Bytes.reverseBytes2(value)), value);
+    }
+
+    function testSymbolicReverseBytes2Dirty(bytes2 value) public pure {
+        assertEq(Bytes.reverseBytes2(Bytes.reverseBytes2(_dirtyBytes2(value))), value);
+        assertEq(Bytes.reverseBytes2(_dirtyBytes2(Bytes.reverseBytes2(value))), value);
+    }
+
+    // Helpers
+    function _dirtyBytes16(bytes16 value) private pure returns (bytes16 dirty) {
+        assembly ("memory-safe") {
+            dirty := or(value, shr(128, not(0)))
+        }
+    }
+
+    function _dirtyBytes8(bytes8 value) private pure returns (bytes8 dirty) {
+        assembly ("memory-safe") {
+            dirty := or(value, shr(192, not(0)))
+        }
+    }
+
+    function _dirtyBytes4(bytes4 value) private pure returns (bytes4 dirty) {
+        assembly ("memory-safe") {
+            dirty := or(value, shr(224, not(0)))
+        }
+    }
+
+    function _dirtyBytes2(bytes2 value) private pure returns (bytes2 dirty) {
+        assembly ("memory-safe") {
+            dirty := or(value, shr(240, not(0)))
+        }
+    }
+}
diff --git a/test/utils/Bytes.test.js b/test/utils/Bytes.test.js
@@ -20,39 +20,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,
+      );
     });
   });
 
@@ -65,7 +81,7 @@ describe('Bytes', function () {
       })) {
         it(descr, async function () {
           const result = ethers.hexlify(lorem.slice(start));
-          expect(await this.mock.$slice(lorem, start)).to.equal(result);
+          await expect(this.mock.$slice(lorem, start)).to.eventually.equal(result);
         });
       }
     });
@@ -80,7 +96,7 @@ 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);
+          await expect(this.mock.$slice(lorem, start, ethers.Typed.uint256(end))).to.eventually.equal(result);
         });
       }
     });

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +'openzeppelin-solidity': patch
 +---
++
 +`Bytes`: Fix `lastIndexOf(bytes,byte,uint256)` with empty buffers and finite position to correctly return `type(uint256).max` instead of accessing uninitialized memory sections.