diff --git a/.changeset/witty-hats-flow.md b/.changeset/witty-hats-flow.md new file mode 100644 index 00000000000..757b8af24de --- /dev/null +++ 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 index b76198e8032..6e03c7b8eed 100644 --- 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 index be0bf783e95..ec2edc7564e 100644 --- a/test/utils/Bytes.t.sol +++ b/test/utils/Bytes.t.sol @@ -9,6 +9,76 @@ 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); 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); }); } });