Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Oct 30, 2024
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9d59336
feat: fix transfer for native to erc20
matthiasmatt Oct 22, 2024
a0afa5a
chore: changelog
matthiasmatt Oct 22, 2024
476ffbc
feat: check transfer occurred by ensuring new balance is higher than …
matthiasmatt Oct 22, 2024
eb72b31
feat: fix the 0 check
matthiasmatt Oct 22, 2024
f356315
fix: fix error message
matthiasmatt Oct 23, 2024
43ae3e1
feat: move transfer logic to erc20.go
matthiasmatt Oct 24, 2024
80d5353
Merge branch 'main' into mat/audit-32
matthiasmatt Oct 28, 2024
3ad846b
Merge branch 'main' into mat/audit-32
Unique-Divine Oct 29, 2024
e4b932d
chore: changelog
Unique-Divine Oct 29, 2024
b2c863d
fix: make the FunToken precompile return the sent amount
Unique-Divine Oct 29, 2024
2235d9d
doc comments
Unique-Divine Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ reflected on all occurences of "base fee" in the codebase. This fixes [#2059](ht
and the [related comments from @Unique-Divine and @berndartmueller](https://github.com/NibiruChain/nibiru/issues/2059#issuecomment-2408625724).
- [#2084](https://github.com/NibiruChain/nibiru/pull/2084) - feat(evm-forge): foundry support and template for Nibiru EVM develoment
- [#2088](https://github.com/NibiruChain/nibiru/pull/2088) - refactor(evm): remove outdated comment and improper error message text
- [#2090](https://github.com/NibiruChain/nibiru/pull/2090) - fix(evm): transfer for native to erc20 for specific tokens


#### Dapp modules: perp, spot, oracle, etc
Expand Down
40 changes: 19 additions & 21 deletions x/evm/keeper/msg_server.go
View file
Open in desktop
Unique-Divine marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -592,10 +592,10 @@ func (k Keeper) convertCoinNativeERC20(

recipientBalanceBefore, err := k.ERC20().BalanceOf(erc20Addr, recipient, ctx)
if err != nil {
return nil, errors.Wrap(err, "failed to retrieve balance")
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")
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Code Rabbit - it's not a module account balance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@onikonychev, glad we agree! Correcting the error messages to reflect the recipient's balance will improve clarity.

(^‿^)/

}

// Escrow Coins on module account
Expand Down Expand Up @@ -626,45 +626,43 @@ func (k Keeper) convertCoinNativeERC20(
}

// unescrow ERC-20 tokens from EVM module address
res, err := k.ERC20().Transfer(
erc20Addr,
evm.EVM_MODULE_ADDRESS,
recipient,
coin.Amount.BigInt(),
ctx,
)
input, err := k.ERC20().ABI.Pack("transfer", recipient, coin.Amount.BigInt())
if err != nil {
return nil, errors.Wrap(err, "failed to transfer ERC20 tokens")
return nil, errors.Wrap(err, "failed to pack ABI args")
}
if !res {
return nil, fmt.Errorf("failed to transfer ERC20 tokens")
_, err = k.ERC20().CallContractWithInput(ctx, evm.EVM_MODULE_ADDRESS, &erc20Addr, true, input)
if err != nil {
return nil, errors.Wrap(err, "failed to transfer ERC20 tokens")
Unique-Divine marked this conversation as resolved.
Show resolved Hide resolved
}

// Check expected Receiver balance after transfer execution
// For fee-on-transfer tokens, the actual amount received may be less than the amount transferred.
// Retrieve the recipient's balance after the transfer to calculate the actual received amount.
recipientBalanceAfter, err := k.ERC20().BalanceOf(erc20Addr, recipient, ctx)
if err != nil {
return nil, errors.Wrap(err, "failed to retrieve balance")
return nil, errors.Wrap(err, "failed to retrieve recipient balance after transfer")
}
if recipientBalanceAfter == nil {
return nil, fmt.Errorf("failed to retrieve balance, balance is nil")
return nil, fmt.Errorf("failed to retrieve recipient balance after transfer, balance is nil")
Unique-Divine marked this conversation as resolved.
Show resolved Hide resolved
}

expectedFinalBalance := big.NewInt(0).Add(recipientBalanceBefore, coin.Amount.BigInt())
if r := recipientBalanceAfter.Cmp(expectedFinalBalance); r != 0 {
return nil, fmt.Errorf("expected balance after transfer to be %s, got %s", expectedFinalBalance, recipientBalanceAfter)
// Burn escrowed Coins based on the actual amount received by the recipient
actualReceivedAmount := big.NewInt(0).Sub(recipientBalanceAfter, recipientBalanceBefore)
if actualReceivedAmount.Sign() == 0 {
return nil, fmt.Errorf("no ERC20 tokens were received by the recipient")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

}

// Burn escrowed Coins
err = k.bankKeeper.BurnCoins(ctx, evm.ModuleName, sdk.NewCoins(coin))
burnCoin := sdk.NewCoin(coin.Denom, sdk.NewIntFromBigInt(actualReceivedAmount))
err = k.bankKeeper.BurnCoins(ctx, evm.ModuleName, sdk.NewCoins(burnCoin))
Unique-Divine marked this conversation as resolved.
Show resolved Hide resolved
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,
})

return &evm.MsgConvertCoinToEvmResponse{}, nil
Expand Down
Loading