Add bytes memory version of Math.modExp

[ernestognw]

Fixes #4880
Fixes LIB-1209

PR Checklist

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

[changeset-bot]

⚠️ No Changeset found

Latest commit: 78a5598

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

Click here to learn what changesets are, and how to add one.

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

[ernestognw]

I updated tryModExp to use the result as a pointer to avoid allocating extra memory. The function should be memory safe because it's not violating the memory safetiness rules and the only effect is that it's leaving dirty bytes between the result and the next memory pointer. Those shouldn't be reachable but I wonder if we should warn about it.

[ernestognw]

The ranges were selected to fit within the max BigInt.

[Amxx]

These values are actually pretty small. The max value they take is

b: 1048576
e: 1048576
m: 6277101735386680763835789423207666416102355444464034512896

In comparaison, type(uint256).max is

115792089237316195423570985008687907853269984665640564039457584007913129639935

[Amxx]

which makes me wonder:

• if all these fit in a uint256, why only test them by the bytes version, and not with the uint256 version
• This looks like fuzzing, but with very specific values (only powers of 2).

Since the values used here are not bigger than then one supported by the foundry fuzzing, are way slower to run, and do not cover a bigger space ... do we need that at all ? I'd remove this part of the tests.

[ernestognw]

Tests passing but coverage dropped. I tried mocking the address(5) with Hardhat's setCode utility but it didn't work:

it('is correctly reverting if the underlying call reverts', async function () {
  const base = 3n;
  const exponent = 200n;
  const modulus = 50n;

  await setCode('0x0000000000000000000000000000000000000005', '0xFD'); // Straight revert

  await expect(this.mock[this.fn](base, exponent, modulus)).to.be.reverted();
});

The revert's missing stack arguments but it should revert anyway and it's not.
I see no way of testing a failed underlying call to the modExp address and that's why the coverage is dropping. Not sure how to proceed tbh

[ernestognw]

Alternatively, we can revert in any case try fails and assume m is 0.

function modExp(bytes memory b, bytes memory e, bytes memory m) internal view returns (bytes memory) {
    (bool success, bytes memory result) = tryModExp(b, e, m);
    if (!success) {
        Panic.panic(Panic.DIVISION_BY_ZERO);
    }
    return result;
}

We don't recommend relying on revert messages so we can change it if there's a proven way to make the modexp call revert. What do you think @Amxx?

[Amxx]

function modExp(bytes memory b, bytes memory e, bytes memory m) internal view returns (bytes memory) {
    (bool success, bytes memory result) = tryModExp(b, e, m);
    if (!success) {
        Panic.panic(Panic.DIVISION_BY_ZERO);
    }
    return result;
}

I'm 100% ok with that. If we do it here, we should do it in both versions of modExp (uint256 and bytes memory).

[ernestognw]

Somewhat agree with 12ea8eb but I think we should keep those test cases. The main reason is that the only benefit from the ethers tests vs Foundry is that we can test outside of the uint256 space in Javascript, and that's the point of the bytes memory implementation vs. the uint256 one.

I'll revert the change but reconfigure the function to avoid performing excessive operations.

[ernestognw]

I readded the large bytes tests with a faster js modexp implementation. I used these params for the tests:

product(
  range(320, 513, 64).map(e => [2n ** BigInt(e) + 1n, e]),
  range(320, 513, 64).map(e => [2n ** BigInt(e) + 1n, e]),
  range(320, 513, 64).map(e => [2n ** BigInt(e) + 1n, e]),
)

In this way, we're using large numbers for all arguments of modexp and the tests are still running fast.

Note I readded the +1, otherwise because all the numbers are powers of 2, the mod N always resulted in 0.

[Amxx]

This works assuming the current solidity behavior:

• result is always allocated (at the free memory pointer)

If solidity ever changes its behavior, reusing memory gaps to put new arrays, then this would set the free memory pointer in an unsafe way.

That is unlikelly to happen for a white. Definitelly not in 0.8.x. So I think its ok.