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

Co-authored-by: Ernesto García <ernestognw@gmail.com>
Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>

.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.

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;
                 }

test/utils/Bytes.t.sol

@@ -0,0 +1,117 @@
+// 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]);
+        }
+    }
+}

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);
         });
       }
     });