fix(evm): issue with infinite recursion in erc20 funtoken contracts #2129
Conversation
WalkthroughThis pull request addresses an infinite recursion issue in ERC20 FunToken contracts. The changes introduce a new test contract Changes
Sequence DiagramsequenceDiagram
participant Contract as TestInfiniteRecursionERC20
participant EVM as Ethereum Virtual Machine
participant Keeper as EVM Keeper
Contract->>EVM: Deploy Contract
EVM-->>Contract: Deployment Confirmed
Contract->>Keeper: Call attackBalance()
Keeper->>Contract: Validate Gas Limit
Keeper-->>Contract: Execution Allowed
Contract->>EVM: Recursive Balance Check
EVM-->>Contract: Balance Returned
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.
| function transfer(address to, uint256 amount) public override returns (bool) { | ||
| // recurse through funtoken sendToBank | ||
| FUNTOKEN_PRECOMPILE.sendToBank( | ||
| address(this), | ||
| amount, | ||
| "nibi1zaavvzxez0elundtn32qnk9lkm8kmcsz44g7xl" // does not matter, it's not reached | ||
| ); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Risky recursion in transfer override.
This function intentionally triggers a recursive path into FUNTOKEN_PRECOMPILE.sendToBank. Ensure thorough testing to avoid an actual infinite recursion that could lock the contract or run out of gas.
There was a problem hiding this comment.
Yes, the point of this smart contract is to test an infinite recursion scenario.
There was a problem hiding this comment.
@k-yang, understood. Thanks for clarifying that the contract is explicitly designed to test an infinite recursion scenario. We'll keep that context in mind for any future references to this code.
(＊￣ω￣＊)ノ♡ (curious bunny)
✏️ Learnings added
Learnt from: k-yang
PR: NibiruChain/nibiru#2129
File: x/evm/embeds/contracts/TestInfiniteRecursionERC20.sol:24-32
Timestamp: 2024-12-26T04:43:43.402Z
Learning: The `TestInfiniteRecursionERC20` contract is intentionally designed to test infinite recursion scenarios in ERC20-based contracts for the `FunToken` architecture.
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.
| function transfer(address to, uint256 amount) public override returns (bool) { | ||
| // recurse through funtoken sendToBank | ||
| FUNTOKEN_PRECOMPILE.sendToBank( | ||
| address(this), | ||
| amount, | ||
| "nibi1zaavvzxez0elundtn32qnk9lkm8kmcsz44g7xl" // does not matter, it's not reached | ||
| ); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Yes, the point of this smart contract is to test an infinite recursion scenario.
| // protection against recursive calls ERC20 -> precompile -> ERC20. | ||
| func getCallGasWithLimit(ctx sdk.Context, gasLimit uint64) uint64 { | ||
| availableGas := ctx.GasMeter().GasRemaining() | ||
| callGas := (availableGas * 63) / 64 |
There was a problem hiding this comment.
For technical accuracy, let's make this N - floor(N/64).
Reference: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-150.md
Summary by CodeRabbit
New Features
TestInfiniteRecursionERC20, which simulates ERC20 behavior with recursive functions.Bug Fixes
Tests
TestFunTokenInfiniteRecursionERC20scenario.