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

[Amxx]

Fix bug introduced in #5252 (v5.2)

Bug description

If the buffer is empty and if pos is not type(uint256).max

• in the computation of Math.min(pos, length - 1) + 1, length-1 overflow and return 2²⁵⁶-1.
• taking the min between pos and that will return something that is NOT 2²⁵⁶-1.
• adding 1 to it will NOT return 0.
• the loop will start at a non-0 index, and try to access data when the buffer is actually empty.

This will return unpredictable results, if s can be found after the buffer. For example, if the memory after the is clean (zero) and you do lastIndexOf('0x', '0x00', 17) ... it will return 17 instead of the expected 2²⁵⁶-1 (not found)

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

🦋 Changeset detected

Latest commit: 6eb86f4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[levi-sledd]

This fix looks good to me.

[vesselinux]

@ernest @hadrien cc @levi The fix LGTM. Just a nit: using the ternary operator, although correct, is not super readable IMO i.e., the logic is not immediately clear. Have you considered the following simpler syntax instead?

uint256 end;
if(pos == type(uint256).max) {
    end = length;
} else {
    end = Math.min(pos + 1, length);
}

[Amxx]

The if / else syntax generates a conditional jump that is quite expensive. So does the ternary operator c ? a : b; that I otherwise like a lot.

Math.ternary does the same using xor and mul opcode that are really cheap. That is why we favor that (branchless) syntax when possible

[Amxx]

This might be more readable, and just as effective.

Suggested change

	uint256 end = Math.ternary(pos == type(uint256).max, length, Math.min(pos + 1, length));
	uint256 end = Math.min(Math.saturatingAdd(pos, 1), length);

@ernestognw @vesselinux @levi-sledd WDYT ?

[levi-sledd]

This is indeed equivalent and more readable.

[vesselinux]

@Amxx Ahh, sorry to say but I find this to be even less readable haha. That's maybe because the overflow logic is buried inside the saturatingAdd thus making handling of edge cases less obvious. At the same time, I fully accept the arguments against my if-else suggestion.

The above said, I appreciate that readability is subjective, so feel free to adopt whichever of the two suggested variants the Contracts team like best. Indeed both are functionally equivalent and that's what matters at the end of the day.

[ernestognw]

the overflow logic is buried inside the saturatingAdd thus making handling of edge cases less obvious

Interesting point of view. imo "burying the logic" should be the goal of the saturatingAdd function (and generally for any other library fn). In fact, I do prefer the saturatingAdd given I'd recommend using it to other developers in a similar situation as this.