-
Notifications
You must be signed in to change notification settings - Fork 12.4k
Add Math.modExp and a Panic library
#3298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
91e39eb
712a0f3
77f33ea
4683f26
7489ce4
d576641
0d78d29
5d99ed8
acb16f2
d7e81cf
85187dd
d458765
01badeb
a1c1439
d81d69d
6e8cced
fd7b8de
26a036e
4ba0a29
988c950
c08ac50
98f7994
f634aab
66a0c1f
eecd818
1583160
19ead8e
8cf355f
113e85e
4e1cf0d
6d7c154
84b285d
9e73f46
76c9afa
1ff0776
f84b1b6
fe32a38
4accc2e
cfd80e9
526d6b9
3718090
cd2f2e9
104002e
d149ea6
05aa60e
f352681
32ea4bb
2e962c8
275c959
24cd52a
50374a1
ee91836
d13e52d
969e259
e64c3f9
06220be
9137bae
81e8ba0
2b72050
2fc20d4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,14 @@ library Math { | |
| * @dev Muldiv operation overflow. | ||
| */ | ||
| error MathOverflowedMulDiv(); | ||
| /** | ||
| * @dev Modulus zero in Mod Exp operation. | ||
| */ | ||
| error MathModulusEqualsZero(); | ||
| /** | ||
| * @dev Static call to Mod Exp precompile fails. | ||
| */ | ||
| error MathModExpCannotBeCalculated(); | ||
|
|
||
| enum Rounding { | ||
| Floor, // Toward negative infinity | ||
|
|
@@ -276,14 +284,21 @@ library Math { | |
| } | ||
|
|
||
| /** | ||
| * @dev Returns the modular exponentiation of the specified base, exponent and modulus (b ** e % m). | ||
| * @dev Returns the modular exponentiation of the specified base, exponent and modulus (b ** e % m) | ||
| * | ||
| * If the modulus is 0 or if the EIP-198 precompile reverts, return 0 (which is obviously an invalid result). | ||
| * Requirements: | ||
| * - modulus can't be zero | ||
| * - result should be obtained successfully | ||
| */ | ||
| function modExp(uint256 b, uint256 e, uint256 m) internal view returns (uint256) { | ||
| if (m == 0) return 0; | ||
| if (m == 0) { | ||
| revert MathModulusEqualsZero(); | ||
| } | ||
| (bool success, bytes memory result) = (address(5).staticcall(abi.encode(32, 32, 32, b, e, m))); | ||
| return success ? abi.decode(result, (uint256)) : 0; | ||
| if (!success) { | ||
| revert MathModExpCannotBeCalculated(); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't work, I tried different gasLimit values in the following test; It reverts as out of gas but doesn't trigger the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, I guess that is because if the precompile has not enough gas, than the 1/64 that remains after that are not enough to check success and revert with a custom error |
||
| return abi.decode(result, (uint256)); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default behavior of the precompile is to return 0 when
m == 0. Wouldn't make more sense to respect the precompile behavior and treat it as a noop? @AmxxThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, returning 0 if m == 0 would be like returning 0 when dividing something by 0.
Maybe at the precompile level its ok to do so, because they expect callers of the precompile will check they are not doing anything stupid, and because they don't want to handle reverts in precompile (just like ecrecover will return address(0) with invalid signatures).
But we are at the library level, and I think we should revert if someone tries to divide by 0 (or do modulo 0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah but why is it stupid to allow
% 0? If there's a concrete reason why this is dangerous, then let's keep it.The thing is that I haven't come up with any good reason for either other than that it's difficult to remove the check once released 😅
I agree with restricting the precompile at the library level, but the main difference with ECDSA is that it's dangerous to use it without the wrapper because there's a way of replaying a signature.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x % 0is the remainder of the division of x by 0. What more can I say ?The result should be a positive integer that is
< 0.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And? How is someone stupid for returning 0?
I am really sorry if this still doesn't make sense to you but I can't see a single reason why that is dangerous so that someone using it is stupid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you seriously consider that
mulDiv(1,1,0)should return 0 as well?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But 0 div panics by default. modexp doesn't.
As I said, I don't have reasons for saying it's okay or not. Doesn't seem harmful and restricting is irreversible.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interrestingly,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continuing the discussion on slack.