fix(erc20): better handling of gas consumption within erc20 contract execution #2089
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2089 +/- ##
==========================================
- Coverage 63.99% 63.98% -0.02%
==========================================
Files 270 270
Lines 21116 21092 -24
==========================================
- Hits 13513 13495 -18
+ Misses 6664 6660 -4
+ Partials 939 937 -2
|
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe pull request includes extensive updates to the Nibiru project's Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (18)
x/evm/embeds/embeds.go (2)
65-71: LGTM: New TestERC20MaliciousName contract added.The addition of this contract aligns with the PR objectives and follows the existing code structure. The comment clearly describes its purpose.
Consider adding a brief note about how this contract is used in tests to the comment, for example:
// SmartContract_TestERC20MaliciousName is a test contract // which simulates malicious ERC20 behavior by adding gas intensive operation -// for function name() intended to attack funtoken creation +// for function name() intended to attack funtoken creation. +// This is used in tests to ensure proper handling of gas-intensive operations.
72-78: LGTM: New TestERC20MaliciousTransfer contract added.The addition of this contract aligns with the PR objectives and follows the existing code structure. The comment clearly describes its purpose.
Consider adding a brief note about how this contract is used in tests to the comment, for example:
// SmartContract_TestERC20MaliciousTransfer is a test contract // which simulates malicious ERC20 behavior by adding gas intensive operation // for function transfer() intended to attack funtoken conversion from erc20 to bank coin +// This is used in tests to ensure proper handling of gas-intensive operations during token transfers.x/evm/embeds/artifacts/contracts/TestERC20MaliciousName.sol/TestERC20MaliciousName.json (1)
1-302: Ensure proper isolation of the malicious test contract.Given that this contract is intentionally designed to be gas-intensive and potentially malicious, it's crucial to:
- Clearly document its purpose and potential risks.
- Ensure it's only used in controlled test environments.
- Implement safeguards in the main contract to handle potential gas-intensive ERC20 tokens.
- Regularly review and update gas limits and handling mechanisms based on the results of tests with this contract.
Consider adding a comment in the main contract or relevant test files explaining how this malicious contract is used and what safeguards are in place to prevent similar exploits in production.
CHANGELOG.md (7)
138-139: Ensure consistency in PR reference format.The format for referencing pull requests is inconsistent. Some use square brackets, while others use parentheses. Consider standardizing to one format throughout the changelog for better readability.
Line range hint
144-145: Consider expanding on the impact of this change.The entry "feat(perp): better handling of gas consumption within erc20 contract execution" could benefit from a brief explanation of how this affects users or developers. What improvements can they expect?
Line range hint
146-149: Ensure consistency in entry formatting.Some entries in this section have more detailed explanations, while others are very brief. Consider adding a short explanation to all entries for consistency and to provide more context to users.
Line range hint
151-151: Consider reviewing change categorization.The Features section is currently empty. Some changes in other sections, particularly in "State Machine Breaking," might qualify as new features. Consider reviewing the categorization to ensure users can easily identify new functionality.
Line range hint
155-156: Consider adding more context to the change.The entry "chore(proto): Add a buf.gen.rs.yaml and corresponding script to create Rust types for Wasm Stargate messages" could benefit from a brief explanation of why this change is important or how it affects developers.
Line range hint
180-207: LGTM: Well-documented dependency updates.The Dependencies section clearly lists all updated dependencies with their old and new versions. This is very helpful for users and developers.
Consider grouping similar dependencies (e.g., all Docker-related updates) for even better readability.
Line range hint
209-1595: LGTM: Comprehensive historical changelog.The entries for previous versions (v1.5.0 and earlier) provide a detailed history of changes, which is valuable for users and developers. The consistent structure across versions is commendable.
For future updates, consider adding a brief summary of major changes or themes for each version at the top of its section. This could help users quickly understand the significance of each release.
x/evm/keeper/erc20.go (3)
227-227: Enhance error message for gas addition failureThe error message
"error adding transient gas used to block"could be more descriptive. Including details about thegasUsedvalue or the context in which the error occurred can aid in debugging and provide clearer insights into the issue.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 227-227: x/evm/keeper/erc20.go#L227
Added line #L227 was not covered by tests
237-245: Improve error messages for EVM execution failuresFor better debugging and user experience, consider enhancing the error messages when the EVM execution fails. Providing more context, such as the reason for failure or relevant transaction details, can help developers quickly identify and resolve issues.
183-185: Increase test coverage for new code pathsThe static analysis tools indicate that the newly added lines are not covered by tests:
- Lines 223-224: Handling
gasUsedwhenevmRespisnil- Line 227: Error handling after adding gas used to block
- Line 232: Wrapping error from
ApplyEvmMsg- Line 235: Checking if
evmRespisnilAdding tests for these scenarios will improve reliability and ensure that edge cases are handled correctly.
Would you like assistance in writing unit tests to cover these code paths?
Also applies to: 219-225, 227-227, 232-235
x/evm/keeper/funtoken_from_erc20_test.go (4)
280-283: Nitpick: Improve function documentation comments.Consider updating the function comments to follow Go conventions for better readability and maintainability. Begin the comment with the function name and provide a clear description.
Apply this diff to enhance the comments:
-// TestCreateFunTokenFromERC20MaliciousName tries to create funtoken from a contract -// with a malicious (gas intensive) name() function. -// Fun token should fail creation with "out of gas" +// TestCreateFunTokenFromERC20MaliciousName tests the creation of a FunToken from an ERC20 contract +// with a malicious (gas-intensive) name() function. +// The FunToken creation should fail with an "out of gas" error. func (s *FunTokenFromErc20Suite) TestCreateFunTokenFromERC20MaliciousName() {
298-301: Nitpick: Simplify EIP55 address initialization.You can directly assign
deployResp.ContractAddrtoerc20Addrsince it's already of typeeth.EIP55Addr.Apply this diff to simplify the code:
-erc20Addr := eth.EIP55Addr{ - Address: deployResp.ContractAddr, -} +erc20Addr := deployResp.ContractAddr
320-323: Nitpick: Improve function documentation comments.As before, updating the comments to match Go conventions enhances readability.
Apply this diff to refine the comments:
-// TestFunTokenFromERC20MaliciousTransfer creates a funtoken from a contract -// with a malicious (gas intensive) transfer() function. -// Fun token should be created but sending from erc20 to bank should fail with out of gas +// TestFunTokenFromERC20MaliciousTransfer tests the creation of a FunToken from an ERC20 contract +// with a malicious (gas-intensive) transfer() function. +// The FunToken creation should succeed, but transferring tokens to the bank should fail with an "out of gas" error.
338-341: Nitpick: Simplify EIP55 address initialization.As with the previous function, you can simplify the assignment of
erc20Addr.Apply this diff:
-erc20Addr := eth.EIP55Addr{ - Address: deployResp.ContractAddr, -} +erc20Addr := deployResp.ContractAddrx/evm/keeper/msg_server.go (1)
710-723: Well-encapsulatedupdateBlockBloomfunctionThe addition of the
updateBlockBloomfunction enhances modularity by encapsulating the logic for updating the block bloom filter and log size. This improves code readability and maintainability.Consider adding unit tests for
updateBlockBloomto ensure its correctness and to prevent future regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- CHANGELOG.md (1 hunks)
- x/evm/embeds/artifacts/contracts/FunToken.sol/IFunToken.json (1 hunks)
- x/evm/embeds/artifacts/contracts/TestERC20MaliciousName.sol/TestERC20MaliciousName.json (1 hunks)
- x/evm/embeds/artifacts/contracts/TestERC20MaliciousTransfer.sol/TestERC20MaliciousTransfer.json (1 hunks)
- x/evm/embeds/contracts/TestERC20MaliciousName.sol (1 hunks)
- x/evm/embeds/contracts/TestERC20MaliciousTransfer.sol (1 hunks)
- x/evm/embeds/embeds.go (3 hunks)
- x/evm/embeds/embeds_test.go (1 hunks)
- x/evm/keeper/erc20.go (3 hunks)
- x/evm/keeper/funtoken_from_erc20_test.go (1 hunks)
- x/evm/keeper/msg_server.go (6 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
x/evm/keeper/erc20.go
[warning] 223-224: x/evm/keeper/erc20.go#L223-L224
Added lines #L223 - L224 were not covered by tests
[warning] 227-227: x/evm/keeper/erc20.go#L227
Added line #L227 was not covered by tests
[warning] 232-232: x/evm/keeper/erc20.go#L232
Added line #L232 was not covered by tests
[warning] 235-235: x/evm/keeper/erc20.go#L235
Added line #L235 was not covered by tests
🔇 Additional comments (25)
x/evm/embeds/embeds_test.go (1)
17-18: LGTM! The changes align well with the PR objectives.The addition of
TestERC20MaliciousNameandTestERC20MaliciousTransferto theTestLoadContractsfunction is a good enhancement. These new test cases align perfectly with the PR's goal of improving robustness against malicious ERC20 contracts.x/evm/embeds/artifacts/contracts/FunToken.sol/IFunToken.json (1)
3-3: LGTM: Contract name updated to reflect interface nature.The change from "FunToken" to "IFunToken" correctly indicates that this is now an interface rather than a concrete implementation. This aligns well with best practices for contract design and separation of concerns.
To ensure consistency across the codebase, please run the following script:
This will help identify any places where the renaming might have been missed or where further updates might be necessary.
✅ Verification successful
LGTM: All necessary references updated to IFunToken.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all references to FunToken have been updated to IFunToken where appropriate # Test: Search for any remaining references to FunToken rg -i 'funtoken' --type ts --type sol --type go # Test: Confirm the presence of IFunToken references rg -i 'ifuntoken' --type ts --type sol --type goLength of output: 152
Script:
#!/bin/bash # Description: Verify that all references to FunToken have been updated to IFunToken where appropriate # Test: Search for any remaining references to FunToken rg -i 'funtoken' -g '*.ts' -g '*.sol' -g '*.go' # Test: Confirm the presence of IFunToken references rg -i 'ifuntoken' -g '*.ts' -g '*.sol' -g '*.go'Length of output: 53808
x/evm/embeds/embeds.go (3)
28-31: LGTM: New malicious ERC20 contract JSONs added.The addition of these embedded JSON files for malicious ERC20 contracts aligns with the PR objectives and follows the existing naming conventions and embedding patterns.
87-88: LGTM: New contracts initialized in init function.The addition of MustLoad() calls for the new malicious contracts ensures they are properly initialized. This change is consistent with the existing pattern for contract initialization.
Line range hint
28-88: Summary: Enhancements for testing malicious ERC20 behavior.The changes in this file successfully introduce two new malicious ERC20 contracts for testing purposes. These additions align with the PR objectives and enhance the project's ability to test and handle gas-intensive operations in ERC20 interactions. The implementation follows existing code patterns and is well-documented.
To ensure these new contracts are properly referenced and utilized in tests, please run the following script:
This will help confirm that the new contracts are being utilized in the test suite as intended.
✅ Verification successful
Verification Successful: Malicious ERC20 contracts are properly utilized in tests.
The new contracts
TestERC20MaliciousNameandTestERC20MaliciousTransferare referenced inembeds_test.goandfuntoken_from_erc20_test.go, ensuring they are actively used in the test suite.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new malicious ERC20 contracts in tests # Test: Search for references to the new contracts in test files echo "Searching for TestERC20MaliciousName usage in tests:" rg -n "TestERC20MaliciousName" --type go --glob "*_test.go" echo "\nSearching for TestERC20MaliciousTransfer usage in tests:" rg -n "TestERC20MaliciousTransfer" --type go --glob "*_test.go"Length of output: 747
x/evm/embeds/artifacts/contracts/TestERC20MaliciousName.sol/TestERC20MaliciousName.json (3)
1-4: Contract metadata aligns with PR objectives.The contract name "TestERC20MaliciousName" and its source file location are consistent with the PR's goal of introducing malicious ERC20 contracts for testing gas-intensive methods, specifically the
name()function.
298-301: Bytecode present, consider security audit.The file includes both deployment and runtime bytecode, confirming successful compilation. Given the contract's intended malicious nature for testing, it's advisable to ensure a thorough security audit has been performed, particularly focusing on the gas consumption patterns in the
name()function implementation.To verify if a security audit has been mentioned or performed, run the following script:
5-297: Verify the implementation of thename()function.The ABI includes the
name()function, which is likely the focus of the malicious behavior for gas consumption testing. While the ABI structure is correct, it's crucial to verify the actual implementation of thename()function in the source code to ensure it meets the gas-intensive testing requirements.To verify the implementation of the
name()function, run the following script:x/evm/embeds/artifacts/contracts/TestERC20MaliciousTransfer.sol/TestERC20MaliciousTransfer.json (6)
1-302: Contract structure aligns with its testing purpose.The TestERC20MaliciousTransfer contract implements standard ERC20 functions, which is appropriate for its role in testing the system's handling of malicious ERC20 tokens. The contract structure serves its purpose well.
5-296: Fully compliant with ERC20 standard.The contract implements all required ERC20 functions and events, as well as optional functions like name, symbol, and decimals. This ensures that the test contract accurately represents a standard ERC20 token, which is essential for reliable testing.
244-266: Gas-intensive behavior aligns with testing objectives.The contract name and PR objectives suggest that this contract implements gas-intensive operations, likely in its transfer function. This design is appropriate for testing the system's ability to handle malicious ERC20 contracts that may attempt to exploit gas limits.
298-302: Bytecode and deployment information are complete and appropriate.The contract's bytecode is provided and appears to be complete. The absence of link references indicates that the contract is self-contained, which is suitable for a test contract. This ensures that the contract can be deployed consistently for testing purposes.
1-302: Well-designed test contract for malicious ERC20 behavior.This TestERC20MaliciousTransfer contract artifact is well-structured and fully compliant with the ERC20 standard. It appropriately implements intentionally malicious behavior for testing purposes, likely focusing on gas-intensive operations during transfers. The contract's design aligns well with the PR objectives of improving handling of malicious ERC20 contracts.
Key points:
- Fully ERC20 compliant
- Implements intentional malicious behavior for testing
- Likely includes gas-intensive operations in the transfer function
- Self-contained with no external dependencies
Ensure that this contract is strictly isolated to testing environments to prevent any unintended interactions in production systems.
1-302: Ensure proper isolation of malicious test contract.This contract is designed to exhibit malicious behavior for testing purposes. While this is valuable for ensuring system robustness, it's crucial to implement strict safeguards to prevent any accidental deployment or interaction with this contract in non-testing environments.
To verify proper isolation, run the following script:
✅ Verification successful
Malicious test contract is properly isolated.
All references to
TestERC20MaliciousTransferare confined to test and spec directories, ensuring that the malicious contract cannot be deployed or interacted with in non-testing environments.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any imports or uses of TestERC20MaliciousTransfer outside of test files # Test: Search for imports or uses of TestERC20MaliciousTransfer rg -g '!*test*' -g '!*spec*' 'TestERC20MaliciousTransfer'Length of output: 897
Script:
#!/bin/bash # Description: Check if embeds.go is referenced in non-test and non-spec files # Search for references to embeds.go outside of test and spec directories rg -g '!*test*' -g '!*spec*' 'embeds\.go' .Length of output: 45
Script:
#!/bin/bash # Description: Verify that embeds.go is not imported or used in any non-test and non-spec files # Search for import statements referencing embeds.go outside of test and spec directories rg -g '!(*test*|*spec*)' 'import\s+["'\''].*embeds\.go["'\'']' .Length of output: 66
CHANGELOG.md (2)
140-140: LGTM: Clear categorization of changes.The changes are well-categorized into "State Machine Breaking," "Features," "Improvements," "Bug Fixes," and "Dependencies." This organization helps users quickly identify the nature of each change.
Line range hint
157-157: LGTM: Clear and concise improvement.The entry "chore(goreleaser): update wasmvm to v1.4.0" is clear and provides useful information about a dependency update.
x/evm/embeds/contracts/TestERC20MaliciousTransfer.sol (1)
12-15:⚠️ Potential issueExcessive gas consumption in
transfermay cause transactions to failThe
_gasIntensiveOperation()in thetransferfunction runs a loop of 100,000 iterations, which may consume excessive gas and cause transactions to fail or be rejected due to gas limits.Consider verifying whether the gas consumption stays within acceptable limits. If necessary, adjust the loop count to prevent out-of-gas errors.
x/evm/embeds/contracts/TestERC20MaliciousName.sol (2)
12-16: Functionname()implementation looks goodThe override of the
name()function correctly includes the gas-intensive operation to simulate high computational cost, fulfilling its intended purpose for testing.
19-28: Verify the gas consumption of_gasIntensiveOperation()The
_gasIntensiveOperation()function runs a loop with 100,000 iterations performing multiple computations. This may consume substantial gas, potentially exceeding block gas limits and causing transactions to fail. Please verify that the gas consumption is acceptable for your testing scenarios.x/evm/keeper/erc20.go (1)
219-225: Verify gas usage calculation whenevmRespisnilWhen
evmRespisnil,gasUsedis assigned the value ofctx.GasMeter().Limit(). Please verify that this correctly represents the gas used in scenarios where the EVM response is unavailable. Incorrect gas accounting may lead to inconsistencies in gas tracking and potential issues with transaction processing.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 223-224: x/evm/keeper/erc20.go#L223-L224
Added lines #L223 - L224 were not covered by testsx/evm/keeper/msg_server.go (5)
87-87: Verify proper initialization oftxConfig.LogIndexIn line 87,
k.updateBlockBloom(ctx, evmResp, uint64(txConfig.LogIndex))is called withuint64(txConfig.LogIndex). Ensure thattxConfig.LogIndexis correctly initialized to prevent potential issues due to type conversion or uninitialized values.
654-655: Avoid potential nil pointer dereferenceThe check
if recipient != nilbefore accessingrecipient.Hex()ensures that you avoid a nil pointer dereference, which enhances the stability of the code whenrecipientis nil (e.g., during contract creation).
682-682: Confirm correct transaction type formattingFormatting
txTypeusingfmt.Sprintf("%d", txType)correctly converts the transaction type to a string for the event attribute. Ensure thattxTypeaccurately represents the transaction type and aligns with the expected values.
688-689: Correctly identify contract creation transactionsChecking
if recipient == nilto determine if the transaction is a contract creation is appropriate. This logic aligns with Ethereum's handling of contract creation transactions, where the recipient address is nil.
640-645: Ensure all calls toEmitEthereumTxEventsare updatedThe signature of
EmitEthereumTxEventshas changed. Verify that all invocations of this function throughout the codebase have been updated to prevent potential runtime errors.Run the following script to find all usages of
EmitEthereumTxEvents:✅ Verification successful
EmitEthereumTxEvents Invocations Verified Successfully
All calls to
EmitEthereumTxEventsare updated according to the new signature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to EmitEthereumTxEvents and ensure parameters are updated. # Search for the function usage. Expectation: All calls reflect the new signature. rg 'EmitEthereumTxEvents\(' -A 2Length of output: 442
| constructor(string memory name, string memory symbol, uint8 decimals_) | ||
| ERC20(name, symbol) { | ||
| _mint(msg.sender, 1000000 * 10**18); | ||
| } |
There was a problem hiding this comment.
Remove unused parameter decimals_ or utilize it to set token decimals
The decimals_ parameter in the constructor is not used, which may lead to confusion. If you intend to set a custom decimals value, consider overriding the decimals() function to return decimals_. Otherwise, remove the parameter to simplify the constructor.
Apply this diff to remove the unused parameter:
- constructor(string memory name, string memory symbol, uint8 decimals_)
+ constructor(string memory name, string memory symbol)
ERC20(name, symbol) {
_mint(msg.sender, 1000000 * 10**18);
}Alternatively, override the decimals() function to set a custom decimals value:
constructor(string memory name, string memory symbol, uint8 decimals_)
ERC20(name, symbol) {
_mint(msg.sender, 1000000 * 10 ** decimals_);
+ _decimals = decimals_;
}
+ uint8 private _decimals;
+ function decimals() public view virtual override returns (uint8) {
+ return _decimals;
+ }Committable suggestion was skipped due to low confidence.
| constructor(string memory name, string memory symbol, uint8 decimals_) | ||
| ERC20(name, symbol) { | ||
| _mint(msg.sender, 1000000 * 10**18); | ||
| } |
There was a problem hiding this comment.
Remove unused parameter decimals_ in constructor
The parameter decimals_ is declared in the constructor but not utilized within the function body. This can lead to confusion or potential errors. Consider removing it if it's not needed or using it appropriately.
Apply this diff to remove the unused parameter:
-constructor(string memory name, string memory symbol, uint8 decimals_)
+constructor(string memory name, string memory symbol)
ERC20(name, symbol) {
_mint(msg.sender, 1000000 * 10**18);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor(string memory name, string memory symbol, uint8 decimals_) | |
| ERC20(name, symbol) { | |
| _mint(msg.sender, 1000000 * 10**18); | |
| } | |
| constructor(string memory name, string memory symbol) | |
| ERC20(name, symbol) { | |
| _mint(msg.sender, 1000000 * 10**18); | |
| } |
| deps := evmtest.NewTestDeps() | ||
|
|
||
| s.T().Log("Deploy ERC20MaliciousName") | ||
| metadata := keeper.ERC20Metadata{ | ||
| Name: "erc20name", | ||
| Symbol: "TOKEN", | ||
| Decimals: 18, | ||
| } | ||
| deployResp, err := evmtest.DeployContract( | ||
| &deps, embeds.SmartContract_TestERC20MaliciousName, | ||
| metadata.Name, metadata.Symbol, metadata.Decimals, | ||
| ) | ||
| s.Require().NoError(err) | ||
|
|
There was a problem hiding this comment.
Issue: Add error handling after contract deployment.
If the deployment of the malicious ERC20 contract fails, subsequent steps may cause a panic. It's safer to check the error immediately after deployment.
Apply this diff to handle the error appropriately:
deployResp, err := evmtest.DeployContract(
&deps, embeds.SmartContract_TestERC20MaliciousName,
metadata.Name, metadata.Symbol, metadata.Decimals,
)
-s.Require().NoError(err)
+if err != nil {
+ s.T().Fatalf("Failed to deploy ERC20MaliciousName contract: %v", err)
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deps := evmtest.NewTestDeps() | |
| s.T().Log("Deploy ERC20MaliciousName") | |
| metadata := keeper.ERC20Metadata{ | |
| Name: "erc20name", | |
| Symbol: "TOKEN", | |
| Decimals: 18, | |
| } | |
| deployResp, err := evmtest.DeployContract( | |
| &deps, embeds.SmartContract_TestERC20MaliciousName, | |
| metadata.Name, metadata.Symbol, metadata.Decimals, | |
| ) | |
| s.Require().NoError(err) | |
| deps := evmtest.NewTestDeps() | |
| s.T().Log("Deploy ERC20MaliciousName") | |
| metadata := keeper.ERC20Metadata{ | |
| Name: "erc20name", | |
| Symbol: "TOKEN", | |
| Decimals: 18, | |
| } | |
| deployResp, err := evmtest.DeployContract( | |
| &deps, embeds.SmartContract_TestERC20MaliciousName, | |
| metadata.Name, metadata.Symbol, metadata.Decimals, | |
| ) | |
| if err != nil { | |
| s.T().Fatalf("Failed to deploy ERC20MaliciousName contract: %v", err) | |
| } |
x/evm/keeper/msg_server.go
Outdated
| @@ -130,7 +94,7 @@ func (k *Keeper) EthereumTx( | |||
| // reset the gas meter for current cosmos transaction | |||
| k.ResetGasMeterAndConsumeGas(ctx, totalGasUsed) | |||
|
|
|||
| err = k.EmitEthereumTxEvents(ctx, tx, msg, evmResp, contractAddr) | |||
| err = k.EmitEthereumTxEvents(ctx, tx.To(), tx.Type(), msg, evmResp) | |||
There was a problem hiding this comment.
Update function call to match new EmitEthereumTxEvents signature
The EmitEthereumTxEvents function now expects recipient *gethcommon.Address and txType uint8 as parameters. Ensure that tx.To() returns a *gethcommon.Address and that tx.Type() provides the correct transaction type according to the updated function signature.
Apply this diff if necessary to align the function call with the new signature:
- err = k.EmitEthereumTxEvents(ctx, tx.To(), tx.Type(), msg, evmResp)
+ recipient := tx.To()
+ txType := tx.Type()
+ err = k.EmitEthereumTxEvents(ctx, recipient, txType, msg, evmResp)Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Test cases look great for the behavior you're trying to test 🎉
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
x/evm/keeper/msg_server.go (4)
Line range hint
34-39: Ensure deferred error wrapping handles nil errors safelyIn the deferred function starting at line 35, wrapping
errwithout checking if it's nil could overwrite a nil error with a non-nil one.Consider modifying the deferred function to only wrap
errif it's not nil:defer func() { + if err != nil { err = fmt.Errorf("EthereumTx error: %w", err) + } }()
45-45: Avoid variable shadowing ofctxAt line 45,
ctxis reassigned when callingsdk.UnwrapSDKContext(goCtx).CacheContext(), which can cause confusion due to variable shadowing.Consider renaming one of the variables or restructuring the code to prevent shadowing and improve code clarity.
71-76: Simplify gas refund calculationThe refund gas calculation can be simplified to improve readability.
Consider the following refactor:
refundGas := evmMsg.Gas() - evmResp.GasUsed if evmResp.GasUsed > evmMsg.Gas() { refundGas = 0 }
709-721: Add missing function documentation forupdateBlockBloomThe
updateBlockBloomfunction introduced at lines 709-721 lacks documentation.Consider adding a comment to describe the purpose and usage of this function for better code maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
eth/indexer.pb.gois excluded by!**/*.pb.gox/evm/evm.pb.gois excluded by!**/*.pb.gox/evm/query.pb.gois excluded by!**/*.pb.gox/evm/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
- proto/eth/evm/v1/evm.proto (2 hunks)
- proto/eth/evm/v1/query.proto (1 hunks)
- proto/eth/evm/v1/tx.proto (4 hunks)
- proto/eth/types/v1/indexer.proto (1 hunks)
- x/evm/keeper/erc20.go (3 hunks)
- x/evm/keeper/msg_server.go (7 hunks)
✅ Files skipped from review due to trivial changes (4)
- proto/eth/evm/v1/evm.proto
- proto/eth/evm/v1/query.proto
- proto/eth/evm/v1/tx.proto
- proto/eth/types/v1/indexer.proto
🚧 Files skipped from review as they are similar to previous changes (1)
- x/evm/keeper/erc20.go
🔇 Additional comments (8)
x/evm/keeper/msg_server.go (8)
63-63: Verify correctness of ApplyEvmMsg parametersThe call to
k.ApplyEvmMsg(ctx, evmMsg, nil, true, evmConfig, txConfig)passesnilas the tracer andtruefor the commit parameter. Confirm that passingnilas the tracer is acceptable, and that committing theStateDBat this point is intentional and doesn't cause unintended side effects.
82-82: Confirm correct update of block bloom filterEnsure that the
k.updateBlockBloom(ctx, evmResp, uint64(txConfig.LogIndex))call correctly updates the block bloom filter and thattxConfig.LogIndexis the appropriate starting point for the logs.
89-90: Ensure accurate gas meter resetResetting the gas meter with
k.ResetGasMeterAndConsumeGas(ctx, totalGasUsed)should accurately reflect the total gas used to maintain consistency.
92-92: Verify parameters for EmitEthereumTxEventsThe updated call to
EmitEthereumTxEventsincludestx.To()andtx.Type(). Ensure that these parameters align with the updated function signature that expectsrecipient *gethcommon.AddressandtxType uint8.
100-102: Commit context only on successful transaction executionThe addition of
commitCtx()within theif !evmResp.Failed()block ensures that the context is committed only when the transaction did not fail. This change helps prevent committing state changes from failed transactions.
638-639: Update function signature and ensure consistencyThe function
EmitEthereumTxEventshas been updated to acceptrecipient *gethcommon.AddressandtxType uint8as parameters. Verify that all calls to this function have been updated accordingly and that the parameters are used consistently within the function.
652-653: Safely handle nil recipient when emitting eventsIn lines 652-653, the code checks if
recipient != nilbefore settingeventEthereumTx.Recipient. This prevents potential nil pointer dereferences.
Line range hint
686-702: Accurate event emission based on transaction detailsThe conditional logic from lines 686 to 702 ensures that the correct events are emitted based on the transaction type (contract creation, contract execution, or value transfer).
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- x/evm/keeper/erc20.go (3 hunks)
- x/evm/keeper/msg_server.go (7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/evm/keeper/erc20.go
🔇 Additional comments (12)
x/evm/keeper/msg_server.go (12)
34-35: Clarify the usage ofdeferfor enhanced error contextThe added comment effectively explains the purpose of the
deferstatement, which improves code readability and maintainability.
45-46: Proper use ofCacheContextfor transactional state managementIntroducing
CacheContextallows for conditional state commitment, enhancing the reliability of state changes within the transaction lifecycle.
64-67: Verify the necessity of passingnilas the tracerPassing
nilas the tracer toApplyEvmMsgrelies on default tracer behavior. Ensure that this meets the tracing requirements for debugging and performance monitoring.If custom tracing is needed, consider initializing a tracer before calling
ApplyEvmMsg.
73-74: State changes are committed only upon successful executionCalling
commitCtx()only when the transaction has not failed ensures that state changes are persisted only when intended, preventing unintended state mutations on failure.
77-81: Accurate calculation of gas refund to match Ethereum behaviorThe logic correctly refunds the unused gas, aligning with Ethereum's gas consumption model and ensuring fair gas accounting.
88-88: Block bloom filter updated to include transaction logsCalling
k.updateBlockBloomensures that the block's bloom filter accurately reflects the logs from the transaction, which is crucial for log searches and event indexing.
95-95: Resetting gas meter to reflect total gas usedUsing
k.ResetGasMeterAndConsumeGas(ctx, totalGasUsed)maintains consistency with the actual gas consumption, ensuring accurate gas accounting in the context.
640-641: Updated function signature improves event emission clarityThe modified parameters in
EmitEthereumTxEventsfocus on directly passing the recipient address and transaction type, simplifying the function's usage and enhancing code readability.
654-655: Safely set recipient in Ethereum transaction eventsThe null check for
recipientbefore settingeventEthereumTx.Recipientprevents potential nil pointer dereferences, ensuring robustness in the event emission process.
682-682: Correct formatting of transaction type in event attributeUsing
fmt.Sprintfto formattxTypeas a string ensures that the event attribute accurately represents the transaction type, which is essential for event consumers.
688-689: Accurate detection of contract creation transactionsBy checking
recipient == nil, the code correctly identifies contract creation scenarios and generates the contract address usingcrypto.CreateAddress, aligning with Ethereum standards.
710-723: RefactoredupdateBlockBloomimproves code modularityEncapsulating the block bloom filter update logic into
updateBlockBloomenhances code maintainability and readability, promoting better separation of concerns.
oops oops 2
Unique-Divine
force-pushed
the
fix/erc20-gas
branch
from
9a6d35d to
b99a20e
Compare
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
Notes
Removed
computeCommitGasLimit()call withinerc20/CallContractWithInput()as that calculation is executed "on chain". Instead, using the default gas cap of 25M. Fixes https://github.com/code-423n4/2024-10-nibiru-zenith/issues/46Fixed bug within
erc20/CallContractWithInput():errhere is nil and the result oferrors.Wrapfreturned nil causing panicnil pointer dereferencein the next methods which assume that iferris nil thenevmRespis not nil.EthereumTxinmsg_server.goTests
Added 2 malicious erc20 smart contracts:
TestERC20MaliciousName.solandTestERC20MaliciousTransfer.solto test behaviour of funtokens created with gas intensive methodsname()andtransfer()Resolves
Summary by CodeRabbit
Summary by CodeRabbit
New Features
TestERC20MaliciousNameandTestERC20MaliciousTransfer) with enhanced functionalities.Improvements
Bug Fixes
Tests