feat(evm): prevent funtoken from erc20 contract to be created with non-complete contracts #2150
Conversation
📝 WalkthroughWalkthroughThis pull request introduces several enhancements to EVM-related contracts and testing infrastructure. The primary changes involve adding a Changes
Sequence DiagramsequenceDiagram
participant Contract as ERC20 Contract
participant Keeper as EVM Keeper
participant Validator as Contract Validator
Keeper->>Validator: Check contract methods
Validator-->>Keeper: Validate method existence
Keeper->>Contract: Attempt method calls
Contract-->>Keeper: Confirm method availability
Keeper->>Contract: Create FunToken
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (3)
✨ Finishing Touches
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 (
|
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
x/evm/keeper/validate_contract.go:101
- The function name 'CheckAllethods' contains a typo. It should be 'CheckAllMethods'.
func (k Keeper) CheckAllethods(
x/evm/keeper/validate_contract.go:112
- [nitpick] The error message could be more descriptive. Suggestion: "The method '%q' was not found in the contract at address %s."
return fmt.Errorf("Method %q not found in contract at %s", name, contractAddr)
matthiasmatt
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (13)
evm-e2e/contracts/TestERC20.sol (1)
8-9: Consider adding documentation for initialSupply calculationThe constant defines a large number that might not be immediately clear to other developers. Consider adding a comment explaining the decimal places adjustment.
- uint256 constant initialSupply = 1000000 * (10 ** 18); + // Initial supply of 1,000,000 tokens with 18 decimal places (standard for ERC20) + uint256 constant initialSupply = 1000000 * (10 ** 18);x/evm/embeds/contracts/TestERC20.sol (1)
5-5: Maintain consistency between test contractsThis contract has different formatting (quotes, indentation) compared to its counterpart in
evm-e2e/contracts/TestERC20.sol. Consider standardizing the formatting across all test contracts to improve maintainability.Key differences to standardize:
- Quote style (single vs double)
- Indentation (2 spaces vs 4 spaces)
- Spacing around operators
Also applies to: 9-9, 16-18
x/evm/embeds/contracts/TestERC20TransferWithFee.sol (1)
Line range hint
13-27: Consider adding events for fee transfersThe transfer function deducts fees but doesn't emit events specifically for fee collection. Consider adding custom events to track fee transfers separately from regular transfers.
+ event FeeCollected(address indexed from, uint256 amount); function transfer( address to, uint256 amount ) public virtual override returns (bool) { address owner = _msgSender(); require(amount > 0, "Transfer amount must be greater than zero"); uint256 fee = (amount * FEE_PERCENTAGE) / 100; uint256 recipientAmount = amount - fee; _transfer(owner, address(this), fee); + emit FeeCollected(owner, fee); _transfer(owner, to, recipientAmount); return true; }x/evm/embeds/contracts/TestERC20MinterMissingFunction.sol (1)
20-49: Add input validation in the constructor.Consider validating the
decimals_parameter to ensure it's within a reasonable range (typically 0-18 for ERC20 tokens).constructor( string memory name, string memory symbol, uint8 decimals_ ) ERC20(name, symbol) { + require(decimals_ <= 18, "ERC20: decimals must be <= 18"); _setupDecimals(decimals_); }x/evm/keeper/validate_contract.go (2)
78-79: Review gas cap configuration.The hardcoded gas cap of 690000000000000000 seems arbitrarily large. Consider:
- Making it configurable through keeper parameters
- Using a more reasonable default value
- GasCap: 690000000000000000, + GasCap: k.GetParams(ctx).MaxGasForMethodCheck,
96-112: Enhance error reporting for missing methods.Instead of returning on the first missing method, consider collecting all missing methods to provide more comprehensive feedback.
func (k Keeper) CheckAllMethods( ctx context.Context, contractAddr common.Address, abiMethods []abi.Method, ) error { + var missingMethods []string for name, method := range abiMethods { hasMethod, err := k.HasMethodInContract(ctx, contractAddr, method) if err != nil { return err } if !hasMethod { - return fmt.Errorf("Method %q not found in contract at %s", name, contractAddr) + missingMethods = append(missingMethods, name) } } + if len(missingMethods) > 0 { + return fmt.Errorf("Methods %v not found in contract at %s", missingMethods, contractAddr) + } return nil }x/evm/keeper/validate_contract_test.go (2)
14-52: Enhance test coverage with additional scenarios.Consider adding test cases for:
- Methods with different parameter types
- Methods that revert with custom error messages
- Edge cases like zero-length input
+ // Test method with multiple parameters + methodTransfer, ok := erc20Abi.Methods["transfer"] + require.True(t, ok) + hasMethod, err = k.HasMethodInContract(ctx, deployResp.ContractAddr, methodTransfer) + require.NoError(t, err) + require.True(t, hasMethod) + + // Test method with zero-length input + methodTotalSupply, ok := erc20Abi.Methods["totalSupply"] + require.True(t, ok) + hasMethod, err = k.HasMethodInContract(ctx, deployResp.ContractAddr, methodTotalSupply) + require.NoError(t, err) + require.True(t, hasMethod)
54-104: Add negative test cases for better coverage.The test suite would benefit from additional negative scenarios:
- Contract address with no code
- Invalid contract address
- Methods with matching names but different parameter types
+ // Test with contract address containing no code + noCodeAddr := gethcommon.HexToAddress("0x1234567890123456789012345678901234567890") + err = k.CheckAllMethods(ctx, noCodeAddr, allMethods) + require.Error(t, err) + require.Contains(t, err.Error(), "no code at address") + + // Test with invalid contract address + invalidAddr := gethcommon.Address{} + err = k.CheckAllMethods(ctx, invalidAddr, allMethods) + require.Error(t, err)x/evm/keeper/funtoken_from_erc20.go (1)
126-130: Enhance error handling with wrapped errors.Wrap the error from
checkErc20ImplementsAllRequiredto provide more context about the failure.if err := k.checkErc20ImplementsAllRequired(ctx, erc20, abi); err != nil { - return funtoken, err + return funtoken, fmt.Errorf("ERC20 contract at %s does not implement required interface: %w", erc20.String(), err) }x/evm/embeds/embeds.go (1)
95-97: Fix incorrect documentation comment.The comment suggests this contract tests malicious transfer behavior, but the contract name and purpose indicate it's for testing missing function scenarios.
Apply this diff to correct the documentation:
- // which simulates malicious ERC20 behavior by adding gas intensive operation - // for function transfer() intended to attack funtoken conversion from erc20 to bank coin + // which simulates incomplete ERC20 implementation by omitting required functions + // intended to test funtoken creation validationx/evm/keeper/funtoken_from_erc20_test.go (2)
429-430: Fix incorrect test documentation.The comment is incorrectly copied from TestFunTokenInfiniteRecursionERC20.
Apply this diff to correct the documentation:
-// TestFunTokenInfiniteRecursionERC20 creates a funtoken from a contract -// with a malicious recursive balanceOf() and transfer() functions. +// TestFunTokenMissingMintFunction tests funtoken creation validation +// when attempting to create from an ERC20 contract missing the mint function.
434-463: Enhance test coverage with additional assertions.The test could be improved by adding assertions to verify the contract state before attempting to create the FunToken.
Consider adding these assertions before line 458:
s.T().Log("Deploy InfiniteRecursionERC20") metadata := keeper.ERC20Metadata{ Name: "erc20name", Symbol: "TOKEN", Decimals: 18, } deployResp, err := evmtest.DeployContract( &deps, embeds.SmartContract_TestERC20MissingFunction, metadata.Name, metadata.Symbol, metadata.Decimals, ) s.Require().NoError(err) + + // Verify contract deployment and metadata + info, err := deps.EvmKeeper.FindERC20Metadata(deps.Ctx, deployResp.ContractAddr) + s.Require().NoError(err) + s.Require().Equal(metadata, *info)x/evm/embeds/artifacts/contracts/TestERC20MinterMissingFunction.sol/ERC20Minter.json (1)
1-4: Consider renaming the contract to better reflect its functionality.The contract name "ERC20Minter" suggests minting capability, but since this contract intentionally omits the mint function (as indicated by the source file name), consider a more accurate name like "ERC20Base" or "ERC20NonMintable" to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
evm-e2e/contracts/TestERC20.sol(1 hunks)x/evm/embeds/abi/ERC20Minter.json(0 hunks)x/evm/embeds/artifacts/contracts/TestERC20.sol/TestERC20.json(2 hunks)x/evm/embeds/artifacts/contracts/TestERC20MaliciousTransfer.sol/TestERC20MaliciousTransfer.json(2 hunks)x/evm/embeds/artifacts/contracts/TestERC20MinterMissingFunction.sol/ERC20Minter.json(1 hunks)x/evm/embeds/artifacts/contracts/TestERC20TransferWithFee.sol/TestERC20TransferWithFee.json(2 hunks)x/evm/embeds/contracts/TestERC20.sol(1 hunks)x/evm/embeds/contracts/TestERC20MaliciousTransfer.sol(2 hunks)x/evm/embeds/contracts/TestERC20MinterMissingFunction.sol(1 hunks)x/evm/embeds/contracts/TestERC20TransferWithFee.sol(2 hunks)x/evm/embeds/embeds.go(3 hunks)x/evm/embeds/embeds_test.go(1 hunks)x/evm/keeper/funtoken_from_erc20.go(6 hunks)x/evm/keeper/funtoken_from_erc20_test.go(1 hunks)x/evm/keeper/msg_server.go(1 hunks)x/evm/keeper/validate_contract.go(1 hunks)x/evm/keeper/validate_contract_test.go(1 hunks)x/evm/precompile/funtoken_test.go(0 hunks)
💤 Files with no reviewable changes (2)
- x/evm/embeds/abi/ERC20Minter.json
- x/evm/precompile/funtoken_test.go
🧰 Additional context used
📓 Learnings (1)
x/evm/keeper/funtoken_from_erc20_test.go (1)
Learnt from: k-yang
PR: NibiruChain/nibiru#2129
File: x/evm/embeds/contracts/TestInfiniteRecursionERC20.sol:24-32
Timestamp: 2024-12-26T04:43:43.966Z
Learning: The `TestInfiniteRecursionERC20` contract is intentionally designed to test infinite recursion scenarios in ERC20-based contracts for the `FunToken` architecture.
🔇 Additional comments (10)
x/evm/embeds/embeds_test.go (1)
19-19: LGTM!The addition of
SmartContract_TestERC20MissingFunction.MustLoad()follows the existing pattern and maintains test coverage for the new contract.x/evm/embeds/contracts/TestERC20MinterMissingFunction.sol (1)
1-19: Well-structured contract with appropriate imports!The contract follows best practices by:
- Using the MIT license
- Importing standard OpenZeppelin contracts
- Including clear documentation
x/evm/keeper/msg_server.go (1)
301-301: LGTM! Good improvement to code clarity.The addition of the
contractCreationvariable makes the code more readable and maintainable by explicitly naming the condition.x/evm/embeds/contracts/TestERC20MaliciousTransfer.sol (1)
7-12: LGTM! Improved code formatting.The formatting changes enhance code readability by properly structuring function parameters.
Also applies to: 15-18
x/evm/embeds/artifacts/contracts/TestERC20.sol/TestERC20.json (1)
189-206: LGTM! The mint function follows ERC20 standards.The implementation includes proper input validation and follows the standard ERC20 minting pattern.
x/evm/embeds/artifacts/contracts/TestERC20MaliciousTransfer.sol/TestERC20MaliciousTransfer.json (1)
205-222: Review security implications of minting in malicious contract.Adding minting capability to a contract named "malicious" requires careful consideration. Ensure this is intended for testing purposes only and won't be deployed to production.
Run the following to check for potential security issues:
✅ Verification successful
Contract is verified as test-only implementation
This contract is exclusively used for testing purposes:
- Located in test contract directory
- Only referenced in test files
- Named with "Test" prefix
- Part of a test suite with other test contracts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test-related imports or comments rg -l "test|Test|testing" --type sol # Check if the contract is only used in test files rg -l "TestERC20MaliciousTransfer" --type solLength of output: 140
Script:
#!/bin/bash # Search for the contract name in all files rg -l "TestERC20MaliciousTransfer" # Search for files containing test-related terms rg -l "test|Test|testing" # Look for deployment configurations fd -e js -e json -e yaml -e yml deploy # Check if the contract is in a test directory fd -p "**/test*/**" -p "**/tests/**" "TestERC20MaliciousTransfer"Length of output: 12127
x/evm/embeds/artifacts/contracts/TestERC20TransferWithFee.sol/TestERC20TransferWithFee.json (1)
200-217: LGTM! Mint function implementation is consistent.The mint function implementation aligns with the other contracts and follows ERC20 standards.
x/evm/embeds/artifacts/contracts/TestERC20MinterMissingFunction.sol/ERC20Minter.json (3)
194-211: Review the non-standardburnFromAuthorityfunction.The contract includes a non-standard
burnFromAuthorityfunction alongside the standard ERC20 burn functions. This could lead to confusion or unexpected behavior. Consider:
- Documenting the specific use case for this function
- Adding access control if it's meant to be restricted
5-398: LGTM! Standard ERC20 implementation.The contract correctly implements all standard ERC20 functions and events, with proper access control through the Ownable pattern. The intentional omission of the mint function aligns with the PR objectives to prevent incomplete contracts.
399-403: LGTM! Contract compiled with secure Solidity version.The contract is compiled with Solidity 0.8.24, which includes important safety features like checked arithmetic and explicit overflow checks. No external library dependencies are required.
| function mint(address to, uint256 amount) public { | ||
| _mint(to, amount); | ||
| } |
There was a problem hiding this comment.
Add access control to mint function
The mint function is public without any access restrictions, which could allow anyone to create unlimited tokens. Consider adding the OpenZeppelin Ownable contract and restricting minting to the contract owner.
+import {Ownable} from '@openzeppelin/contracts/access/Ownable.sol';
-contract TestERC20 is ERC20 {
+contract TestERC20 is ERC20, Ownable {
// ... other code ...
- function mint(address to, uint256 amount) public {
+ function mint(address to, uint256 amount) public onlyOwner {
_mint(to, amount);
}Committable suggestion skipped: line range outside the PR's diff.
| function mint(address to, uint256 amount) public virtual { | ||
| _mint(to, amount); | ||
| } |
There was a problem hiding this comment.
Address security and consistency issues in mint function
- The
mintfunction lacks access control, similar to other contracts - The
virtualkeyword usage is inconsistent with other contracts
+import {Ownable} from '@openzeppelin/contracts/access/Ownable.sol';
-contract TestERC20TransferWithFee is ERC20 {
+contract TestERC20TransferWithFee is ERC20, Ownable {
// ... other code ...
- function mint(address to, uint256 amount) public virtual {
+ function mint(address to, uint256 amount) public onlyOwner {
_mint(to, amount);
}Committable suggestion skipped: line range outside the PR's diff.
| /** | ||
| * @dev Destroys `amount` new tokens for `to`. Suitable when the contract owner | ||
| * should have authority to burn tokens from an account directly, such as in | ||
| * the case of regulatory compliance, or actions selected via | ||
| * decentralized governance. | ||
| * | ||
| * See {ERC20-_burn}. | ||
| * | ||
| */ | ||
| function burnFromAuthority( | ||
| address from, | ||
| uint256 amount | ||
| ) public virtual onlyOwner { | ||
| _burn(from, amount); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Verified: burnFromAuthority needs additional security measures
The review comment's concerns are valid. The codebase search confirms:
- No events exist for tracking authority burns
- No time-lock mechanism is implemented
- Documentation is limited to in-code comments
The suggested improvements would enhance transparency and security while maintaining the regulatory compliance purpose.
🔗 Analysis chain
Review security implications of burnFromAuthority.
The burnFromAuthority function gives significant power to the contract owner, allowing them to burn tokens from any address without consent. While this might be necessary for regulatory compliance, users should be aware of this capability.
Consider:
- Adding events to log such burns for transparency
- Documenting this power in user-facing documentation
- Implementing a time-lock or governance mechanism
function burnFromAuthority(
address from,
uint256 amount
) public virtual onlyOwner {
+ emit AuthorityBurn(from, amount);
_burn(from, amount);
}
+event AuthorityBurn(address indexed from, uint256 amount);🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for existing burn-related events
echo "=== Searching for burn events ==="
rg -l "event.*Burn" --type sol
# Search for documentation about burning authority
echo -e "\n=== Searching for burn authority documentation ==="
rg -i "burn.*authority|authority.*burn" --type sol
# Look for time-lock or governance mechanisms
echo -e "\n=== Searching for timelock or governance patterns ==="
rg -l "timelock|governance" --type sol
Length of output: 527
Script:
#!/bin/bash
# Search for the contract and its context
echo "=== Searching for contract definition ==="
rg -A 5 "contract TestERC20MinterMissingFunction"
# Search for existing burn-related events
echo -e "\n=== Searching for burn events ==="
rg "event.*Burn"
# Search for documentation about burning authority
echo -e "\n=== Searching for burn authority documentation ==="
rg -i "burn.*authority|authority.*burn"
# Look for time-lock or governance mechanisms
echo -e "\n=== Searching for timelock or governance patterns ==="
rg "timelock|governance"
Length of output: 3745
| if strings.Contains(err.Error(), "Estimate gas VMError: execution reverted, but unable to parse reason") { | ||
| return false, nil | ||
| } | ||
| return true, nil |
There was a problem hiding this comment.
Improve error handling for method detection.
The current implementation assumes any error other than the specific revert message indicates the method exists. This could lead to false positives.
-if strings.Contains(err.Error(), "Estimate gas VMError: execution reverted, but unable to parse reason") {
+if strings.Contains(err.Error(), "execution reverted") {
return false, nil
}
-return true, nil
+return false, fmt.Errorf("unexpected error checking method: %w", 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.
| if strings.Contains(err.Error(), "Estimate gas VMError: execution reverted, but unable to parse reason") { | |
| return false, nil | |
| } | |
| return true, nil | |
| if strings.Contains(err.Error(), "execution reverted") { | |
| return false, nil | |
| } | |
| return false, fmt.Errorf("unexpected error checking method: %w", err) |
x/evm/keeper/funtoken_from_erc20.go
Outdated
| func (k Keeper) checkErc20ImplementsAllRequired( | ||
| ctx sdk.Context, erc20Addr gethcommon.Address, abi *gethabi.ABI, | ||
| ) error { | ||
| methodNames := []string{"name", | ||
| "symbol", | ||
| "decimals", | ||
| "totalSupply", | ||
| "balanceOf", | ||
| "transfer", | ||
| "allowance", | ||
| "approve", | ||
| "transferFrom", | ||
| "increaseAllowance", | ||
| "decreaseAllowance", | ||
| "mint", | ||
| } | ||
|
|
||
| for _, methodName := range methodNames { | ||
| method, ok := abi.Methods[methodName] | ||
| if !ok { | ||
| return fmt.Errorf("method '%s' not found in contract at %s", methodName, erc20Addr.Hex()) | ||
| } | ||
| hasMethod, err := k.HasMethodInContract(ctx, erc20Addr, method) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if !hasMethod { | ||
| return fmt.Errorf("method '%s' not found in contract at %s", methodName, erc20Addr.Hex()) | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve method list management and validation.
Consider the following improvements:
- Move method list to a constant or configuration
- Categorize methods as required vs optional
- Add version-specific method checks
+// Required ERC20 methods as per EIP-20
+var requiredERC20Methods = []string{
+ "totalSupply",
+ "balanceOf",
+ "transfer",
+ "transferFrom",
+ "approve",
+ "allowance",
+}
+
+// Optional but commonly implemented ERC20 methods
+var optionalERC20Methods = []string{
+ "name",
+ "symbol",
+ "decimals",
+ "increaseAllowance",
+ "decreaseAllowance",
+}
+
+// Additional required methods for FunToken compatibility
+var funTokenRequiredMethods = []string{
+ "mint",
+}
+
func (k Keeper) checkErc20ImplementsAllRequired(
ctx sdk.Context, erc20Addr gethcommon.Address, abi *gethabi.ABI,
) error {
- methodNames := []string{"name", ...}
+ // Check required ERC20 methods
+ for _, methodName := range requiredERC20Methods {
+ if err := k.checkMethod(ctx, erc20Addr, abi, methodName, true); err != nil {
+ return err
+ }
+ }
+
+ // Check FunToken required methods
+ for _, methodName := range funTokenRequiredMethods {
+ if err := k.checkMethod(ctx, erc20Addr, abi, methodName, true); err != nil {
+ return err
+ }
+ }
+
+ // Check optional methods
+ for _, methodName := range optionalERC20Methods {
+ if err := k.checkMethod(ctx, erc20Addr, abi, methodName, false); err != nil {
+ return err
+ }
+ }
+ return nil
+}
+
+func (k Keeper) checkMethod(
+ ctx sdk.Context,
+ erc20Addr gethcommon.Address,
+ abi *gethabi.ABI,
+ methodName string,
+ required bool,
+) error {
+ method, ok := abi.Methods[methodName]
+ if !ok {
+ if required {
+ return fmt.Errorf("required method '%s' not found in ABI", methodName)
+ }
+ return nil
+ }
+ hasMethod, err := k.HasMethodInContract(ctx, erc20Addr, method)
+ if err != nil {
+ return err
+ }
+ if !hasMethod && required {
+ return fmt.Errorf("required method '%s' not found in contract", methodName)
+ }
+ return nil
}📝 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.
| func (k Keeper) checkErc20ImplementsAllRequired( | |
| ctx sdk.Context, erc20Addr gethcommon.Address, abi *gethabi.ABI, | |
| ) error { | |
| methodNames := []string{"name", | |
| "symbol", | |
| "decimals", | |
| "totalSupply", | |
| "balanceOf", | |
| "transfer", | |
| "allowance", | |
| "approve", | |
| "transferFrom", | |
| "increaseAllowance", | |
| "decreaseAllowance", | |
| "mint", | |
| } | |
| for _, methodName := range methodNames { | |
| method, ok := abi.Methods[methodName] | |
| if !ok { | |
| return fmt.Errorf("method '%s' not found in contract at %s", methodName, erc20Addr.Hex()) | |
| } | |
| hasMethod, err := k.HasMethodInContract(ctx, erc20Addr, method) | |
| if err != nil { | |
| return err | |
| } | |
| if !hasMethod { | |
| return fmt.Errorf("method '%s' not found in contract at %s", methodName, erc20Addr.Hex()) | |
| } | |
| } | |
| return nil | |
| } | |
| // Required ERC20 methods as per EIP-20 | |
| var requiredERC20Methods = []string{ | |
| "totalSupply", | |
| "balanceOf", | |
| "transfer", | |
| "transferFrom", | |
| "approve", | |
| "allowance", | |
| } | |
| // Optional but commonly implemented ERC20 methods | |
| var optionalERC20Methods = []string{ | |
| "name", | |
| "symbol", | |
| "decimals", | |
| "increaseAllowance", | |
| "decreaseAllowance", | |
| } | |
| // Additional required methods for FunToken compatibility | |
| var funTokenRequiredMethods = []string{ | |
| "mint", | |
| } | |
| func (k Keeper) checkErc20ImplementsAllRequired( | |
| ctx sdk.Context, erc20Addr gethcommon.Address, abi *gethabi.ABI, | |
| ) error { | |
| // Check required ERC20 methods | |
| for _, methodName := range requiredERC20Methods { | |
| if err := k.checkMethod(ctx, erc20Addr, abi, methodName, true); err != nil { | |
| return err | |
| } | |
| } | |
| // Check FunToken required methods | |
| for _, methodName := range funTokenRequiredMethods { | |
| if err := k.checkMethod(ctx, erc20Addr, abi, methodName, true); err != nil { | |
| return err | |
| } | |
| } | |
| // Check optional methods | |
| for _, methodName := range optionalERC20Methods { | |
| if err := k.checkMethod(ctx, erc20Addr, abi, methodName, false); err != nil { | |
| return err | |
| } | |
| } | |
| return nil | |
| } | |
| func (k Keeper) checkMethod( | |
| ctx sdk.Context, | |
| erc20Addr gethcommon.Address, | |
| abi *gethabi.ABI, | |
| methodName string, | |
| required bool, | |
| ) error { | |
| method, ok := abi.Methods[methodName] | |
| if !ok { | |
| if required { | |
| return fmt.Errorf("required method '%s' not found in ABI", methodName) | |
| } | |
| return nil | |
| } | |
| hasMethod, err := k.HasMethodInContract(ctx, erc20Addr, method) | |
| if err != nil { | |
| return err | |
| } | |
| if !hasMethod && required { | |
| return fmt.Errorf("required method '%s' not found in contract", methodName) | |
| } | |
| return nil | |
| } |
| _mint(to, amount); | ||
| } |
There was a problem hiding this comment.
Add access control to the mint function.
The mint function lacks access control, allowing anyone to mint tokens. This could be a security risk if not intended for testing purposes.
Apply this diff to add access control:
- function mint(address to, uint256 amount) public virtual {
+ function mint(address to, uint256 amount) public virtual {
+ require(msg.sender == owner(), "TestERC20MaliciousTransfer: must be owner");
_mint(to, amount);
}Additionally, consider:
- Importing and using OpenZeppelin's
Ownablecontract - Removing the
virtualmodifier if inheritance is not intended
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@onikonychev Thanks for confirming! Let me know if you need any help implementing these changes.
🐰 happy to help
✏️ Learnings added
Learnt from: onikonychev
PR: NibiruChain/nibiru#2150
File: x/evm/embeds/contracts/TestERC20MaliciousTransfer.sol:36-38
Timestamp: 2025-01-31T07:16:53.976Z
Learning: Mint functions in ERC20 tokens should have access control (e.g., using OpenZeppelin's Ownable) to prevent unauthorized minting. The function should check for owner privileges using `require(msg.sender == owner(), "must be owner")`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
x/evm/keeper/funtoken_from_erc20.go (1)
160-165: 🛠️ Refactor suggestionConsider using a more robust method verification approach.
The current implementation could be improved by using a more structured approach to method verification.
Apply the refactoring suggestions from the previous review to:
- Move method lists to constants
- Categorize methods as required vs optional
- Add version-specific method checks
🧹 Nitpick comments (8)
x/evm/embeds/abi/DSNote.json (2)
7-8: Consider using more descriptive parameter names.The parameter names
guy,foo, andbarare not descriptive. Consider renaming them to better reflect their purpose:
guycould beaccount,user, oraddressfooandbarshould reflect their actual use case- "name": "guy", + "name": "account",Also applies to: 12-13, 17-18, 22-23
36-36: Consider a more descriptive event name.The name
LogNoteis generic. Consider prefixing it with a verb that describes the action being logged (e.g.,ActionLogged,OperationRecorded).eth/rpc/backend/gas_used_test.go (1)
106-106: Consider optimizing the gas limit.The gas limit of 3,500,000 seems unnecessarily high since successful transactions in this test consistently use less than 500,000 gas. Consider reducing it to be closer to the actual gas usage to better detect potential gas optimization issues.
- Gas: 3_500_000, + Gas: 1_000_000,eth/rpc/backend/tx_logs_test.go (1)
106-106: Consider implementing gas limit constants.To improve maintainability and consistency, consider introducing named constants for gas limits based on operation types. This would:
- Standardize gas limits across similar operations
- Make it easier to adjust limits globally
- Document the reasoning behind different gas limits
- Help detect unexpected gas usage patterns
Example implementation:
const ( // GasLimitERC20Transfer represents the gas limit for ERC20 token transfers GasLimitERC20Transfer = 1_000_000 // GasLimitFunTokenOps represents the gas limit for FunToken operations GasLimitFunTokenOps = 1_500_000 )Also applies to: 109-109
x/evm/embeds/abi/DSAuthority.json (1)
1-28: Consider adding documentation about expected behavior.To improve maintainability, consider adding documentation that describes:
- The expected return value for authorized/unauthorized calls
- Any special cases or restrictions
- Examples of typical usage patterns
Example documentation to add at the contract level:
/// @title DSAuthority /// @notice Authority contract for managing access control between contracts /// @dev Implements granular function-level access control /// /// @notice canCall - Checks if a source address can call a specific function on a destination address /// @param src The address attempting to make the call /// @param dst The target contract address /// @param sig The function signature being called (first 4 bytes of keccak256 of the function signature) /// @return bool Returns true if the call is authorized, false otherwisex/evm/embeds/abi/DSToken.json (1)
293-332: Redundant transfer helper functions.The contract includes
push,pull, andmovefunctions that appear to duplicate the functionality of standard ERC20 transfer methods:
push≈transferpull≈transferFrommove≈transferFromThis increases the attack surface and could lead to confusion.
Consider removing these helper functions to reduce complexity and potential attack vectors, or document their specific use cases if they must be retained.
x/evm/keeper/funtoken_from_erc20_test.go (2)
450-452: Fix incorrect function comment.The comment is copied from
TestFunTokenInfiniteRecursionERC20and doesn't match the actual test function.Apply this diff to fix the comment:
-// TestFunTokenInfiniteRecursionERC20 creates a funtoken from a contract -// with a malicious recursive balanceOf() and transfer() functions. +// TestFunTokenMissingMintFunction tests that creating a funtoken fails +// when the contract doesn't implement the required mint function.
455-455: Fix inconsistent test log messages.The test log messages don't match the actual test scenario.
Apply this diff to fix the messages:
- s.T().Log("Deploy InfiniteRecursionERC20") + s.T().Log("Deploy ERC20 without mint function") - s.T().Log("happy: CreateFunToken for ERC20 with infinite recursion") + s.T().Log("sad: CreateFunToken for ERC20 without mint function")Also applies to: 471-471
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
eth/rpc/backend/gas_used_test.go(1 hunks)eth/rpc/backend/tx_logs_test.go(1 hunks)x/evm/embeds/abi/DSAuth.json(1 hunks)x/evm/embeds/abi/DSAuthEvents.json(1 hunks)x/evm/embeds/abi/DSAuthority.json(1 hunks)x/evm/embeds/abi/DSMath.json(1 hunks)x/evm/embeds/abi/DSNote.json(1 hunks)x/evm/embeds/abi/DSStop.json(1 hunks)x/evm/embeds/abi/DSThing.json(1 hunks)x/evm/embeds/abi/DSToken.json(1 hunks)x/evm/embeds/abi/DSTokenBase.json(1 hunks)x/evm/embeds/abi/ERC20.json(1 hunks)x/evm/embeds/embeds.go(3 hunks)x/evm/embeds/embeds_test.go(1 hunks)x/evm/keeper/funtoken_from_erc20.go(5 hunks)x/evm/keeper/funtoken_from_erc20_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/evm/embeds/abi/DSMath.json
🚧 Files skipped from review as they are similar to previous changes (1)
- x/evm/embeds/embeds_test.go
🧰 Additional context used
📓 Learnings (1)
x/evm/keeper/funtoken_from_erc20_test.go (1)
Learnt from: k-yang
PR: NibiruChain/nibiru#2129
File: x/evm/embeds/contracts/TestInfiniteRecursionERC20.sol:24-32
Timestamp: 2024-12-26T04:43:43.966Z
Learning: The `TestInfiniteRecursionERC20` contract is intentionally designed to test infinite recursion scenarios in ERC20-based contracts for the `FunToken` architecture.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e-evm
- GitHub Check: integration-tests
🔇 Additional comments (24)
x/evm/embeds/abi/DSNote.json (2)
6-9: Verify indexed parameter usage.The event uses the maximum allowed number of indexed parameters (4). While this is valid, it's important to ensure that these parameters are the most relevant for filtering events.
Let's analyze the event filtering usage:
Also applies to: 11-14, 16-19, 21-24
3-3: Verify the necessity of anonymous event.The event is marked as anonymous, which means it won't include the event signature in the log topics. This can make it harder to filter logs. Consider removing the anonymous flag unless there's a specific reason for it.
Let's verify how this event is used in the codebase:
✅ Verification successful
The anonymous flag is correct and intentional.
The LogNote event is part of the DappSys framework's logging system, where the anonymous flag is deliberately used as a gas optimization. The event captures function signatures and parameters automatically through the
notemodifier, used throughout the contract system for tracking administrative actions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for LogNote event usage rg -l "LogNote" --type solLength of output: 54
Script:
#!/bin/bash # Search for LogNote across all files rg -l "LogNote" # List all files in the embeds directory fd . "x/evm/embeds" # Search for DSNote across all files rg -l "DSNote"Length of output: 5998
Script:
#!/bin/bash # Check the MKR contract implementation cat x/evm/embeds/contracts/MKR.sol # Check if there's any documentation cat x/evm/embeds/HACKING.md cat x/evm/embeds/README.mdLength of output: 19381
x/evm/embeds/embeds.go (3)
32-33: LGTM! Variable declaration follows established patterns.The embedded JSON variable declaration follows the project's naming conventions and file organization patterns.
190-190: LGTM! Initialization follows established patterns.The contract initialization is properly added using
MustLoad()and follows the same pattern as other test contracts.
32-33: Verify ERC20 interface completeness check.Let's verify that the embedded JSON properly defines the ERC20 interface for testing incomplete contracts.
x/evm/embeds/abi/DSAuthority.json (2)
1-28: LGTM! The authority check implementation follows security best practices.The
canCallfunction provides granular control over contract interactions by validating calls based on source address, destination address, and function signature. The implementation follows standard patterns for authority checks in Solidity.
1-28: Verify integration with ERC20 minting functionality.Since this authority contract is part of changes involving ERC20 minting capabilities, let's verify its integration:
✅ Verification successful
Standard authority contract implementation verified
The DSAuthority contract provides a standard permission-checking interface through its
canCallfunction, which is correctly integrated with the token contracts' authority system. This implementation is consistent with the changes involving ERC20 minting capabilities.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how the authority contract is used with ERC20 minting # Search for DSAuthority usage in contracts echo "Checking DSAuthority usage in contracts:" rg -A 5 "DSAuthority" --type sol # Search for canCall implementations related to minting echo -e "\nChecking canCall implementations related to minting:" rg -B 2 -A 5 "function canCall.*bytes4.*mint" # Search for mint function access controls echo -e "\nChecking mint function access controls:" ast-grep --pattern 'function mint($$$) { $$$ }'Length of output: 467
Script:
#!/bin/bash # Search in JSON files for related contracts and permissions echo "Checking JSON files for DSAuthority references:" rg -t json "DSAuthority" echo -e "\nChecking for mint function signatures in JSON files:" rg -t json '"name":\s*"mint"' -A 10 echo -e "\nChecking for authority/permission patterns:" rg -t json "canCall|authority|permission" -A 5Length of output: 18153
x/evm/embeds/abi/DSThing.json (3)
95-118: LGTM! Well-structured authority and ownership events.The
LogSetAuthorityandLogSetOwnerevents are well-defined with:
- Properly indexed address parameters
- Clear, descriptive names
- Appropriate non-anonymous setting
16-29: Clarify the relationship between owner and authority roles.The contract defines both owner and authority roles. Please document:
- The differences between these roles
- The hierarchy or relationship between them
- The specific permissions granted to each role
Also applies to: 44-57
✅ Verification successful
Owner has absolute authority, while Authority provides configurable permissions
The implementation shows a clear hierarchy:
- Owner is a superuser with unrestricted access to all functions
- Authority is a configurable role that can provide granular permissions through its
canCall()function- Both roles are complementary: owner for administrative control, authority for flexible permission management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for documentation or comments explaining the roles rg -i "authority|owner" --type mdLength of output: 672
Script:
#!/bin/bash # Find the contract implementation and related files fd -e sol -e js -e ts . | xargs rg -l "DSThing" # Search for implementation details of authority and owner roles ast-grep --pattern 'contract DSThing'Length of output: 172
Script:
#!/bin/bash # Examine the contract implementation cat x/evm/embeds/contracts/MKR.sol # Search for authority and owner usage patterns rg -A 5 -B 5 "authority|owner" x/evm/embeds/contracts/MKR.solLength of output: 17195
2-15: Verify access controls in the contract implementation.The ownership management functions are well-defined in the ABI, but ensure the contract implementation includes proper access controls for
setOwnerto prevent unauthorized ownership changes.Also applies to: 30-43
✅ Verification successful
Access controls properly implemented for setOwner function
The implementation in DSAuth contract includes comprehensive access controls:
- Only the current owner or authorized addresses can call setOwner
- Changes are tracked via LogSetOwner events
- Multiple authorization layers through DSAuthority contract integration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the contract implementation to verify access controls ast-grep --pattern 'function setOwner(address owner_) { $$$ }'Length of output: 66
Script:
#!/bin/bash # Strategy 1: Search for setOwner function implementations rg -l "function setOwner" # Strategy 2: Search for DSThing contract files fd -t f "DSThing" # Strategy 3: Search for owner-related patterns in potential Solidity files rg -l "owner" --type-add 'sol:*.sol' -tsolLength of output: 383
Script:
#!/bin/bash # Get the implementation details from MKR.sol cat x/evm/embeds/contracts/MKR.solLength of output: 15144
x/evm/embeds/abi/DSToken.json (4)
117-134:⚠️ Potential issueDuplicate mint functions with different signatures.
The contract has two
mintfunctions with different signatures:
mint(address guy, uint256 wad)mint(uint256 wad)This could lead to confusion and potential security risks if not properly access-controlled.
Consider:
- Removing one of the mint functions to avoid confusion
- Adding clear documentation about the difference between these functions
- Ensuring proper access control mechanisms are in place
Also applies to: 256-269
5-14: Non-standard return types for name and symbol functions.The
name()andsymbol()functions returnbytes32instead of the standardstringtype specified in EIP-20. This deviation from the ERC20 standard might cause compatibility issues with existing tools and interfaces.Consider updating the return types to
stringto ensure better compatibility with the ERC20 standard.Also applies to: 227-236
135-148:⚠️ Potential issueDuplicate burn functions with different signatures.
Similar to the mint functions, there are two
burnfunctions with different signatures:
burn(address guy, uint256 wad)burn(uint256 wad)This could lead to confusion and potential security risks if not properly access-controlled.
Consider the same recommendations as for the mint functions.
Also applies to: 238-255
182-209: Verify access control for administrative functions.The contract includes powerful administrative functions (
stop,start,setOwner,setAuthority) that could potentially be dangerous if not properly access-controlled.Ensure these functions:
- Have proper access control modifiers
- Emit events for all state changes
- Include checks to prevent unauthorized access
✅ Verification successful
Access controls properly implemented and verified
The administrative functions are well-protected through a robust access control system:
- All admin functions are guarded by the
authmodifier- Flexible authority system with owner and optional authority contract
- Events emitted for all administrative actions
- Emergency stop functionality properly controlled
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for access control modifiers on administrative functions ast-grep --pattern 'modifier $$$owner$$$' # Check for authority checks in the implementation ast-grep --pattern 'function $_($$) { require($$$authority$$$); $$$ }'Length of output: 120
Script:
#!/bin/bash # Find the Solidity source file fd -e sol -x grep -l "DSToken" {} # Look for any related contract files fd -e sol -x grep -l "setAuthority\|setOwner\|stop\|start" {}Length of output: 3799
Script:
#!/bin/bash # Check MKR.sol content cat ./x/evm/embeds/contracts/MKR.sol # Look for auth-related imports or base contracts rg -l "auth|DSAuth" ./x/evm/embeds/contracts/Length of output: 15327
x/evm/embeds/abi/DSAuthEvents.json (1)
1-26: LGTM! Event definitions follow security best practices.The events are well-structured with:
- Proper indexing of address parameters for efficient filtering
- Non-anonymous events for better traceability
- Clear naming convention with "Log" prefix
Let's verify the usage of these events across the codebase:
✅ Verification successful
Events are properly implemented and used in the codebase
The events are correctly:
- Declared in DSAuthEvents and inherited through DSAuth
- Emitted in authorization functions within MKR.sol
- Used consistently across the contract hierarchy (DSAuth, DSThing, DSStop)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for event emissions to ensure they're properly used rg -U "emit LogSet(Authority|Owner)" --type solLength of output: 77
Script:
#!/bin/bash # Search for event declarations and emissions echo "=== Searching for LogSetAuthority ===" rg "LogSetAuthority" echo -e "\n=== Searching for LogSetOwner ===" rg "LogSetOwner" echo -e "\n=== Searching for potential contract implementations ===" rg "contract.*DSAuth"Length of output: 2750
x/evm/embeds/abi/DSAuth.json (4)
30-57: LGTM! Getter functions are properly defined.The
ownerandauthorityview functions are correctly defined with:
- Appropriate view modifier
- Proper return type (address)
- Non-payable state mutability
58-63: LGTM! Constructor is properly defined.The constructor is correctly marked as non-payable.
64-87: LGTM! Events are consistent with DSAuthEvents.json.The event definitions match exactly with those in DSAuthEvents.json, maintaining consistency across the system.
2-29: Verify access control implementation for state-changing functions.The
setOwnerandsetAuthorityfunctions modify critical state but access control isn't visible in the ABI. Ensure the implementation includes proper authorization checks.Let's verify the implementation:
✅ Verification successful
Access control is properly implemented for state-changing functions
Both
setOwnerandsetAuthorityare protected by theauthmodifier in the Solidity implementation, and ownership changes are properly logged via events. The ABI accurately reflects the implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for access control modifiers on these functions ast-grep --pattern 'function set(Owner|Authority)($$$) { $$$ }'Length of output: 67
Script:
#!/bin/bash # Find DSAuth Solidity files fd -e sol DSAuth # If found, check the content to verify it's the right file rg -l "function setOwner|function setAuthority" $(fd -e sol DSAuth)Length of output: 119
Script:
#!/bin/bash # Get the full context of setOwner and setAuthority implementations rg -B 5 -A 5 "function set(Owner|Authority)" x/evm/embeds/contracts/MKR.sol # Look for auth-related modifiers or base contracts rg "contract DSAuth|modifier auth" x/evm/embeds/contracts/MKR.solLength of output: 628
x/evm/embeds/abi/ERC20.json (1)
1-175: LGTM! Standard ERC20 interface implementation.The ABI correctly implements all required ERC20 functions and events according to the EIP-20 specification.
x/evm/embeds/abi/DSTokenBase.json (1)
1-186: LGTM! Well-structured token base contract.The ABI correctly implements the ERC20 interface and includes a constructor for initial supply. All functions and events are properly defined.
x/evm/keeper/funtoken_from_erc20.go (2)
103-106: LGTM! Clear documentation of function steps.The function documentation clearly outlines the steps involved in creating a FunToken from an ERC20 token.
203-241: 🛠️ Refactor suggestionImprove error handling and remove duplicate logging.
The function has several issues:
- Duplicate error messages in printf and error return
- No distinction between required and optional methods
- Hardcoded method list mixing required and optional methods
Apply this diff to improve the implementation:
+// Required ERC20 methods +var requiredMethods = []string{ + "totalSupply", + "balanceOf", + "transfer", + "transferFrom", + "approve", + "allowance", + "mint", +} + +// Optional ERC20 methods +var optionalMethods = []string{ + "name", + "symbol", + "decimals", + "increaseAllowance", + "decreaseAllowance", +} func (k Keeper) checkErc20ImplementsAllRequired( ctx sdk.Context, erc20Addr gethcommon.Address, abi *gethabi.ABI, ) error { - methodNames := []string{"name", ...} - for _, methodName := range methodNames { + // Check required methods + for _, methodName := range requiredMethods { method, ok := abi.Methods[methodName] if !ok { - fmt.Printf( - "method '%s' not found in contract at %s", - methodName, - erc20Addr.Hex(), - ) return fmt.Errorf("method '%s' not found in contract at %s", methodName, erc20Addr.Hex()) } hasMethod, err := k.HasMethodInContract(ctx, erc20Addr, method) if err != nil { - fmt.Println(err) return err } if !hasMethod { - fmt.Printf("method '%s' not found in contract at %s\n", methodName, erc20Addr.Hex()) return fmt.Errorf("method '%s' not found in contract at %s", methodName, erc20Addr.Hex()) } } + + // Check optional methods + for _, methodName := range optionalMethods { + method, ok := abi.Methods[methodName] + if !ok { + continue + } + hasMethod, err := k.HasMethodInContract(ctx, erc20Addr, method) + if err != nil { + return err + } + if !hasMethod { + continue + } + } return nil }Likely invalid or redundant comment.
x/evm/keeper/funtoken_from_erc20_test.go (1)
445-448: LGTM! Clear error validation.The test correctly validates that a contract with infinite recursion in the transfer function fails to create a FunToken.
x/evm/embeds/embeds.go
Outdated
| // SmartContract_TestERC20MissingFunction 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 | ||
| SmartContract_TestERC20MissingFunction = CompiledEvmContract{ | ||
| Name: "TestERC20MissingFunction.sol", | ||
| EmbedJSON: testErc20MissingFunctionJson, | ||
| } |
There was a problem hiding this comment.
Fix incorrect documentation comment.
The documentation comment appears to be copy-pasted from TestERC20MaliciousTransfer and doesn't accurately describe this contract's purpose. Based on the PR objectives and contract name, this contract is meant to test ERC20 contracts with missing functions.
Apply this diff to fix the documentation:
- // which simulates malicious ERC20 behavior by adding gas intensive operation
- // for function transfer() intended to attack funtoken conversion from erc20 to bank coin
+ // which simulates an incomplete ERC20 contract by omitting required functions
+ // to test prevention of funtoken creation with non-compliant contracts📝 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.
| // SmartContract_TestERC20MissingFunction 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 | |
| SmartContract_TestERC20MissingFunction = CompiledEvmContract{ | |
| Name: "TestERC20MissingFunction.sol", | |
| EmbedJSON: testErc20MissingFunctionJson, | |
| } | |
| // SmartContract_TestERC20MissingFunction is a test contract | |
| // which simulates an incomplete ERC20 contract by omitting required functions | |
| // to test prevention of funtoken creation with non-compliant contracts | |
| SmartContract_TestERC20MissingFunction = CompiledEvmContract{ | |
| Name: "TestERC20MissingFunction.sol", | |
| EmbedJSON: testErc20MissingFunctionJson, | |
| } |
| @@ -106,7 +106,7 @@ func (s *BackendSuite) TestLogs() { | |||
| Nonce: s.getCurrentNonce(s.fundedAccEthAddr), | |||
| To: &precompile.PrecompileAddr_FunToken, | |||
| Data: packedArgsPass, | |||
| Gas: 1_500_000, | |||
| Gas: 2_500_000, | |||
There was a problem hiding this comment.
💡 Codebase verification
Standardize gas limit to 1,500,000 for consistency
The gas limit of 2,500,000 is inconsistent with similar operations in the codebase. Other token operations in the test files consistently use 1,500,000 gas, including:
eth/rpc/backend/gas_used_test.goeth/rpc/backend/backend_suite_test.go
Consider reducing the gas limit to 1,500,000 to maintain consistency across the test suite.
🔗 Analysis chain
Maintain consistent gas limits across similar operations.
The gas limit of 2,500,000 for sending ERC20 tokens to coin seems inconsistent with similar operations in other tests. Consider standardizing gas limits across similar operations to ensure consistent testing and better detection of gas usage regressions.
Let's verify gas usage patterns for similar operations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar ERC20 token operations and their gas limits
rg -A 5 "PrecompileAddr_FunToken.*Gas:" .
Length of output: 43
Script:
#!/bin/bash
# Search for gas limit patterns in test files
rg -A 2 "Gas:\s+\d" --type go --glob "*_test.go"
# Search specifically in the eth/rpc/backend directory for context
rg -A 5 "Gas:" eth/rpc/backend/
Length of output: 10817
| { | ||
| "anonymous": true, | ||
| "inputs": [ | ||
| { | ||
| "indexed": true, | ||
| "name": "sig", | ||
| "type": "bytes4" | ||
| }, | ||
| { | ||
| "indexed": true, | ||
| "name": "guy", | ||
| "type": "address" | ||
| }, | ||
| { | ||
| "indexed": true, | ||
| "name": "foo", | ||
| "type": "bytes32" | ||
| }, | ||
| { | ||
| "indexed": true, | ||
| "name": "bar", | ||
| "type": "bytes32" | ||
| }, | ||
| { | ||
| "indexed": false, | ||
| "name": "wad", | ||
| "type": "uint256" | ||
| }, | ||
| { | ||
| "indexed": false, | ||
| "name": "fax", | ||
| "type": "bytes" | ||
| } | ||
| ], | ||
| "name": "LogNote", | ||
| "type": "event" | ||
| }, |
There was a problem hiding this comment.
Reduce indexed parameters in LogNote event.
The LogNote event has 4 indexed parameters (sig, guy, foo, bar), but Solidity only allows a maximum of 3 indexed parameters per event. Additionally, parameter names like foo, bar, guy are not descriptive enough.
Consider:
- Reducing the number of indexed parameters to 3
- Using more descriptive parameter names
{
"anonymous": true,
"inputs": [
{
"indexed": true,
"name": "sig",
"type": "bytes4"
},
{
"indexed": true,
- "name": "guy",
+ "name": "caller",
"type": "address"
},
{
"indexed": true,
- "name": "foo",
+ "name": "param1",
"type": "bytes32"
},
{
- "indexed": true,
+ "indexed": false,
- "name": "bar",
+ "name": "param2",
"type": "bytes32"
},
{
"indexed": false,
"name": "wad",
"type": "uint256"
},
{
"indexed": false,
"name": "fax",
"type": "bytes"
}
],
"name": "LogNote",
"type": "event"
}📝 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.
| { | |
| "anonymous": true, | |
| "inputs": [ | |
| { | |
| "indexed": true, | |
| "name": "sig", | |
| "type": "bytes4" | |
| }, | |
| { | |
| "indexed": true, | |
| "name": "guy", | |
| "type": "address" | |
| }, | |
| { | |
| "indexed": true, | |
| "name": "foo", | |
| "type": "bytes32" | |
| }, | |
| { | |
| "indexed": true, | |
| "name": "bar", | |
| "type": "bytes32" | |
| }, | |
| { | |
| "indexed": false, | |
| "name": "wad", | |
| "type": "uint256" | |
| }, | |
| { | |
| "indexed": false, | |
| "name": "fax", | |
| "type": "bytes" | |
| } | |
| ], | |
| "name": "LogNote", | |
| "type": "event" | |
| }, | |
| { | |
| "anonymous": true, | |
| "inputs": [ | |
| { | |
| "indexed": true, | |
| "name": "sig", | |
| "type": "bytes4" | |
| }, | |
| { | |
| "indexed": true, | |
| "name": "caller", | |
| "type": "address" | |
| }, | |
| { | |
| "indexed": true, | |
| "name": "param1", | |
| "type": "bytes32" | |
| }, | |
| { | |
| "indexed": false, | |
| "name": "param2", | |
| "type": "bytes32" | |
| }, | |
| { | |
| "indexed": false, | |
| "name": "wad", | |
| "type": "uint256" | |
| }, | |
| { | |
| "indexed": false, | |
| "name": "fax", | |
| "type": "bytes" | |
| } | |
| ], | |
| "name": "LogNote", | |
| "type": "event" | |
| }, |
| { | ||
| "anonymous": true, | ||
| "inputs": [ | ||
| { | ||
| "indexed": true, | ||
| "name": "sig", | ||
| "type": "bytes4" | ||
| }, | ||
| { | ||
| "indexed": true, | ||
| "name": "guy", | ||
| "type": "address" | ||
| }, | ||
| { | ||
| "indexed": true, | ||
| "name": "foo", | ||
| "type": "bytes32" | ||
| }, | ||
| { | ||
| "indexed": true, | ||
| "name": "bar", | ||
| "type": "bytes32" | ||
| }, | ||
| { | ||
| "indexed": false, | ||
| "name": "wad", | ||
| "type": "uint256" | ||
| }, | ||
| { | ||
| "indexed": false, | ||
| "name": "fax", | ||
| "type": "bytes" | ||
| } | ||
| ], | ||
| "name": "LogNote", | ||
| "type": "event" | ||
| }, |
There was a problem hiding this comment.
Potential event indexing issue in LogNote.
The LogNote event has more than 3 indexed parameters (sig, guy, foo, bar), which exceeds Solidity's limit of 3 indexed parameters per event.
- "anonymous": true,
- "inputs": [
- {
- "indexed": true,
- "name": "sig",
- "type": "bytes4"
- },
- {
- "indexed": true,
- "name": "guy",
- "type": "address"
- },
- {
- "indexed": true,
- "name": "foo",
- "type": "bytes32"
- },
- {
- "indexed": true,
- "name": "bar",
- "type": "bytes32"
- },
+ "anonymous": true,
+ "inputs": [
+ {
+ "indexed": true,
+ "name": "sig",
+ "type": "bytes4"
+ },
+ {
+ "indexed": true,
+ "name": "guy",
+ "type": "address"
+ },
+ {
+ "indexed": true,
+ "name": "foo",
+ "type": "bytes32"
+ },
+ {
+ "indexed": false,
+ "name": "bar",
+ "type": "bytes32"
+ },📝 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.
| { | |
| "anonymous": true, | |
| "inputs": [ | |
| { | |
| "indexed": true, | |
| "name": "sig", | |
| "type": "bytes4" | |
| }, | |
| { | |
| "indexed": true, | |
| "name": "guy", | |
| "type": "address" | |
| }, | |
| { | |
| "indexed": true, | |
| "name": "foo", | |
| "type": "bytes32" | |
| }, | |
| { | |
| "indexed": true, | |
| "name": "bar", | |
| "type": "bytes32" | |
| }, | |
| { | |
| "indexed": false, | |
| "name": "wad", | |
| "type": "uint256" | |
| }, | |
| { | |
| "indexed": false, | |
| "name": "fax", | |
| "type": "bytes" | |
| } | |
| ], | |
| "name": "LogNote", | |
| "type": "event" | |
| }, | |
| { | |
| "anonymous": true, | |
| "inputs": [ | |
| { | |
| "indexed": true, | |
| "name": "sig", | |
| "type": "bytes4" | |
| }, | |
| { | |
| "indexed": true, | |
| "name": "guy", | |
| "type": "address" | |
| }, | |
| { | |
| "indexed": true, | |
| "name": "foo", | |
| "type": "bytes32" | |
| }, | |
| { | |
| "indexed": false, | |
| "name": "bar", | |
| "type": "bytes32" | |
| }, | |
| { | |
| "indexed": false, | |
| "name": "wad", | |
| "type": "uint256" | |
| }, | |
| { | |
| "indexed": false, | |
| "name": "fax", | |
| "type": "bytes" | |
| } | |
| ], | |
| "name": "LogNote", | |
| "type": "event" | |
| }, |
| { | ||
| "anonymous": true, | ||
| "inputs": [ | ||
| { | ||
| "indexed": true, | ||
| "name": "sig", | ||
| "type": "bytes4" | ||
| }, | ||
| { | ||
| "indexed": true, | ||
| "name": "guy", | ||
| "type": "address" | ||
| }, | ||
| { | ||
| "indexed": true, | ||
| "name": "foo", | ||
| "type": "bytes32" | ||
| }, | ||
| { | ||
| "indexed": true, | ||
| "name": "bar", | ||
| "type": "bytes32" | ||
| }, | ||
| { | ||
| "indexed": false, | ||
| "name": "wad", | ||
| "type": "uint256" | ||
| }, | ||
| { | ||
| "indexed": false, | ||
| "name": "fax", | ||
| "type": "bytes" | ||
| } | ||
| ], | ||
| "name": "LogNote", | ||
| "type": "event" | ||
| } |
There was a problem hiding this comment.
Reduce the number of indexed parameters in LogNote event.
The LogNote event has 4 indexed parameters (sig, guy, foo, bar), which exceeds Solidity's limit of 3 indexed parameters per event. This could cause compatibility issues with some tools and increase gas costs.
Apply this diff to fix the issue:
{
"anonymous": true,
"inputs": [
{
"indexed": true,
"name": "sig",
"type": "bytes4"
},
{
"indexed": true,
"name": "guy",
"type": "address"
},
{
"indexed": true,
"name": "foo",
"type": "bytes32"
},
{
- "indexed": true,
+ "indexed": false,
"name": "bar",
"type": "bytes32"
},
{
"indexed": false,
"name": "wad",
"type": "uint256"
},
{
"indexed": false,
"name": "fax",
"type": "bytes"
}
],
"name": "LogNote",
"type": "event"
}📝 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.
| { | |
| "anonymous": true, | |
| "inputs": [ | |
| { | |
| "indexed": true, | |
| "name": "sig", | |
| "type": "bytes4" | |
| }, | |
| { | |
| "indexed": true, | |
| "name": "guy", | |
| "type": "address" | |
| }, | |
| { | |
| "indexed": true, | |
| "name": "foo", | |
| "type": "bytes32" | |
| }, | |
| { | |
| "indexed": true, | |
| "name": "bar", | |
| "type": "bytes32" | |
| }, | |
| { | |
| "indexed": false, | |
| "name": "wad", | |
| "type": "uint256" | |
| }, | |
| { | |
| "indexed": false, | |
| "name": "fax", | |
| "type": "bytes" | |
| } | |
| ], | |
| "name": "LogNote", | |
| "type": "event" | |
| } | |
| { | |
| "anonymous": true, | |
| "inputs": [ | |
| { | |
| "indexed": true, | |
| "name": "sig", | |
| "type": "bytes4" | |
| }, | |
| { | |
| "indexed": true, | |
| "name": "guy", | |
| "type": "address" | |
| }, | |
| { | |
| "indexed": true, | |
| "name": "foo", | |
| "type": "bytes32" | |
| }, | |
| { | |
| "indexed": false, | |
| "name": "bar", | |
| "type": "bytes32" | |
| }, | |
| { | |
| "indexed": false, | |
| "name": "wad", | |
| "type": "uint256" | |
| }, | |
| { | |
| "indexed": false, | |
| "name": "fax", | |
| "type": "bytes" | |
| } | |
| ], | |
| "name": "LogNote", | |
| "type": "event" | |
| } |
x/evm/embeds/embeds.go
Outdated
| // SmartContract_TestERC20MissingFunction 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 | ||
| SmartContract_TestERC20MissingFunction = CompiledEvmContract{ |
x/evm/keeper/funtoken_from_erc20.go
Outdated
| "transferFrom", | ||
| "increaseAllowance", | ||
| "decreaseAllowance", | ||
| "mint", |
There was a problem hiding this comment.
so we require mint but do not require burn? Technically ERC20 could have no mint and just have all coins supply upon creation.
There was a problem hiding this comment.
I've limited the scope of these function to
methodNames := []string{
"balanceOf",
"transfer",
"symbol",
"decimals",
"name",
"totalSupply",
}| // TestFunTokenInfiniteRecursionERC20 creates a funtoken from a contract | ||
| // with a malicious recursive balanceOf() and transfer() functions. | ||
| func (s *FunTokenFromErc20Suite) TestFunTokenMissingMintFunction() { |
| Args: jsonTxArgs, | ||
| // This gas cap is big enough, if out of gas, something is suspicious | ||
| // in the transfer function. | ||
| GasCap: 690000000000000000, |
There was a problem hiding this comment.
why the value is so uneven?
There was a problem hiding this comment.
It's just a big gas cap so we don't fail the estimate gas due to passing the gas cap
| if strings.Contains(err.Error(), "Estimate gas VMError: execution reverted, but unable to parse reason") { | ||
| return false, nil | ||
| } | ||
| return true, nil |
There was a problem hiding this comment.
so, we accept err != nil as success (method exists)?
There was a problem hiding this comment.
Yes, sometimes there can be issue specific to how we call the function i.e. can't call the transfer function with 0 etc. The error we get if the function does not exist is execution reverted, but unable to parse reason.
There was a problem hiding this comment.
I'm not a fan of hard-checking that the error string equals Estimate gas VMError: execution reverted, but unable to parse reason because the string could change in the future. It's better to use errors.Is against the sentinel error value.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
x/evm/embeds/contracts/TestERC20InvalidFunction.sol (1)
87-89:⚠️ Potential issueFix incomplete _transfer implementation.
The
_transferfunction lacks parameters and implementation, and doesn't emit the required Transfer event.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
x/evm/embeds/artifacts/contracts/TestERC20InvalidFunction.sol/TestERC20InvalidFunction.json(1 hunks)x/evm/embeds/artifacts/contracts/TestERC20MaliciousTransfer.sol/TestERC20MaliciousTransfer.json(1 hunks)x/evm/embeds/artifacts/contracts/TestERC20MissingFunction.sol/TestERC20MissingFunction.json(1 hunks)x/evm/embeds/artifacts/contracts/TestERC20TransferWithFee.sol/TestERC20TransferWithFee.json(1 hunks)x/evm/embeds/contracts/TestERC20InvalidFunction.sol(1 hunks)x/evm/embeds/contracts/TestERC20MaliciousTransfer.sol(2 hunks)x/evm/embeds/contracts/TestERC20MissingFunction.sol(1 hunks)x/evm/embeds/contracts/TestERC20TransferWithFee.sol(1 hunks)x/evm/embeds/embeds.go(3 hunks)x/evm/embeds/embeds_test.go(1 hunks)x/evm/keeper/funtoken_from_erc20.go(5 hunks)x/evm/keeper/funtoken_from_erc20_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- x/evm/embeds/artifacts/contracts/TestERC20MissingFunction.sol/TestERC20MissingFunction.json
- x/evm/embeds/artifacts/contracts/TestERC20InvalidFunction.sol/TestERC20InvalidFunction.json
🚧 Files skipped from review as they are similar to previous changes (3)
- x/evm/embeds/embeds_test.go
- x/evm/embeds/contracts/TestERC20MaliciousTransfer.sol
- x/evm/embeds/contracts/TestERC20TransferWithFee.sol
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: integration-tests
- GitHub Check: e2e-evm
🔇 Additional comments (11)
x/evm/keeper/funtoken_from_erc20.go (2)
103-106: LGTM!The documentation accurately reflects the new step for verifying ERC20 functions.
160-164: LGTM!The implementation correctly adds the verification step for ERC20 functions before proceeding with the FunToken creation.
x/evm/keeper/funtoken_from_erc20_test.go (3)
445-448: LGTM!The test correctly validates that a contract with recursive functions is rejected during FunToken creation.
450-489: LGTM!The test correctly validates that a contract missing the required
balanceOffunction is rejected during FunToken creation.
491-529: LGTM!The test correctly validates that a contract with an invalid
transferfunction signature is rejected during FunToken creation.x/evm/embeds/artifacts/contracts/TestERC20MaliciousTransfer.sol/TestERC20MaliciousTransfer.json (1)
298-299: Verify the security implications of the malicious transfer contract.This contract appears to be designed for testing malicious ERC20 transfer scenarios. Ensure it's properly isolated and only used in test environments.
Run the following script to check for any production usage:
✅ Verification successful
Contract is properly isolated in test environment
The TestERC20MaliciousTransfer contract is correctly contained within the test infrastructure:
- Explicitly documented as a test contract for simulating malicious behavior
- Only used in test files for security testing scenarios
- No production usage detected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any production usage of the malicious transfer contract # Search for imports or deployments of TestERC20MaliciousTransfer # Look for contract imports rg -l "TestERC20MaliciousTransfer" --type rust --type go --type js --type ts # Look for contract deployments rg -l "new TestERC20MaliciousTransfer|deploy.*TestERC20MaliciousTransfer" --type rust --type go --type js --type tsLength of output: 286
Script:
#!/bin/bash # Check how TestERC20MaliciousTransfer is used in embeds.go and test files # Show context of TestERC20MaliciousTransfer in embeds.go echo "=== embeds.go usage ===" rg -C 5 "TestERC20MaliciousTransfer" x/evm/embeds/embeds.go # Show test usage patterns echo -e "\n=== Test file usage ===" rg -C 5 "TestERC20MaliciousTransfer" "x/evm/keeper/funtoken_from_erc20_test.go" "x/evm/embeds/embeds_test.go"Length of output: 3884
x/evm/embeds/artifacts/contracts/TestERC20TransferWithFee.sol/TestERC20TransferWithFee.json (1)
293-294: Review fee calculation for edge cases.The contract implements a 10% fee on transfers which could lead to rounding issues with small transfer amounts. Consider:
- Minimum transfer amount to ensure fee calculation is meaningful
- Rounding behavior for amounts that don't divide evenly
Run the following script to analyze the fee calculation implementation:
x/evm/embeds/contracts/TestERC20MissingFunction.sol (1)
46-54: Verify that commenting outbalanceOfaligns with test objectives.The
balanceOffunction is a required method in the ERC20 interface. Commenting it out makes this contract non-compliant with ERC20 standards, which appears intentional based on the contract name and PR objectives.Let's verify if this is the only required ERC20 function that's intentionally missing:
✅ Verification successful
Confirmed: Missing functions align with test contract objectives
The contract intentionally omits multiple required ERC20 functions (approve, allowance, transferFrom) and comments out balanceOf, while maintaining others (totalSupply, transfer). This pattern aligns with the contract's purpose as a test fixture for handling non-compliant ERC20 implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other required ERC20 functions ast-grep --pattern 'function approve($$$) public $$$' ast-grep --pattern 'function allowance($$$) public $$$' ast-grep --pattern 'function transferFrom($$$) public $$$'Length of output: 174
Script:
#!/bin/bash # Find the contract file fd TestERC20MissingFunction.sol # Search for all ERC20 core functions with context echo "Searching for ERC20 functions in the contract:" rg -U "function (transfer|approve|allowance|transferFrom|totalSupply)" -A 2 -B 2 $(fd TestERC20MissingFunction.sol)Length of output: 1252
x/evm/embeds/contracts/TestERC20InvalidFunction.sol (1)
64-71: Fix invalid transfer function signature.The
transferfunction is missing the requiredamountparameter, making it non-compliant with ERC20 standards. This appears intentional based on the contract name and PR objectives.Let's verify if this is the only function with an invalid signature:
✅ Verification successful
Confirmed: Invalid signature is intentional for testing purposes
This is a test contract specifically designed to implement invalid ERC20 functions, as evidenced by the contract name
TestERC20InvalidFunction. The missingamountparameter in thetransferfunction is intentional for testing purposes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other ERC20 functions with invalid signatures ast-grep --pattern 'function approve($$$) public $$$' ast-grep --pattern 'function allowance($$$) public $$$' ast-grep --pattern 'function transferFrom($$$) public $$$'Length of output: 174
Script:
#!/bin/bash # View the complete contract implementation cat x/evm/embeds/contracts/TestERC20InvalidFunction.solLength of output: 2366
x/evm/embeds/embeds.go (2)
105-107: 🛠️ Refactor suggestionFix incorrect documentation comment.
The documentation comment incorrectly states that the contract simulates malicious behavior by missing the
balanceOffunction. However, based on the PR objectives and contract implementation, this contract is designed to test prevention of funtoken creation with non-compliant contracts.Apply this diff to fix the documentation:
- // which simulates malicious ERC20 behavior by missing the balanceOf function + // which simulates an incomplete ERC20 contract by omitting required functions + // to test prevention of funtoken creation with non-compliant contractsLikely invalid or redundant comment.
111-114: 🛠️ Refactor suggestionFix incorrect documentation comment.
The documentation comment incorrectly states that the contract simulates malicious behavior. However, based on the PR objectives and contract implementation, this contract is designed to test prevention of funtoken creation with non-compliant contracts.
Apply this diff to fix the documentation:
- // which simulates malicious ERC20 behavior by adding an invalid transfer - // function with no `amount` parameter + // which simulates an invalid ERC20 contract by implementing a non-compliant transfer + // function to test prevention of funtoken creation with non-compliant contractsLikely invalid or redundant comment.
| func (k Keeper) checkErc20ImplementsAllRequired( | ||
| ctx sdk.Context, erc20Addr gethcommon.Address, abi *gethabi.ABI, | ||
| ) error { | ||
| methodNames := []string{ | ||
| "balanceOf", | ||
| "transfer", | ||
| "symbol", | ||
| "decimals", | ||
| "name", | ||
| "totalSupply", | ||
| } | ||
|
|
||
| for _, methodName := range methodNames { | ||
| fmt.Println("@@@@@@@@@@@@@@@@@@@@@@@@ Checking method: ", methodName) | ||
| method, ok := abi.Methods[methodName] | ||
| if !ok { | ||
| fmt.Println( | ||
| "method not found in contract at ", | ||
| methodName, | ||
| erc20Addr.Hex(), | ||
| ) | ||
| return fmt.Errorf("method '%s' not found in contract at %s", methodName, erc20Addr.Hex()) | ||
| } | ||
| fmt.Println("@@@@@@@@@@@@@@@@@@@@@@@@ asking: ", methodName) | ||
| hasMethod, err := k.HasMethodInContract(ctx, erc20Addr, method) | ||
| if err != nil { | ||
| fmt.Println("method not found in contract at \n", methodName, erc20Addr.Hex()) | ||
| return err | ||
| } | ||
| if !hasMethod { | ||
| fmt.Println("method not found in contract at \n", methodName, erc20Addr.Hex()) | ||
| return fmt.Errorf("method '%s' not found in contract at %s", methodName, erc20Addr.Hex()) | ||
| } | ||
| fmt.Println("@@@@@@@@@@@@@@@@@@@@@@@@@@@@@Method found: ", methodName) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clean up debug statements and improve error handling.
- Remove debug print statements (
fmt.Println). - Consolidate duplicate error messages.
- Enhance error messages to be more descriptive.
Apply this diff to improve the implementation:
func (k Keeper) checkErc20ImplementsAllRequired(
ctx sdk.Context, erc20Addr gethcommon.Address, abi *gethabi.ABI,
) error {
methodNames := []string{
"balanceOf",
"transfer",
"symbol",
"decimals",
"name",
"totalSupply",
}
for _, methodName := range methodNames {
- fmt.Println("@@@@@@@@@@@@@@@@@@@@@@@@ Checking method: ", methodName)
method, ok := abi.Methods[methodName]
if !ok {
- fmt.Println(
- "method not found in contract at ",
- methodName,
- erc20Addr.Hex(),
- )
- return fmt.Errorf("method '%s' not found in contract at %s", methodName, erc20Addr.Hex())
+ return fmt.Errorf("required ERC20 method '%s' not found in ABI at %s", methodName, erc20Addr.Hex())
}
- fmt.Println("@@@@@@@@@@@@@@@@@@@@@@@@ asking: ", methodName)
hasMethod, err := k.HasMethodInContract(ctx, erc20Addr, method)
if err != nil {
- fmt.Println("method not found in contract at \n", methodName, erc20Addr.Hex())
return err
}
if !hasMethod {
- fmt.Println("method not found in contract at \n", methodName, erc20Addr.Hex())
- return fmt.Errorf("method '%s' not found in contract at %s", methodName, erc20Addr.Hex())
+ return fmt.Errorf("required ERC20 method '%s' not implemented in contract at %s", methodName, erc20Addr.Hex())
}
- fmt.Println("@@@@@@@@@@@@@@@@@@@@@@@@@@@@@Method found: ", methodName)
}
return nil
}📝 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.
| func (k Keeper) checkErc20ImplementsAllRequired( | |
| ctx sdk.Context, erc20Addr gethcommon.Address, abi *gethabi.ABI, | |
| ) error { | |
| methodNames := []string{ | |
| "balanceOf", | |
| "transfer", | |
| "symbol", | |
| "decimals", | |
| "name", | |
| "totalSupply", | |
| } | |
| for _, methodName := range methodNames { | |
| fmt.Println("@@@@@@@@@@@@@@@@@@@@@@@@ Checking method: ", methodName) | |
| method, ok := abi.Methods[methodName] | |
| if !ok { | |
| fmt.Println( | |
| "method not found in contract at ", | |
| methodName, | |
| erc20Addr.Hex(), | |
| ) | |
| return fmt.Errorf("method '%s' not found in contract at %s", methodName, erc20Addr.Hex()) | |
| } | |
| fmt.Println("@@@@@@@@@@@@@@@@@@@@@@@@ asking: ", methodName) | |
| hasMethod, err := k.HasMethodInContract(ctx, erc20Addr, method) | |
| if err != nil { | |
| fmt.Println("method not found in contract at \n", methodName, erc20Addr.Hex()) | |
| return err | |
| } | |
| if !hasMethod { | |
| fmt.Println("method not found in contract at \n", methodName, erc20Addr.Hex()) | |
| return fmt.Errorf("method '%s' not found in contract at %s", methodName, erc20Addr.Hex()) | |
| } | |
| fmt.Println("@@@@@@@@@@@@@@@@@@@@@@@@@@@@@Method found: ", methodName) | |
| } | |
| return nil | |
| } | |
| func (k Keeper) checkErc20ImplementsAllRequired( | |
| ctx sdk.Context, erc20Addr gethcommon.Address, abi *gethabi.ABI, | |
| ) error { | |
| methodNames := []string{ | |
| "balanceOf", | |
| "transfer", | |
| "symbol", | |
| "decimals", | |
| "name", | |
| "totalSupply", | |
| } | |
| for _, methodName := range methodNames { | |
| method, ok := abi.Methods[methodName] | |
| if !ok { | |
| return fmt.Errorf("required ERC20 method '%s' not found in ABI at %s", methodName, erc20Addr.Hex()) | |
| } | |
| hasMethod, err := k.HasMethodInContract(ctx, erc20Addr, method) | |
| if err != nil { | |
| return err | |
| } | |
| if !hasMethod { | |
| return fmt.Errorf("required ERC20 method '%s' not implemented in contract at %s", methodName, erc20Addr.Hex()) | |
| } | |
| } | |
| return nil | |
| } |
| function transfer( | ||
| address to, | ||
| uint256 amount | ||
| ) public virtual returns (bool) { | ||
| _transfer(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Fix incomplete transfer implementation.
The transfer function doesn't use the amount parameter and the _transfer call doesn't pass any parameters, making it non-compliant with ERC20 standards.
Apply this diff to fix the implementation:
-function transfer(address to, uint256 amount) public virtual returns (bool) {
- _transfer();
- return true;
+function transfer(address to, uint256 amount) public virtual returns (bool) {
+ _transfer(msg.sender, to, amount);
+ return true;
+}📝 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.
| function transfer( | |
| address to, | |
| uint256 amount | |
| ) public virtual returns (bool) { | |
| _transfer(); | |
| return true; | |
| } | |
| function transfer( | |
| address to, | |
| uint256 amount | |
| ) public virtual returns (bool) { | |
| _transfer(msg.sender, to, amount); | |
| return true; | |
| } |
| function _transfer() internal virtual returns (bool) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Fix incomplete _transfer implementation.
The _transfer function lacks parameters and implementation, and doesn't emit the required Transfer event.
Apply this diff to fix the implementation:
-function _transfer() internal virtual returns (bool) {
- return true;
+function _transfer(
+ address from,
+ address to,
+ uint256 amount
+) internal virtual returns (bool) {
+ require(from != address(0), "ERC20: transfer from the zero address");
+ require(to != address(0), "ERC20: transfer to the zero address");
+ uint256 fromBalance = _balances[from];
+ require(fromBalance >= amount, "ERC20: transfer amount exceeds balance");
+ unchecked {
+ _balances[from] = fromBalance - amount;
+ _balances[to] += amount;
+ }
+ emit Transfer(from, to, amount);
+ return true;
}Committable suggestion skipped: line range outside the PR's diff.
| constructor(string memory name_, string memory symbol_, uint8 decimals_) { | ||
| if (decimals_ == 0) { | ||
| // just to silence warnings | ||
| _totalSupply = 0; | ||
| } | ||
| _name = name_; | ||
| _symbol = symbol_; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused decimals parameter.
The constructor accepts a decimals_ parameter but only uses it in a redundant condition that doesn't affect the contract's state. The decimals() function always returns 18 regardless of this parameter.
Apply this diff to fix the implementation:
-constructor(string memory name_, string memory symbol_, uint8 decimals_) {
- if (decimals_ == 0) {
- // just to silence warnings
- _totalSupply = 0;
- }
- _name = name_;
- _symbol = symbol_;
+constructor(string memory name_, string memory symbol_) {
+ _name = name_;
+ _symbol = symbol_;
}📝 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_) { | |
| if (decimals_ == 0) { | |
| // just to silence warnings | |
| _totalSupply = 0; | |
| } | |
| _name = name_; | |
| _symbol = symbol_; | |
| } | |
| constructor(string memory name_, string memory symbol_) { | |
| _name = name_; | |
| _symbol = symbol_; | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2150 +/- ##
==========================================
- Coverage 64.99% 64.99% -0.01%
==========================================
Files 277 278 +1
Lines 22346 22456 +110
==========================================
+ Hits 14524 14595 +71
- Misses 6832 6863 +31
- Partials 990 998 +8
|
| if strings.Contains(err.Error(), "Estimate gas VMError: execution reverted, but unable to parse reason") { | ||
| return false, nil | ||
| } | ||
| return true, nil |
There was a problem hiding this comment.
I'm not a fan of hard-checking that the error string equals Estimate gas VMError: execution reverted, but unable to parse reason because the string could change in the future. It's better to use errors.Is against the sentinel error value.
| ChainId: k.EthChainID(ctx).Int64(), | ||
| } | ||
|
|
||
| _, err = k.EstimateGasForEvmCallType(goCtx, ðCallRequest, evm.CallTypeRPC) |
There was a problem hiding this comment.
Would prefer not to use EstimateGas because it binary searches for the appropriate gas amount, leading to multiple EVM calls. Maybe use CallContractWithInput directly?
| } | ||
|
|
||
| for _, methodName := range methodNames { | ||
| fmt.Println("@@@@@@@@@@@@@@@@@@@@@@@@ Checking method: ", methodName) |
There was a problem hiding this comment.
Do we want this Println in production code?
This pull request includes changes to the
TestERC20contract and its associated ABI and artifact files. The main changes involve adding a newmintfunction to the contract and updating the corresponding JSON files to reflect this addition.Changes to the
TestERC20contract:evm-e2e/contracts/TestERC20.sol: Added a newmintfunction to allow minting of tokens to a specified address.Updates to ABI and artifact files:
x/evm/embeds/abi/ERC20Minter.json: Removed themintfunction definition.x/evm/embeds/artifacts/contracts/TestERC20.sol/TestERC20.json: Added themintfunction definition to match the updated contract.