fix(abi-encoder): Implement recursion depth limit for parameter encoding and decoding#4717
Open
sergei-boiko-trustwallet wants to merge 3 commits intomasterfrom
Open
fix(abi-encoder): Implement recursion depth limit for parameter encoding and decoding#4717sergei-boiko-trustwallet wants to merge 3 commits intomasterfrom
sergei-boiko-trustwallet wants to merge 3 commits intomasterfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a recursion-depth guard to EVM ABI proto/token/param-type conversions to prevent stack overflows on deeply nested inputs, and refactors ABI helpers to propagate failures via AbiResult instead of silently defaulting.
Changes:
- Introduces
MAX_RECURSION_DEPTHand enforces it across recursive ABI encode/decode conversion paths. - Refactors recursive conversion helpers (
*_to_proto,*_from_proto, array/tuple handling) to use step functions with explicitcurrent_depthand error propagation. - Updates entry points to return
AbiResult<String>for function signature generation and adds regression tests for recursion depth failures.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| rust/tw_evm/tests/abi_encoder.rs | Adds tests that assert encoding/decoding/signature generation fail with a non-empty error message when nesting exceeds the recursion limit. |
| rust/tw_evm/src/modules/abi_encoder.rs | Implements depth checking and converts previously infallible proto conversions into AbiResult-returning, depth-aware step functions. |
| rust/tw_evm/src/evm_entry.rs | Updates function-signature API to return AbiResult and maps proto deserialization failures to ABI decoding errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rust/tw_evm/tests/abi_encoder.rs
Outdated
Comment on lines
+717
to
+732
| // 20 levels of tuple nesting. | ||
| let deep_param = deeply_nested_param_type(20); | ||
| let input = Proto::FunctionGetTypeInput { | ||
| function_name: "transfer".into(), | ||
| inputs: vec![deep_param], | ||
| }; | ||
|
|
||
| AbiEncoder::<StandardEvmContext>::get_function_signature_from_proto(input) | ||
| .expect_err("Expected error due to too-deep param"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_decode_params_max_recursion_depth() { | ||
| // 20 levels of tuple nesting. | ||
| let deep_param = deeply_nested_param_type(20); | ||
| let input = Proto::ParamsDecodingInput { |
There was a problem hiding this comment.
These test comments mention "tuple nesting", but deeply_nested_param_type builds a param type with array nesting (not tuples). Please adjust the comments to avoid confusion when interpreting the recursion-depth tests.
Binary size comparison➡️ aarch64-apple-ios: - 14.34 MB
+ 14.35 MB +16 KB➡️ aarch64-apple-ios-sim: - 14.34 MB
+ 14.36 MB +16 KB➡️ aarch64-linux-android: - 18.77 MB
+ 18.79 MB +21 KB➡️ armv7-linux-androideabi: - 16.20 MB
+ 16.22 MB +23 KB➡️ wasm32-unknown-emscripten: - 13.68 MB
+ 13.71 MB +25 KB |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces significant improvements to ABI (Application Binary Interface) encoding and decoding logic in the EVM (Ethereum Virtual Machine) module. The main focus is on making ABI conversions more robust by introducing recursion depth checks to prevent stack overflows and ensuring errors are properly propagated using
AbiResult. Additionally, several functions now return richer error information, and the code has been refactored for better maintainability and safety.The most important changes are:
ABI Conversion Safety and Robustness
MAX_RECURSION_DEPTH) and depth checking in all recursive ABI conversion functions to prevent stack overflows and handle deeply nested ABI structures safely. (rust/tw_evm/src/modules/abi_encoder.rs)param_type_to_proto,param_type_from_proto,token_to_proto,array_to_proto, etc.) to accept and increment acurrent_depthparameter, invokingcheck_depthat each step. (rust/tw_evm/src/modules/abi_encoder.rs)Error Handling Improvements
AbiResult, ensuring that errors are properly propagated and handled throughout the ABI encoding/decoding process. (rust/tw_evm/src/evm_entry.rs,rust/tw_evm/src/modules/abi_encoder.rs)get_function_signature_from_protoand related methods to propagate errors instead of panicking or returning default values. (rust/tw_evm/src/evm_entry.rs,rust/tw_evm/src/modules/abi_encoder.rs)Refactoring and Consistency
AbiResultfor return types, improving code clarity and error traceability. (rust/tw_evm/src/evm_entry.rs,rust/tw_evm/src/modules/abi_encoder.rs)param_to_proto_step,token_to_proto_step, etc.) that handle recursion and error checking. (rust/tw_evm/src/modules/abi_encoder.rs)These changes make the ABI encoding/decoding logic safer and more maintainable, reducing the risk of runtime errors due to deeply nested or malformed ABI structures.