-
Notifications
You must be signed in to change notification settings - Fork 82
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
fix: reachable panic due to out-of-memory in modexp
precompile
#689
Conversation
2952de0
to
3c98fa7
Compare
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.
This PR currently does not address the issue. The problem is that we eagerly allocate memory based on the lengths in the input (this happens at engine-precompiles/src/modexp.rs:70
, but it's outside the diff so I can't put the comment there). I think our parsing of the lengths was fine, it's what we do with those lengths is currently a problem.
modexp
precompilemodexp
precompile [WIP]
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.
Nice! I like how simple this fix is, and I do think that this fixes the problem 🚀
Just some minor comments on the tests.
Co-authored-by: Michael Birch <[email protected]>
Co-authored-by: Michael Birch <[email protected]>
Co-authored-by: Michael Birch <[email protected]>
Co-authored-by: Michael Birch <[email protected]>
Co-authored-by: Michael Birch <[email protected]>
Co-authored-by: Michael Birch <[email protected]>
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.
LGTM, nice work @0x3bfc
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.
Overall looks good 👍🏻
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.
Have you thought concatenating base
with specific endian encoding method?
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.
My understanding of this part of the code is very limited so please take my questions more as my trying to learn.
* chore: trigger `capacity overflow` and `attempt to add with overflow` panics * fix: tests and add extra edge cases tests * fix: clippy error * fix: update checks for out of memory and capacity errors * fix: revert to old gas estimate * fix: minor test refactor * fix: oom but it fails with capacity overflow * fix: add capacity error check * fix: never returns errors from precompiles * chore: clean up and add more tests * chore: more tests * refactor: zero modulus check * fix: remove unncessary check * Update engine-precompiles/src/modexp.rs Co-authored-by: Michael Birch <[email protected]> * Update engine-precompiles/src/modexp.rs Co-authored-by: Michael Birch <[email protected]> * fix: add helper function for test inputs * fix: add integration tests * Update engine-precompiles/src/modexp.rs Co-authored-by: Michael Birch <[email protected]> * Update engine-precompiles/src/modexp.rs Co-authored-by: Michael Birch <[email protected]> * Update engine-precompiles/src/modexp.rs Co-authored-by: Michael Birch <[email protected]> * Update engine-precompiles/src/modexp.rs Co-authored-by: Michael Birch <[email protected]> * Update engine-precompiles/src/modexp.rs Co-authored-by: Michael Birch <[email protected]> * Update engine-precompiles/src/modexp.rs Co-authored-by: Michael Birch <[email protected]> * fix: `StandardPrecompiles.sol` compilation errors * fix: revert to old implementation of `StandardPrecompiles` * fix: integration tests * refactor: `generate_modexp_test_input` * fix: adding struct for test input --------- Co-authored-by: Michael Birch <[email protected]>
* chore: trigger `capacity overflow` and `attempt to add with overflow` panics * fix: tests and add extra edge cases tests * fix: clippy error * fix: update checks for out of memory and capacity errors * fix: revert to old gas estimate * fix: minor test refactor * fix: oom but it fails with capacity overflow * fix: add capacity error check * fix: never returns errors from precompiles * chore: clean up and add more tests * chore: more tests * refactor: zero modulus check * fix: remove unncessary check * Update engine-precompiles/src/modexp.rs Co-authored-by: Michael Birch <[email protected]> * Update engine-precompiles/src/modexp.rs Co-authored-by: Michael Birch <[email protected]> * fix: add helper function for test inputs * fix: add integration tests * Update engine-precompiles/src/modexp.rs Co-authored-by: Michael Birch <[email protected]> * Update engine-precompiles/src/modexp.rs Co-authored-by: Michael Birch <[email protected]> * Update engine-precompiles/src/modexp.rs Co-authored-by: Michael Birch <[email protected]> * Update engine-precompiles/src/modexp.rs Co-authored-by: Michael Birch <[email protected]> * Update engine-precompiles/src/modexp.rs Co-authored-by: Michael Birch <[email protected]> * Update engine-precompiles/src/modexp.rs Co-authored-by: Michael Birch <[email protected]> * fix: `StandardPrecompiles.sol` compilation errors * fix: revert to old implementation of `StandardPrecompiles` * fix: integration tests * refactor: `generate_modexp_test_input` * fix: adding struct for test input --------- Co-authored-by: Michael Birch <[email protected]>
## [2.9.0] 2023-04-05 ### Added - Enabled XCC for mainnet release by [@birchmd]. ([#694]) - Added `set_owner` contract method which sets the owner of the contract by [@hskang9]. ([#690]) - New variant of submit function `submit_with_args` which accepts additional arguments along with the transaction such as the max gas price a user is ready to pay by [@aleksuss]. ([#696]) - Added the ability to create and fund XCC sub-accounts from external NEAR accounts by [@birchmd]. ([#735]) ### Changes - Replaced `rjson` with `serde_json` by [@aleksuss]. ([#677]) - Changed owner intended contract methods to now require owner or the contract itself by [@hskang9]. ([#676]) ### Fixes - Fixed nonce incorrectly being incremented on an out of fund failure by [@joshuajbouw]. ([#671]) - Fixed a check in promise results before executing cross contract calls (XCC) callbacks by [@birchmd]. ([#693]) - Fixed a reachable panic in `receive_erc20_tokens` by [@0x3bfc]. ([#709]) - Fixed a lack of minimum size checks when instantiating a new `EthGas` object by [@lempire123]. ([#722]) - Fixed a lack of division by 0 checks in `EthGas::Div()` by [@lempire123]. ([#718]) - Fixed the validation of the return of `exports:storage_remove` by [@0x3bfc]. ([#712]) - Fixed missing account validations of NEAR account IDs by [@0x3bfc]. ([#703]) - Fixed a reachable panic in the `exitToNear` and `exitToEthereum` precompiles if the input amount is greater than 1^128 when cast from a `U256` to `u128` by [@0x3bfc]. ([#681]) - Fixed a reachable panic in `modExp` due to arithmetic overflow by [@0x3bfc]. ([#688]) - Fixed the ability attaching values to Aurora specific precompiles, this no longer is possible, by [@0x3bfc]. ([#714]) - Fixed a return error if an ecrecover signature length is not exactly 65 by [@0x3bfc]. ([#717]) - Fixed size checks on input array passed to `exitToNear` and `exitToEthereum` precompiles by [@0x3bfc]. ([#684]) - Fixed missing gas costs in `exitToNear` and `exitToEthereum` precompiles by [@lempire123]. ([#687]) - Fixed a reachable panic due to out of memory in the `modExp` precompile by [@0x3bfc]. ([#689]) - Fixed an assurance that the `sender_id` has a balance greater than the amount in `ft_transfer_call` by [@0x3bfc]. ([#708]) - Fixed returning `0x` when a length cannot be cast as `usize` instead of returning an error in the `modExp` precompile by [@birchmd]. ([#737]) - Miscellaneous minor fixes by [@0x3bfc]. ([#738]) --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Leandro Casuso Montero <[email protected]> Co-authored-by: Michael Birch <[email protected]> Co-authored-by: Alexey Lapitsky <[email protected]> Co-authored-by: Oleksandr Anyshchenko <[email protected]> Co-authored-by: Alexey Lapitsky <[email protected]> Co-authored-by: Hyungsuk Kang <[email protected]> Co-authored-by: Ahmed Ali <[email protected]> Co-authored-by: lempire123 <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Description
The precompile may panic if the exponent length is greater than the allocable space when calling
parse_bytes()
to retrieve the exponent length from the input. To trigger this panic the base length and modulus length must bezero
due to gas requirements.isize::MAX
(2^31−1 for the wasm32 platform), themodexp
precompile will panic withcapacity overflow
.isize::MAX
and there is not enough allocable space, themodexp
precompile will trigger a panic.