fix(evm): issue with infinite recursion in erc20 funtoken contracts #2129
Conversation
WalkthroughThis pull request introduces a new entry in the changelog for the Nibiru EVM project, highlighting a bug fix related to infinite recursion in ERC20 FunToken contracts. A new smart contract, Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 1
🧹 Nitpick comments (7)
x/evm/embeds/contracts/TestInfiniteRecursionERC20.sol (2)
7-11: Consider clarifying the mint amount or introducing a parameter.
Currently, the contract mints a fixed supply of 1,000,000 tokens (multiplied by 10^18) to the deployer. If the test scenario needs different initial supplies, consider making this parameter configurable.
34-40: Attacks rely on zero address.
BothattackBalance()andattackTransfer()use the zero address. This is likely designed to test malicious scenarios. Consider adding comments or requiring a configurable target address for broader test coverage.x/evm/keeper/erc20.go (1)
29-35: Ensure the 63/64 gas rule is well-documented.
ThegetCallGasWithLimitfunction logically applies EIP-150, returningmin((availableGas * 63) / 64, gasLimit). Consider expanding the comment to mention the rationale for this approach and possible side effects if future gas-related upgrades occur.x/evm/embeds/embeds.go (2)
123-128: Document malicious recursion intent.
The contract name and comments indicate an infinite recursion test, but consider clarifying it further to avoid confusion for new maintainers.
143-143: Loading new contracts on init.
Automatically loadingSmartContract_TestInfiniteRecursionERC20oninitis convenient for testing, but for a production environment, evaluate on-demand or carefully manage which contracts are loaded globally.x/evm/keeper/funtoken_from_erc20_test.go (2)
391-407: Check for revert or fallback.
When deploying a contract with known malicious recursion, consider verifying if the constructor itself can trigger any recursion. Currently it’s safe, but for more rigorous testing, you might guard or log constructor calls.
428-441: ValidateattackBalanceoutcome.
This test confirms no error is raised. If desired, a deeper check (like verifying state changes or confirming logs) might clarify that no partial recursion occurred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)x/evm/embeds/artifacts/contracts/TestInfiniteRecursionERC20.sol/TestInfiniteRecursionERC20.json(1 hunks)x/evm/embeds/contracts/TestInfiniteRecursionERC20.sol(1 hunks)x/evm/embeds/embeds.go(3 hunks)x/evm/embeds/embeds_test.go(1 hunks)x/evm/keeper/erc20.go(7 hunks)x/evm/keeper/funtoken_from_erc20_test.go(1 hunks)
👮 Files not reviewed due to content moderation or server errors (2)
- x/evm/embeds/artifacts/contracts/TestInfiniteRecursionERC20.sol/TestInfiniteRecursionERC20.json
- CHANGELOG.md
🔇 Additional comments (12)
x/evm/embeds/contracts/TestInfiniteRecursionERC20.sol (1)
13-22: Use caution with unhandled staticcall return values.
This staticcall to FUNTOKEN_PRECOMPILE_ADDRESS discards returned data. In real scenarios, ignoring unsuccessful calls might introduce silent failures or mislead logic. Consider verifying the success and returned data for production contracts.
x/evm/keeper/erc20.go (6)
71-71: Appropriate usage of getCallGasWithLimit.
Replacing hardcoded gas logic with dynamic calculation mitigates potential recursion vulnerabilities.
93-93: Consistent approach for transfer.
Applying the same dynamic gas limit here ensures uniform handling across contract calls.
152-152: Maintain consistency for burn.
Using getCallGasWithLimit for burn keeps consistency in gas usage patterns.
185-185: Parameter usage for read-only calls.
Erc20GasLimitQuery is applied with getCallGasWithLimit for retrieving the ERC20 string. Ensure minimal overhead for read-only calls.
213-213: Same dynamic query gas limit for decimals.
Using the 63/64 rule prevents malicious decimals retrieval from causing unforeseen loops or gas exhaustion.
243-243: Apply safe approach for BalanceOf.
Adopting the dynamic limit for balance queries confirms no malicious recursion can bypass gas constraints.
x/evm/embeds/embeds_test.go (1)
22-22: Expanded test coverage.
Including SmartContract_TestInfiniteRecursionERC20.MustLoad() ensures the new contract is also verified during load tests, catching any early compile or embed issues.
x/evm/embeds/embeds.go (1)
40-41: Keep artifact paths in sync.
Ensure that the newly embedded file path matches any directory structure changes. Inconsistent naming or file moves can break the embed.
x/evm/keeper/funtoken_from_erc20_test.go (3)
388-390: Clear test documentation.
The docstring accurately describes the malicious recursion checks. Good clarity for future reference.
409-427: Verify creation fees are fully tested.
We see the fun token creation after soldiering funds. Ensure coverage for corner cases such as partially funded attempts.
442-453: Reverts as expected for malicious transfer.
This confirms the system halts a malicious infinite recursion. The expectation is that the EVM reverts. This behavior is correct for security.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2129 +/- ##
==========================================
+ Coverage 64.93% 64.99% +0.05%
==========================================
Files 273 273
Lines 21656 21661 +5
==========================================
+ Hits 14063 14078 +15
+ Misses 6617 6611 -6
+ Partials 976 972 -4
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
x/evm/keeper/erc20.go (2)
30-36: Enhance technical accuracy of gas calculationThe gas calculation implementation can be made more technically accurate according to EIP-150.
Consider this alternative implementation that more closely follows the EIP-150 specification:
func getCallGasWithLimit(ctx sdk.Context, gasLimit uint64) uint64 { availableGas := ctx.GasMeter().GasRemaining() - callGas := availableGas - uint64(math.Floor(float64(availableGas)/64)) + callGas := availableGas - (availableGas / 64) return min(callGas, gasLimit) }This change:
- Removes unnecessary floating-point operations
- Matches EIP-150's
N - floor(N/64)formula more directly- Is more gas efficient
72-72: LGTM! Systematic protection against recursive callsThe consistent application of
getCallGasWithLimitacross all ERC20 operations effectively prevents infinite recursion by implementing EIP-150's gas limiting mechanism. This change systematically protects against potential recursive attacks in:
- Mint operations
- Transfer operations
- Burn operations
- Loading operations (name, symbol, decimals, etc.)
This implementation follows the defense-in-depth principle by applying gas limits at every contract interaction point, making it resilient against various forms of recursive attacks.
Also applies to: 94-94, 153-153, 186-186, 214-214, 244-244
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/evm/keeper/erc20.go(8 hunks)
🧰 Additional context used
📓 Learnings (1)
📓 Common learnings
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 (1)
x/evm/keeper/erc20.go (1)
Line range hint 1-258: Verify complete coverage of gas limiting
Let's ensure all contract calls are protected with the new gas limiting mechanism.
✅ Verification successful
Let me gather more specific information about the unprotected calls.
Based on the analysis of the shell script results, I can now provide the final response:
All ERC20-related contract calls are properly protected with gas limiting mechanism
The verification shows that:
-
All ERC20 operations in
x/evm/keeper/erc20.goconsistently usegetCallGasWithLimitwith appropriate gas limits:Erc20GasLimitDeploy(2M) for deploymentErc20GasLimitQuery(100K) for queriesErc20GasLimitExecute(200K) for state-changing operations
-
The gas limiting is properly implemented across all ERC20 operations:
- Mint: Uses
getCallGasWithLimit(ctx, Erc20GasLimitExecute) - Transfer: Uses
getCallGasWithLimit(ctx, Erc20GasLimitExecute) - Burn: Uses
getCallGasWithLimit(ctx, Erc20GasLimitExecute) - Query operations (name, symbol, decimals, balanceOf): Use
getCallGasWithLimit(ctx, Erc20GasLimitQuery)
- Mint: Uses
The unprotected CallContract usages found in the codebase are either in test files or in the base implementation itself, which is expected. The ERC20-specific operations are all properly protected against recursive calls through the EIP-150 gas limiting mechanism.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining unprotected contract calls
# Look for CallContract invocations that might not use getCallGasWithLimit
echo "Checking for potentially unprotected contract calls..."
rg "CallContract\(" --type go | while read -r line; do
if ! rg -C 5 "getCallGasWithLimit" "${line%%:*}" > /dev/null; then
echo "Potential unprotected call in: $line"
fi
done
# Look for any other ERC20-related files that might need similar protection
echo "Checking for other ERC20-related files..."
fd -e go -e sol | rg -l "(?i)erc20"
Length of output: 10677
Script:
#!/bin/bash
# Check the context of CallContract usage in non-test files
echo "Checking CallContract usage in non-test files..."
rg "CallContract\(" --type go -g '!*test*' -C 10
echo -e "\nChecking if any other keeper methods directly use CallContract..."
rg "func.*Keeper.*CallContract" --type go -g '!*test*'
echo -e "\nChecking for gas limit usage in non-ERC20 contract calls..."
rg "GasLimit.*=|gas.*:" --type go -g '!*test*'
Length of output: 19339
Summary by CodeRabbit
New Features
TestInfiniteRecursionERC20, which simulates ERC20 behavior with recursive functions.Bug Fixes
Tests
TestFunTokenInfiniteRecursionERC20scenario.