fix(evm): erc20 born funtoken: properly burn bank coins after convert… #2139
Conversation
…ing coin back to erc20
|
Warning Rate limit exceeded@Unique-Divine has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces updates to the Nibiru EVM project, focusing on a bug fix for ERC20 token burning and the addition of a new test contract. It includes a changelog entry documenting a fix related to the ERC20 born funtoken, ensuring proper burning of bank coins. A new Solidity contract, Changes
Sequence DiagramsequenceDiagram
participant User
participant ERC20Contract
participant Bank
participant EVMKeeper
User->>ERC20Contract: Transfer tokens
ERC20Contract->>ERC20Contract: Calculate and deduct fee
ERC20Contract->>Bank: Send remaining tokens
Bank->>EVMKeeper: Convert tokens
EVMKeeper->>Bank: Burn converted coins
EVMKeeper-->>User: Confirm conversion
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
x/evm/embeds/contracts/TestERC20TransferWithFee.sol (1)
14-25: Fee Deduction LogicThis logic successfully deducts 10% and transfers it to the contract. Check for potential corner cases (e.g., dust amounts, large integer overflows, or fee distribution beyond the contract).
If needed, consider parameterizing the fee or distributing it to a designated feeCollector address.
- uint256 constant FEE_PERCENTAGE = 10; + uint256 public feePercentage = 10; ... - _transfer(owner, address(this), fee); + _transfer(owner, feeCollector, fee);x/evm/keeper/funtoken_from_erc20_test.go (4)
455-457: Add descriptive comments for newly introduced test method.
It might be beneficial to add a concise description or docstring above the test method, clarifying that this test verifies handling of transfer fees, ERC20-Bank conversions, and burned coins.
460-470: Consider renaming the log message or grouping logs.
To ensure clarity when scanning test logs, rename or group repeated logs (e.g. “Deploy ERC20”) if necessary, or provide more details about the contract features (fee structure) being deployed.
479-488: Ensure test coverage for all error paths of CreateFunToken.
For example, consider adding negative tests where the contract is not fee-enabled or the user lacks enough funds.
491-495: Refine logging or verify random account usage.
Ensure that the random account is both funded (if needed) and consistently used across the test. If it’s only used to receive tokens, clarifying comments could help maintain the test.x/evm/keeper/msg_server.go (1)
606-607: Clarify the explanatory comment.
The statement is helpful, but adding an explicit mention that this pre-escrow design ensures re-conversions remain consistent may further clarify the rationale.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.md(1 hunks)x/evm/const.go(2 hunks)x/evm/embeds/artifacts/contracts/TestERC20TransferWithFee.sol/TestERC20TransferWithFee.json(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_test.go(1 hunks)x/evm/keeper/msg_server.go(3 hunks)
🔇 Additional comments (21)
x/evm/embeds/embeds_test.go (1)
23-23: Ensure Additional Coverage for Fee MechanismsAdding the
SmartContract_TestERC20TransferWithFee.MustLoad()call is good, but please confirm the associated fee logic is covered through relevant test cases. If not, add or expand test coverage to ensure the contract’s fee behavior is verified.x/evm/embeds/contracts/TestERC20TransferWithFee.sol (3)
1-2: License and Compatibility AdvisoryGreat to see the SPDX license identifier. Confirm that the chosen license is compatible with other project dependencies and that all relevant requirements are satisfied.
4-5: OpenZeppelin Import CheckImporting
@openzeppelin/contracts/token/ERC20/ERC20.solis standard. Verify that the project is using the correct version of OpenZeppelin for your Solidity compiler version.
6-13: Constructor InitializationThe constructor properly sets up the token name, symbol, and mints 1000 tokens to the deployer. However, ensure that 1000 tokens is sufficient for tests or real usage, or parameterize if needed.
x/evm/const.go (3)
9-9: Import Usage ValidationYou’ve introduced the Cosmos SDK
typespackage (aliased assdk), but confirm that it’s needed for the new code or used consistently elsewhere.
88-88: Separate Module Address VariablesDefining both
EVM_MODULE_ADDRESSandEVM_MODULE_ADDRESS_NIBIcan help maintain clarity. Verify all references toEVM_MODULE_ADDRESSfully account for this new variable, especially if existing logic depends on the original address.
91-92: Initialization OrderThe initialization sets
EVM_MODULE_ADDRESS_NIBIfirst, thenEVM_MODULE_ADDRESS. This is correct. Just ensure no downstream usage occurs before these are fully set.x/evm/embeds/embeds.go (3)
42-43: Embedded JSON DeclarationThe embedded JSON for
TestERC20TransferWithFeeis introduced. Ensure the artifact is correctly generated and up to date with the Solidity contract’s bytecode and ABIs.
131-136: New SmartContract InstanceDefining
SmartContract_TestERC20TransferWithFeeis a good approach. Confirm the contract name, file path, and JSON data reference match exactly to avoid initialization errors.
152-152: Initialization SequenceLoading the new contract in
init()ensures it’s available at runtime. Verify that the order of.MustLoad()calls meets the dependencies for any existing or future tests, especially if fee-collecting logic depends on other contracts being loaded first.x/evm/keeper/funtoken_from_erc20_test.go (6)
458-459: No concerns found.
Initialization and environment setup viaevmtest.NewTestDeps()is standard practice and appears correct.
471-478: Check for consistent error handling after funding the account.
The code funds the user’s account and then callsCreateFunToken. If any additional steps require user funds, ensure that exceptions or failures are consistently handled/logged.
489-490: No concerns found.
ThebankDemonassignment looks straightforward, and naming is consistent with the rest of the test.
496-508: Good coverage of ERC20 → Bank flow.
The flow checks final ERC20 and bank balances thoroughly. This is comprehensive and ensures the 10% transfer fee logic is validated. No issues detected.
510-515: Thorough final checks.
Verifying module address, user address, and random account address balances after the ERC20→Bank conversion is robust. Looks good.
516-536: Comprehensive coverage of the Bank → ERC20 flow.
This section verifies that coins are burned after conversion, ensuring that supply consistency is maintained. The tests look correct.x/evm/keeper/msg_server.go (3)
613-613: Ensure inline references remain up to date.
Check that the reference to the preceding ERC20→BC conversion step is accurate if the relevant code shifts or is refactored.
628-628: Appropriate coin burning logic.
The burn step is crucial for supply consistency. It is well-placed to ensure bank coin supply remains sync’d to the ERC20 supply.
638-638: No concerns for event emission.
This event usage for conversion is consistent with the existing pattern. Looks correct and transparent for auditing.x/evm/embeds/artifacts/contracts/TestERC20TransferWithFee.sol/TestERC20TransferWithFee.json (1)
1-297: ABI definitions look standard.
No security or correctness issues found in the ABI. Consider documenting any special constructor arguments or fee logic in a separate doc to help integrators.CHANGELOG.md (1)
57-57: Good practice adding the PR reference.
This clarifies the fix related to ERC20 FunToken burn logic. No concerns.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2139 +/- ##
=======================================
Coverage 65.13% 65.13%
=======================================
Files 277 277
Lines 22167 22168 +1
=======================================
+ Hits 14439 14440 +1
Misses 6738 6738
Partials 990 990
|
There was a problem hiding this comment.
This looks like the re-introduction of a bug fixed in the first audit. Please explain further why the sent amount is being ignored now
There was a problem hiding this comment.
All this is related to some weird or malicious contracts which do not transfer 1:1 of the tokens they are intended to transfer.
The most realistic case which I can imagine is that part of the tokens is transferred as a fee to another (non-recipient) account. So, the total amount of ERC20 tokens remains the same but the actualSentAmount in this case is lower. If we do not burn the full amount in this case - we have an imbalance with extra coins accumulated on a module account.
There was a problem hiding this comment.
Here's my take on the issue here: https://github.com/code-423n4/2024-11-nibiru-findings/issues/48#issuecomment-2573453351
There was a problem hiding this comment.
@Unique-Divine I think we should burn them (approve this PR). For reference, this types of transfer function are usually either:
- Burned (sent to a dead address).
- Reflected (distributed to existing holders).
- Routed to a Marketing/Dev Wallet (for project funding).
- Used for Liquidity (auto-liquidity functions).
- Any combination of the above (split fees).
In any of these cases, all of the tokens are expected to be used by the function and derived to wallets, so we can burn all the amount.
As for the previous fix, it was related to this code but not exactly the same. We use to check that input in = input sent to the transferee wallet, which is not correct considering these weird fee-on-transfer tokens, and that's why we made it more lax and don't check this anymore.
Summary by CodeRabbit
Bug Fixes
New Features
Documentation