fix(evm): improve safety of ERC20 transfers, accounting for boolean success return values and recipient balance changes that don't match the ERC20 transfer amount. #2090
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several updates to the Nibiru project, primarily focusing on enhancing ERC20 contract interactions and improving the handling of token transfers. Key changes include modifications to the Changes
Assessment against linked issues
Possibly related PRs
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 (
|
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- x/evm/keeper/msg_server.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
CHANGELOG.md (1)
138-138: LGTM! The changelog entry is well-formatted and accurately describes the fix.The entry properly documents the fix for native to ERC20 token transfers, follows the changelog format guidelines, and aligns with the PR objectives of addressing non-standard ERC20 token handling.
x/evm/keeper/msg_server.go
Outdated
| return nil, errors.Wrap(err, "failed to retrieve EVM module account balance") | ||
| } | ||
| if recipientBalanceBefore == nil { | ||
| return nil, fmt.Errorf("failed to retrieve balance, balance is nil") | ||
| return nil, fmt.Errorf("failed to retrieve EVM module account balance, balance is nil") |
There was a problem hiding this comment.
Correct error messages to reflect the recipient's balance
The error messages at lines 595 and 598 incorrectly refer to the "EVM module account balance" when they should reference the "recipient's balance" to accurately reflect the operation being performed.
Apply this diff to correct the error messages:
if err != nil {
- return nil, errors.Wrap(err, "failed to retrieve EVM module account balance")
+ return nil, errors.Wrap(err, "failed to retrieve recipient balance before transfer")
}
if recipientBalanceBefore == nil {
- return nil, fmt.Errorf("failed to retrieve EVM module account balance, balance is nil")
+ return nil, fmt.Errorf("failed to retrieve recipient balance before transfer, balance is 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.
| return nil, errors.Wrap(err, "failed to retrieve EVM module account balance") | |
| } | |
| if recipientBalanceBefore == nil { | |
| return nil, fmt.Errorf("failed to retrieve balance, balance is nil") | |
| return nil, fmt.Errorf("failed to retrieve EVM module account balance, balance is nil") | |
| return nil, errors.Wrap(err, "failed to retrieve recipient balance before transfer") | |
| } | |
| if recipientBalanceBefore == nil { | |
| return nil, fmt.Errorf("failed to retrieve recipient balance before transfer, balance is nil") |
There was a problem hiding this comment.
I agree with Code Rabbit - it's not a module account balance.
There was a problem hiding this comment.
@onikonychev, glad we agree! Correcting the error messages to reflect the recipient's balance will improve clarity.
(^‿^)/
x/evm/keeper/msg_server.go
Outdated
| actualReceivedAmount := big.NewInt(0).Sub(recipientBalanceAfter, recipientBalanceBefore) | ||
| if actualReceivedAmount.Sign() == 0 { | ||
| return nil, fmt.Errorf("no ERC20 tokens were received by the recipient") |
There was a problem hiding this comment.
Ensure actualReceivedAmount is positive to handle negative values
The current check only verifies if actualReceivedAmount is zero. If the value is negative, the code may proceed incorrectly. Update the condition to check if actualReceivedAmount is less than or equal to zero to handle both zero and negative values.
Apply this diff to update the condition:
if actualReceivedAmount.Sign() == 0 {
+ return nil, fmt.Errorf("no ERC20 tokens were received by the recipient")
+}
+if actualReceivedAmount.Sign() < 0 {
return nil, fmt.Errorf("received amount is negative, possible balance inconsistency")
}Committable suggestion was skipped due to low confidence.
x/evm/keeper/msg_server.go
Outdated
| return nil, errors.Wrap(err, "failed to retrieve EVM module account balance") | ||
| } | ||
| if recipientBalanceBefore == nil { | ||
| return nil, fmt.Errorf("failed to retrieve balance, balance is nil") | ||
| return nil, fmt.Errorf("failed to retrieve EVM module account balance, balance is nil") |
There was a problem hiding this comment.
I agree with Code Rabbit - it's not a module account balance.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2090 +/- ##
==========================================
+ Coverage 64.36% 64.52% +0.15%
==========================================
Files 270 271 +1
Lines 21192 21235 +43
==========================================
+ Hits 13640 13701 +61
+ Misses 6603 6584 -19
- Partials 949 950 +1
|
x/evm/keeper/msg_server.go
Outdated
| return nil, errors.Wrap(err, "failed to retrieve recipient balance") | ||
| } | ||
| if recipientBalanceBefore == nil { | ||
| return nil, fmt.Errorf("failed to retrieve balance, balance is nil") | ||
| return nil, fmt.Errorf("failed to retrieve recipient balance, balance is nil") |
There was a problem hiding this comment.
Since we only intend for ERC20 transfers to properly send an amount that alters the recipient balance, let's put the balanceOf recipient checks before and after to inside of ERC20().Transfer. That'll make using it more safe.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
x/evm/keeper/erc20_test.go (1)
Line range hint
1-63: Add documentation for thereceivedparameterConsider adding comments explaining:
- The purpose of the new
receivedparameter in transfer operations- How it helps handle non-standard ERC20 implementations
- Its role in accurate balance accounting
This documentation would help other developers understand the changes and their relationship to the audit findings.
x/evm/evmmodule/genesis_test.go (1)
Line range hint
102-104: Update balance verification for fee-on-transfer tokens.The balance verification assumes the exact transfer amounts, which may not hold true for fee-on-transfer tokens where the received amount could be less than the sent amount.
Consider storing the actual received amounts during transfers and using those for verification:
+ // Store actual received amounts during transfers + var ( + actualReceivedA *big.Int + actualReceivedB *big.Int + ) + _, err, actualReceivedA = deps.EvmKeeper.ERC20().Transfer(...) + _, err, actualReceivedB = deps.EvmKeeper.ERC20().Transfer(...) // Verify erc20 balances for users A, B and sender balance, err := deps.EvmKeeper.ERC20().BalanceOf(erc20Addr, toUserA, deps.Ctx) s.Require().NoError(err) - s.Require().Equal(amountToSendA, balance) + s.Require().Equal(actualReceivedA, balance) balance, err = deps.EvmKeeper.ERC20().BalanceOf(erc20Addr, toUserB, deps.Ctx) s.Require().NoError(err) - s.Require().Equal(amountToSendB, balance) + s.Require().Equal(actualReceivedB, balance)Also applies to: 107-109
x/evm/keeper/erc20.go (3)
83-91: Enhance error messages for better debuggingWhile the error handling is correct, consider adding more context to error messages to aid in debugging transfer failures.
- return false, fmt.Errorf("failed to pack ABI args: %w", err), received + return false, fmt.Errorf("failed to pack transfer args for contract %s: %w", contract.Hex(), err), received
109-122: LGTM: Comprehensive handling of non-standard ERC20 implementationsThe implementation successfully handles both standard ERC20 tokens that return boolean values and non-standard ones that don't. Consider adding debug logging for better observability.
if err == nil { + k.Logger(ctx).Debug("ERC20 transfer returned success value", "success", erc20Bool.Value) success = erc20Bool.Value return success, nil, received } + k.Logger(ctx).Debug("ERC20 transfer did not return value, falling back to balance check") success = true return
76-122: Excellent architectural solution for non-standard ERC20 tokensThe implementation provides a robust solution for handling non-standard ERC20 tokens by:
- Using balance tracking instead of relying on transfer return values
- Supporting fee-on-transfer tokens through actual amount verification
- Maintaining compatibility with both standard and non-standard implementations
- Providing clear error handling and validation
This approach aligns well with real-world ERC20 token implementations and provides better reliability for token transfers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- x/evm/evmmodule/genesis_test.go (1 hunks)
- x/evm/keeper/erc20.go (1 hunks)
- x/evm/keeper/erc20_test.go (1 hunks)
- x/evm/keeper/msg_server.go (1 hunks)
- x/evm/precompile/funtoken.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/evm/keeper/msg_server.go
🔇 Additional comments (5)
x/evm/keeper/erc20_test.go (2)
34-39: LGTM! Good test coverage for failed transfersThe addition of the
receivedamount verification enhances the test by ensuring that failed transfers result in zero tokens received, which aligns well with the PR's objective of accurate balance tracking.
44-48: 🛠️ Refactor suggestionAdd test cases for non-standard ERC20 tokens
While the current test verifies basic transfer functionality, consider adding test cases for:
- Fee-on-transfer tokens where
receivedamount is less than transfer amount- ERC20 tokens that don't return values from transfer functions
This would ensure complete coverage of the scenarios mentioned in the PR objectives.
Here's a suggested test structure:
s.T().Log("Transfer - Fee on transfer token") { // Mock a fee-on-transfer token where 10% is deducted transferAmount := big.NewInt(1000) expectedReceived := big.NewInt(900) // 10% fee _, err, received := deps.EvmKeeper.ERC20().Transfer( feeToken.Address, evm.EVM_MODULE_ADDRESS, deps.Sender.EthAddr, transferAmount, deps.Ctx, ) s.Require().NoError(err) s.Require().Equal(received, expectedReceived) }Let's verify if there are any existing test cases for these scenarios:
x/evm/keeper/erc20.go (3)
76-78: LGTM: Method signature enhancement for fee-on-transfer supportThe addition of
received *big.Intto the return values enables tracking of actual received amounts, which is crucial for handling fee-on-transfer tokens.
79-82: LGTM: Pre-transfer balance tracking implementationThe implementation correctly captures the recipient's initial balance, which is necessary for calculating the actual received amount after transfer.
93-104: LGTM: Robust implementation for fee-on-transfer tokensThe implementation correctly:
- Tracks post-transfer balance
- Calculates actual received amount
- Validates that tokens were actually received
- Handles fee-on-transfer tokens by not assuming received = amount
The comments effectively explain the reasoning behind not checking for exact amounts.
x/evm/evmmodule/genesis_test.go
Outdated
| @@ -54,11 +54,11 @@ func (s *Suite) TestExportInitGenesis() { | |||
| s.Require().NoError(err) | |||
|
|
|||
| // Transfer ERC-20 tokens to user A | |||
| _, err = deps.EvmKeeper.ERC20().Transfer(erc20Addr, fromUser, toUserA, amountToSendA, deps.Ctx) | |||
| _, err, _ = deps.EvmKeeper.ERC20().Transfer(erc20Addr, fromUser, toUserA, amountToSendA, deps.Ctx) | |||
There was a problem hiding this comment.
Update transfer result handling for fee-on-transfer tokens.
The test ignores the actual received amount (third return value) from Transfer(). For fee-on-transfer tokens, the received amount could be less than the sent amount, which this test wouldn't catch.
Consider updating the test to handle fee-on-transfer scenarios:
- _, err, _ = deps.EvmKeeper.ERC20().Transfer(erc20Addr, fromUser, toUserA, amountToSendA, deps.Ctx)
+ _, err, receivedAmount := deps.EvmKeeper.ERC20().Transfer(erc20Addr, fromUser, toUserA, amountToSendA, deps.Ctx)
s.Require().NoError(err)
+ // For fee-on-transfer tokens, verify actual received amount
+ balance, err := deps.EvmKeeper.ERC20().BalanceOf(erc20Addr, toUserA, deps.Ctx)
+ s.Require().NoError(err)
+ s.Require().Equal(receivedAmount, balance)Also applies to: 61-61
x/evm/precompile/funtoken.go
Outdated
| @@ -159,7 +159,7 @@ func (p precompileFunToken) bankSend( | |||
|
|
|||
| // Caller transfers ERC20 to the EVM account | |||
| transferTo := evm.EVM_MODULE_ADDRESS | |||
| _, err = p.evmKeeper.ERC20().Transfer(erc20, caller, transferTo, amount, ctx) | |||
| _, err, _ = p.evmKeeper.ERC20().Transfer(erc20, caller, transferTo, amount, ctx) | |||
There was a problem hiding this comment.
Handle the Received Amount from Transfer Method
At line 162:
_, err, _ = p.evmKeeper.ERC20().Transfer(erc20, caller, transferTo, amount, ctx)The Transfer method now returns an additional value received *big.Int, which represents the actual amount transferred. Ignoring this value may lead to incorrect accounting, especially for tokens with fee-on-transfer mechanisms where the received amount could be less than the requested amount.
Consider capturing and utilizing the received amount to ensure accurate processing:
-receivedAmount, err, _ := p.evmKeeper.ERC20().Transfer(erc20, caller, transferTo, amount, ctx)
+receivedAmount, err, _ := p.evmKeeper.ERC20().Transfer(erc20, caller, transferTo, amount, ctx)
if err != nil {
return nil, fmt.Errorf("failed to send from caller to the EVM account: %w", err)
}
+// Update the amount to reflect the actual received amount
+amount = receivedAmountBy handling receivedAmount, you ensure that subsequent operations use the correct token amount, which is crucial for maintaining consistency and correctness in transactions involving tokens with special transfer behaviors.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
x/evm/keeper/erc20.go (2)
76-78: Update function documentation to reflect new return value.The method signature has been updated to return the actual received amount, which is great for handling fee-on-transfer tokens. However, the function documentation needs to be updated to explain the new
receivedreturn value and its significance.Add this to the function documentation:
/* Transfer implements "ERC20.transfer" ```solidity /// @dev Moves `amount` tokens from the caller's account to `to`. /// Returns a boolean value indicating whether the operation succeeded. /// Emits a {Transfer} event. function transfer(address to, uint256 amount) external returns (bool); ``` + + @return success - boolean indicating if the transfer was successful + @return err - error if any occurred during the transfer + @return received - actual amount of tokens received by the recipient, which may be less than + the transfer amount for fee-on-transfer tokens */
109-118: Robust handling of non-standard ERC20 implementations but needs test coverage.The implementation correctly handles both standard and non-standard ERC20 tokens:
- For standard tokens: Uses the contract's return value
- For non-standard tokens: Falls back to checking actual token receipt
However, the fallback path (lines 121-122) lacks test coverage. Consider adding test cases for:
- Standard ERC20 tokens that return true/false
- Non-standard tokens that don't return a value
- Tokens that return invalid data
Would you like me to help generate comprehensive test cases for these scenarios?
Also applies to: 121-122
x/evm/keeper/msg_server.go (2)
593-593: Increase test coverage for critical error handling pathsThe line added at
593handles a critical error case but is not covered by tests. Consider adding unit tests to cover this error path to enhance reliability.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 593-593: x/evm/keeper/msg_server.go#L593
Added line #L593 was not covered by tests
578-578: Increase test coverage for nil balance error caseThe error handling at line
578for anilbalance is not covered by tests. Adding unit tests for this scenario will help ensure proper error handling.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 578-578: x/evm/keeper/msg_server.go#L578
Added line #L578 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- x/evm/keeper/erc20.go (1 hunks)
- x/evm/keeper/erc20_test.go (1 hunks)
- x/evm/keeper/msg_server.go (1 hunks)
- x/evm/precompile/funtoken.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/evm/keeper/erc20_test.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
x/evm/keeper/erc20.go
[warning] 81-82: x/evm/keeper/erc20.go#L81-L82
Added lines #L81 - L82 were not covered by tests
[warning] 86-86: x/evm/keeper/erc20.go#L86
Added line #L86 was not covered by tests
[warning] 95-96: x/evm/keeper/erc20.go#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 104-104: x/evm/keeper/erc20.go#L104
Added line #L104 was not covered by tests
[warning] 121-122: x/evm/keeper/erc20.go#L121-L122
Added lines #L121 - L122 were not covered by testsx/evm/keeper/msg_server.go
[warning] 578-578: x/evm/keeper/msg_server.go#L578
Added line #L578 was not covered by tests
[warning] 593-593: x/evm/keeper/msg_server.go#L593
Added line #L593 was not covered by tests
🔇 Additional comments (3)
x/evm/precompile/funtoken.go (1)
147-147:⚠️ Potential issueCRITICAL: Security vulnerability in fee-on-transfer token handling
The current implementation ignores the actual received amount from the
Transfermethod, which creates a critical vulnerability when handling fee-on-transfer tokens. This could lead to token inflation as the system would mint more bank coins than the actual ERC20 tokens received.Example scenario:
- User transfers 100 tokens of a fee-on-transfer token
- Due to the fee mechanism, only 95 tokens are actually received
- Current code mints 100 bank coins instead of 95
- This creates 5 extra bank coins, breaking the 1:1 peg
Apply this fix to properly handle the received amount and add error handling:
-_, err, _ = p.evmKeeper.ERC20().Transfer(erc20, caller, transferTo, amount, ctx) +receivedAmount, err, success := p.evmKeeper.ERC20().Transfer(erc20, caller, transferTo, amount, ctx) if err != nil { return nil, fmt.Errorf("failed to send from caller to the EVM account: %w", err) } +if !success { + return nil, fmt.Errorf("transfer failed: ERC20 contract returned false") +} +if receivedAmount == nil { + return nil, fmt.Errorf("transfer failed: received amount is nil") +} +if receivedAmount.Cmp(amount) < 0 { + // Update amount to the actual received amount for fee-on-transfer tokens + amount = receivedAmount +}Run this verification script to identify potential fee-on-transfer tokens in the system:
x/evm/keeper/msg_server.go (2)
578-578: Previous comment still applies: Correct the error message to reflect the recipient's balanceThe error message refers to the "EVM module account balance," but it should reference the "recipient's balance" to accurately reflect the operation being performed.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 578-578: x/evm/keeper/msg_server.go#L578
Added line #L578 was not covered by tests
596-597: Previous comment still applies: EnsureactualReceivedAmountis positiveIt's important to check that
actualReceivedAmountis greater than zero to prevent potential issues with zero or negative amounts during the burn operation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
x/evm/keeper/erc20.go (3)
79-82: Enhance error context for balance retrieval failures
While the error handling is good, consider adding more context about why the balance check failed, as it could help diagnose issues with non-standard tokens.
- return false, errors.Wrap(err, "failed to retrieve recipient balance"), received
+ return false, errors.Wrapf(err, "failed to retrieve recipient balance for address %s", to.String()), received109-122: Add logging for non-standard token behavior
The fallback mechanism for non-standard tokens is good, but consider adding debug logs when encountering such tokens to help with monitoring and debugging.
if err == nil {
// should be true if the transfer was successful but we do it anyway
// to respect the contract's return value
success = erc20Bool.Value
return success, nil, received
}
+ // Log non-standard token behavior
+ k.Logger(ctx).Debug(
+ "encountered non-standard ERC20 token",
+ "contract", contract.String(),
+ "error", err.Error(),
+ )
success = true
return76-122: Add test coverage for fee-on-transfer scenarios
The implementation needs test coverage for:
- Fee-on-transfer tokens where received amount < sent amount
- Tokens with non-standard return values
- Edge cases in balance calculation
Would you like me to help create comprehensive test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- CHANGELOG.md (2 hunks)
- x/evm/keeper/erc20.go (1 hunks)
- x/evm/keeper/msg_server.go (1 hunks)
- x/evm/precompile/funtoken.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- x/evm/precompile/funtoken.go
🔇 Additional comments (5)
x/evm/keeper/erc20.go (3)
76-78: LGTM: Method signature enhancement for fee-on-transfer support
The addition of received *big.Int as a return value enables proper tracking of actual received tokens, which is crucial for fee-on-transfer tokens.
86-91: LGTM: Consistent error handling
The error handling maintains a consistent pattern of returning the received amount even in error cases.
93-104: Verify balance calculation safety
While the balance difference calculation is logically correct, consider:
- Using
SafeSubto prevent integer overflow - The possibility of concurrent transfers affecting the balance
Let's check if there are any existing safety measures:
x/evm/keeper/msg_server.go (2)
592-593: Handle nil error correctly when transfer fails
If err is nil but success is false, using errors.Wrap(err, ...) with a nil error may result in an unintended error message. Update the error handling to correctly construct the error message when err is nil.
578-578: Correct error message to reflect the EVM module account balance
The error message refers to the "EVM module account balance" when retrieving the module's balance. Ensure this accurately reflects the account involved in the operation.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
x/evm/embeds/artifacts/contracts/IFunToken.sol/IFunToken.json (1)
25-31: Excellent addition of sentAmount return value for handling non-standard tokens!
This change is crucial for properly handling "Fee on Transfer" tokens where the actual amount received differs from the amount sent. By returning the sentAmount, the contract can now accurately track token transfers and maintain correct balance accounting.
Consider documenting in the interface comments that:
- For standard ERC20 tokens,
sentAmountwill equal the inputamount - For fee-on-transfer tokens,
sentAmountmay be less than the inputamount - This return value should be used for balance tracking instead of the input amount
x/evm/embeds/contracts/IFunToken.sol (1)
8-14: LGTM! Consider adding examples for clarity.
The documentation clearly explains the parameters and the return value, particularly highlighting the potential difference between sent and received amounts for fee-on-transfer tokens.
Consider adding a documentation example to illustrate the fee-on-transfer scenario:
/// @return sentAmount - amount of tokens received by the recipient. This may
/// not be equal to `amount` if the corresponding ERC20 contract has a fee or
/// deduction on transfer.
+ /// @dev Example: If amount=100 and token has 2% transfer fee, sentAmount=98x/evm/keeper/erc20_test.go (1)
Line range hint 1-56: Consider restructuring tests for better isolation
The current test structure combines multiple scenarios in a single test function. This makes it harder to understand failures and maintain the tests.
Consider these improvements:
- Split into separate test functions:
func (s *Suite) TestERC20Transfer_InsufficientFunds() {}
func (s *Suite) TestERC20Transfer_Success() {}
func (s *Suite) TestERC20Transfer_FeeOnTransfer() {}
func (s *Suite) TestERC20Transfer_NoReturnValue() {}
func (s *Suite) TestERC20Transfer_FalseReturn() {}- Add helper functions for token deployment:
func (s *Suite) deployMockToken(opts MockTokenOpts) *evmtest.FunToken {
// Configure token behavior (fees, return values, etc.)
}
type MockTokenOpts struct {
HasFeeOnTransfer bool
FeePercentage uint8
ReturnsValue bool
AlwaysReturns bool
}x/evm/keeper/erc20.go (1)
111-119: Consider improving error message formatting.
The implementation correctly handles fee-on-transfer tokens by tracking actual balance changes. However, the error message could be more readable.
- "amount of ERC20 tokens received MUST be positive: the balance of recipient %s would've changed by %v for token %s",
- to.Hex(), balanceIncrease.String(), contract.Hex(),
+ "amount of ERC20 tokens received MUST be positive: recipient=%s received_amount=%v token=%s",
+ to.Hex(), balanceIncrease.String(), contract.Hex(),📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- CHANGELOG.md (1 hunks)
- x/evm/embeds/artifacts/contracts/IFunToken.sol/IFunToken.json (1 hunks)
- x/evm/embeds/artifacts/contracts/TestERC20TransferThenPrecompileSend.sol/TestERC20TransferThenPrecompileSend.json (1 hunks)
- x/evm/embeds/artifacts/contracts/TestNativeSendThenPrecompileSend.sol/TestNativeSendThenPrecompileSend.json (1 hunks)
- x/evm/embeds/contracts/IFunToken.sol (1 hunks)
- x/evm/keeper/erc20.go (1 hunks)
- x/evm/keeper/erc20_test.go (2 hunks)
- x/evm/keeper/msg_server.go (3 hunks)
- x/evm/precompile/funtoken.go (2 hunks)
- x/evm/precompile/funtoken_test.go (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- x/evm/embeds/artifacts/contracts/TestERC20TransferThenPrecompileSend.sol/TestERC20TransferThenPrecompileSend.json
- x/evm/embeds/artifacts/contracts/TestNativeSendThenPrecompileSend.sol/TestNativeSendThenPrecompileSend.json
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- x/evm/precompile/funtoken.go
🔇 Additional comments (9)
x/evm/embeds/contracts/IFunToken.sol (1)
15-19: LGTM! Verify implementation in precompile.
The addition of sentAmount return value is a crucial enhancement for handling fee-on-transfer tokens correctly. This change aligns perfectly with the PR objectives and maintains backward compatibility.
Let's verify the precompile implementation handles this new return value:
✅ Verification successful
The initial search revealed the precompile exists but we need to verify:
- If the implementation returns the actual sent amount
- If there are tests covering this functionality
The new search will help us see the complete implementation and any test coverage.
The previous search showed the beginning of the bankSend implementation and some test references, but we need to see:
- The complete implementation to verify if it returns the actual sent amount
- The test contract implementation to understand how the return value is handled in tests
We need to see:
- The rest of the bankSend implementation, particularly how it handles the return value after validating the address
- The location of the test contract to verify how it handles the return value
The previous search was cut off at the address validation part, and we couldn't find the test contract in the expected location.
Implementation correctly handles fee-on-transfer tokens and returns actual sent amount
The precompile implementation properly handles fee-on-transfer tokens by:
- Capturing the actual transferred amount via
gotAmountfromERC20().Transfer() - Using this
gotAmountfor subsequent operations (minting/burning) - Correctly returning the actual sent amount via
method.Outputs.Pack(gotAmount)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the precompile implementation matches the interface changes
# Test 1: Check if the precompile code handles the new return value
rg -A 10 "func.*[Bb]ank[Ss]end.*"
# Test 2: Look for related test cases
rg -A 10 "TestBankSend.*fee.*transfer"
Length of output: 2063
Script:
#!/bin/bash
# Let's check the implementation details of bankSend in the precompile
rg -B 5 -A 20 "func \(p precompileFunToken\) bankSend" x/evm/precompile/funtoken.go
# Also check if there are any test files related to bankSend
fd ".*test.*\.go$" | xargs rg "bankSend"
Length of output: 1692
Script:
#!/bin/bash
# Let's see the complete implementation of bankSend and its return value handling
rg -B 20 -A 40 "func \(p precompileFunToken\) bankSend" x/evm/precompile/funtoken.go
# Also check the test contract implementation to see how it handles the return value
rg -A 20 "contract.*TestContract" x/evm/keeper/testdata/contracts/
Length of output: 1963
Script:
#!/bin/bash
# Let's see the complete implementation by searching more lines after the last visible part
rg -A 60 "toAddr, err := sdk.AccAddressFromBech32\(to\)" x/evm/precompile/funtoken.go
# Also try to find the test contract in a different location
fd -t f "TestContract.sol"
Length of output: 2022
x/evm/precompile/funtoken_test.go (3)
129-136: LGTM! Improved variable naming.
The variable rename from resp to ethTxResp enhances code readability by better describing the response type from the EVM transaction.
138-143: LGTM! Improved test readability.
The multi-line formatting of balance assertions enhances readability while maintaining the critical checks for token balance accuracy.
148-156: 🛠️ Refactor suggestion
Add test cases for fee-on-transfer tokens.
While the new validation is good, consider adding test cases that verify the handling of fee-on-transfer tokens where the received amount differs from the sent amount. This would better align with the PR objectives of handling non-standard ERC20 tokens.
Example test case structure:
s.T().Log("Test fee-on-transfer token")
{
// Mock a fee-on-transfer token where recipient receives 90% of sent amount
amtToSend := int64(1000)
expectedReceived := int64(900) // 90% of sent amount
// ... perform transfer ...
// Verify actual received amount
var sentAmt *big.Int
err = embeds.SmartContract_FunToken.ABI.UnpackIntoInterface(
&sentAmt,
string(precompile.FunTokenMethod_BankSend),
ethTxResp.Ret,
)
s.NoError(err)
s.Require().Equal(expectedReceived, sentAmt.Int64())
}Let me verify if there are any existing test cases for fee-on-transfer tokens:
x/evm/keeper/erc20.go (3)
77-81: LGTM! Balance tracking implementation is correct.
The implementation properly retrieves and tracks the recipient's balance before the transfer, with appropriate error handling and descriptive error messages.
98-102: LGTM! Improved error handling for non-reverting transfers.
The explicit success check addresses issue #2080, properly handling tokens like ZRX and BAT that return false instead of reverting on failed transfers.
76-77: LGTM! Verify callers of this method.
The signature change from (bool, error) to (*big.Int, error) is a good improvement that allows tracking the actual amount of tokens received, which is crucial for fee-on-transfer tokens.
✅ Verification successful
Let me analyze the callers of the Transfer method to ensure we have a complete picture.
Let me check one more pattern to ensure we haven't missed any direct calls to Transfer.
All callers of Transfer method have been updated correctly
Based on the verification results:
- The only caller that uses the returned balance value is in
precompile/funtoken.go, which correctly captures it asgotAmount - All other callers in test files only check the error return value using
_, err :=pattern, which is appropriate since they don't need the balance change - No instances of incorrect usage patterns were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all callers of the Transfer method to ensure they're updated
# to handle the new return value.
# Search for method calls
rg -B 2 -A 2 "Transfer\s*\([^)]*\)" --type go
Length of output: 8260
Script:
#!/bin/bash
# Let's look at the actual implementation of the Transfer method callers
# to see how they handle the return value
# First, let's check the precompile/funtoken.go implementation
ast-grep --pattern 'gotAmount, err := p.evmKeeper.ERC20().Transfer($$$)'
# Then check the test implementations
ast-grep --pattern '_, err := $$.Transfer($$$)'
ast-grep --pattern 'err := $$.Transfer($$$)'
Length of output: 292
Script:
#!/bin/bash
# Search for any Transfer method calls that might store the return value
# in a variable or use it directly
ast-grep --pattern '$$ := $$.Transfer($$$)'
ast-grep --pattern '$$ = $$.Transfer($$$)'
# Also check for direct usage in if statements or function calls
ast-grep --pattern 'if $$.Transfer($$$)'
ast-grep --pattern '$$.Transfer($$$)'
Length of output: 173
x/evm/keeper/msg_server.go (2)
494-496: LGTM! Clear separation of concerns based on token origin.
The code now correctly routes the conversion based on whether the token originated from a coin or ERC20, improving the handling of different token types.
Line range hint 500-544: LGTM! Well-structured implementation with proper error handling.
The function follows a clear two-step process with proper error handling and event emission. The implementation correctly handles the minting of ERC20 tokens for coin-originated tokens.
| amt := big.NewInt(9_420) | ||
| _, err := deps.EvmKeeper.ERC20().Transfer(contract, deps.Sender.EthAddr, evm.EVM_MODULE_ADDRESS, amt, deps.Ctx) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for fee-on-transfer tokens
While this test covers the basic insufficient funds scenario, we should add test cases for fee-on-transfer tokens where the received amount differs from the sent amount.
Consider adding these test scenarios:
s.T().Log("Transfer - Fee on transfer token")
{
amt := big.NewInt(1000)
expectedReceivedAmt := big.NewInt(950) // Assuming 5% fee
sentAmt, err := deps.EvmKeeper.ERC20().Transfer(
feeToken, // Mock fee-on-transfer token
evm.EVM_MODULE_ADDRESS,
deps.Sender.EthAddr,
amt,
deps.Ctx,
)
s.Require().NoError(err)
s.Require().Equal(sentAmt.String(), expectedReceivedAmt.String())
}| amt := big.NewInt(9_420) | ||
| sentAmt, err := deps.EvmKeeper.ERC20().Transfer( | ||
| contract, evm.EVM_MODULE_ADDRESS, deps.Sender.EthAddr, amt, deps.Ctx, | ||
| ) | ||
| s.Require().NoError(err) | ||
| evmtest.AssertERC20BalanceEqual(s.T(), deps, contract, deps.Sender.EthAddr, big.NewInt(9_420)) | ||
| evmtest.AssertERC20BalanceEqual(s.T(), deps, contract, evm.EVM_MODULE_ADDRESS, big.NewInt(60_000)) | ||
| evmtest.AssertERC20BalanceEqual( | ||
| s.T(), deps, contract, deps.Sender.EthAddr, big.NewInt(9_420)) | ||
| evmtest.AssertERC20BalanceEqual( | ||
| s.T(), deps, contract, evm.EVM_MODULE_ADDRESS, big.NewInt(60_000)) | ||
| s.Require().Equal(sentAmt.String(), amt.String()) |
There was a problem hiding this comment.
Enhance test coverage for non-standard ERC20 tokens
While this test verifies basic transfer functionality, it doesn't cover the specific cases mentioned in issue #2080 and the PR objectives.
- Add test cases for tokens that return
falseinstead of reverting:
s.T().Log("Transfer - Handle false return value")
{
amt := big.NewInt(1000)
sentAmt, err := deps.EvmKeeper.ERC20().Transfer(
falseReturningToken, // Mock token that returns false
evm.EVM_MODULE_ADDRESS,
deps.Sender.EthAddr,
amt,
deps.Ctx,
)
s.Require().Error(err) // Should handle false return as error
s.Require().Nil(sentAmt)
}- Add test cases for tokens without return values:
s.T().Log("Transfer - Handle missing return value")
{
amt := big.NewInt(1000)
sentAmt, err := deps.EvmKeeper.ERC20().Transfer(
noReturnToken, // Mock token without return value
evm.EVM_MODULE_ADDRESS,
deps.Sender.EthAddr,
amt,
deps.Ctx,
)
s.Require().NoError(err)
s.Require().Equal(sentAmt.String(), amt.String())
}- Make balance assertions more explicit:
-evmtest.AssertERC20BalanceEqual(
- s.T(), deps, contract, deps.Sender.EthAddr, big.NewInt(9_420))
-evmtest.AssertERC20BalanceEqual(
- s.T(), deps, contract, evm.EVM_MODULE_ADDRESS, big.NewInt(60_000))
+// Initial balance (0) + transferred amount (9_420) = 9_420
+evmtest.AssertERC20BalanceEqual(
+ s.T(), deps, contract, deps.Sender.EthAddr, amt)
+// Initial balance (69_420) - transferred amount (9_420) = 60_000
+evmtest.AssertERC20BalanceEqual(
+ s.T(), deps, contract, evm.EVM_MODULE_ADDRESS,
+ new(big.Int).Sub(big.NewInt(69_420), amt))| burnCoin := sdk.NewCoin(coin.Denom, sdk.NewIntFromBigInt(actualSentAmount)) | ||
| err = k.bankKeeper.BurnCoins(ctx, evm.ModuleName, sdk.NewCoins(burnCoin)) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "failed to burn coins") | ||
| } | ||
|
|
||
| // Emit event with the actual amount received | ||
| _ = ctx.EventManager().EmitTypedEvent(&evm.EventConvertCoinToEvm{ | ||
| Sender: sender.String(), | ||
| Erc20ContractAddress: funTokenMapping.Erc20Addr.String(), | ||
| ToEthAddr: recipient.String(), | ||
| BankCoin: coin, | ||
| BankCoin: burnCoin, |
There was a problem hiding this comment.
Verify actualSentAmount before burning coins.
The code should verify that actualSentAmount is positive before creating and burning coins to handle potential issues with fee-on-transfer tokens.
Apply this diff to add the verification:
+if actualSentAmount.Sign() <= 0 {
+ return nil, fmt.Errorf("invalid actual sent amount: %s", actualSentAmount)
+}
+
burnCoin := sdk.NewCoin(coin.Denom, sdk.NewIntFromBigInt(actualSentAmount))
err = k.bankKeeper.BurnCoins(ctx, evm.ModuleName, sdk.NewCoins(burnCoin))
if err != nil {
return nil, errors.Wrap(err, "failed to burn coins")
}📝 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.
| burnCoin := sdk.NewCoin(coin.Denom, sdk.NewIntFromBigInt(actualSentAmount)) | |
| err = k.bankKeeper.BurnCoins(ctx, evm.ModuleName, sdk.NewCoins(burnCoin)) | |
| if err != nil { | |
| return nil, errors.Wrap(err, "failed to burn coins") | |
| } | |
| // Emit event with the actual amount received | |
| _ = ctx.EventManager().EmitTypedEvent(&evm.EventConvertCoinToEvm{ | |
| Sender: sender.String(), | |
| Erc20ContractAddress: funTokenMapping.Erc20Addr.String(), | |
| ToEthAddr: recipient.String(), | |
| BankCoin: coin, | |
| BankCoin: burnCoin, | |
| if actualSentAmount.Sign() <= 0 { | |
| return nil, fmt.Errorf("invalid actual sent amount: %s", actualSentAmount) | |
| } | |
| burnCoin := sdk.NewCoin(coin.Denom, sdk.NewIntFromBigInt(actualSentAmount)) | |
| err = k.bankKeeper.BurnCoins(ctx, evm.ModuleName, sdk.NewCoins(burnCoin)) | |
| if err != nil { | |
| return nil, errors.Wrap(err, "failed to burn coins") | |
| } | |
| // Emit event with the actual amount received | |
| _ = ctx.EventManager().EmitTypedEvent(&evm.EventConvertCoinToEvm{ | |
| Sender: sender.String(), | |
| Erc20ContractAddress: funTokenMapping.Erc20Addr.String(), | |
| ToEthAddr: recipient.String(), | |
| BankCoin: burnCoin, |
Unique-Divine
changed the title
Purpose / Abstract
Description:
The current implementation lacks support for certain non-standard ERC20 tokens, specifically those with "Fee on Transfer" mechanisms and those that have missing return values in their transfer functions.
According to the project documentation, these tokens should be properly handled by the system. The issue arises because fee-on-transfer tokens deduct a fee during transfer operations, meaning the recipient receives less than the amount specified. The fee is usually burned or redistributed according to the token's smart contract logic.
The existing code assumes however, that the actual transferred amount is equal to the intended transfer amount, so assertions or checks enforcing this equality fail when interacting with fee-on-transfer tokens, leading to transaction failures or incorrect accounting of token balances.
Similarly, some ERC20 tokens do not return a boolean value upon executing transfer or transferFrom functions, even though the ERC20 standard specifies that these functions should return true on success.
In the x/evm/keeper/erc20.go:73, the Transfer function, the code attempts to unpack the return value of the transfer function. If a token does not return any value, this unpacking operation fails, causing the transaction to revert and making it impossible to interact with such tokens through the current implementation.
There is an existing issue documented in #2063 acknowledging that some tokens do not adhere strictly to the ERC20 standard. While tokens that lack fundamental ERC20 functions (like no transfer function) are rightly considered non-ERC20 and can be excluded, tokens with minor deviations like fee-on-transfer or missing return values are prevalent and widely used.
Recommendation:
We recommend that to improve compatibility and user experience, the system should be updated to handle these "weird" ERC20 token behaviors gracefully. For fee-on-transfer tokens, the transfer logic should be adjusted to not assume that the transfer amount equals the received amount. After performing a transfer, the recipient's balance before and after the transfer should be retrieved to calculate the actual amount received, ensuring that any accounting or state updates reflect the actual amount received.
For tokens with missing return values, function calls should be modified to use low-level calls that do not expect a return value, avoiding unpacking the return value when calling the transfer function. After the transfer call, it's important to verify that the transaction did not revert by checking for errors or ensuring the transaction receipt indicates success.
Additionally, documentation and user guidance should be updated to inform users
Summary by CodeRabbit
New Features
debug_traceCallmethod for improved call tracing.bankSendfunction to return the actual amount sent during transfers.Bug Fixes
bankSendmethod.Documentation
CHANGELOG.mdto reflect new features and fixes.Tests