fix(evm): erc20 born funtoken: properly burn bank coins after convert…

…ing coin back to erc20 (#2139)

Co-authored-by: Unique Divine <[email protected]>

CHANGELOG.md

@@ -55,6 +55,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - [#2125](https://github.com/NibiruChain/nibiru/pull/2125) - feat(evm-precompile):Emit EVM events created to reflect the ABCI events that occur outside the EVM to make sure that block explorers and indexers can find indexed ABCI event information.
 - [#2129](https://github.com/NibiruChain/nibiru/pull/2129) - fix(evm): issue with infinite recursion in erc20 funtoken contracts
 - [#2134](https://github.com/NibiruChain/nibiru/pull/2134) - fix(evm): query of NIBI should use bank state, not the StateDB
+- [#2139](https://github.com/NibiruChain/nibiru/pull/2139) - fix(evm): erc20 born funtoken: properly burn bank coins after converting coin back to erc20 
 - [#2140](https://github.com/NibiruChain/nibiru/pull/2140) - fix(bank): bank keeper extension now charges gas for the bank operations
 - [#2141](https://github.com/NibiruChain/nibiru/pull/2141) - refactor: simplify account retrieval operation in `nibid q evm account`.
 - [#2142](https://github.com/NibiruChain/nibiru/pull/2142) - fix(bank): add additional missing methods to the NibiruBankKeeper

x/evm/const.go

@@ -6,6 +6,7 @@ import (
 	"math/big"
 
 	"github.com/NibiruChain/collections"
+	sdk "github.com/cosmos/cosmos-sdk/types"
 	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
 	gethcommon "github.com/ethereum/go-ethereum/common"
 )
@@ -84,9 +85,11 @@ const (
 )
 
 var EVM_MODULE_ADDRESS gethcommon.Address
+var EVM_MODULE_ADDRESS_NIBI sdk.AccAddress
 
 func init() {
-	EVM_MODULE_ADDRESS = gethcommon.BytesToAddress(authtypes.NewModuleAddress(ModuleName))
+	EVM_MODULE_ADDRESS_NIBI = authtypes.NewModuleAddress(ModuleName)
+	EVM_MODULE_ADDRESS = gethcommon.BytesToAddress(EVM_MODULE_ADDRESS_NIBI)
 }
 
 // NativeToWei converts a "unibi" amount to "wei" units for the EVM.

x/evm/embeds/contracts/TestERC20TransferWithFee.sol

@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
+
+contract TestERC20TransferWithFee is ERC20 {
+    uint256 constant FEE_PERCENTAGE = 10;
+
+    constructor(string memory name, string memory symbol)
+    ERC20(name, symbol) {
+        _mint(msg.sender, 1000);
+    }
+
+    function transfer(address to, uint256 amount) public virtual override returns (bool) {
+        address owner = _msgSender();
+        require(amount > 0, "Transfer amount must be greater than zero");
+
+        uint256 fee = (amount * FEE_PERCENTAGE) / 100;
+        uint256 recipientAmount = amount - fee;
+
+        _transfer(owner, address(this), fee);
+        _transfer(owner, to, recipientAmount);
+
+        return true;
+    }
+}

x/evm/embeds/embeds.go

@@ -39,6 +39,8 @@ var (
 	testPrecompileSelfCallRevertJson []byte
 	//go:embed artifacts/contracts/TestInfiniteRecursionERC20.sol/TestInfiniteRecursionERC20.json
 	testInfiniteRecursionERC20Json []byte
+	//go:embed artifacts/contracts/TestERC20TransferWithFee.sol/TestERC20TransferWithFee.json
+	testERC20TransferWithFee []byte
 )
 
 var (
@@ -126,6 +128,12 @@ var (
 		Name:      "TestInfiniteRecursionERC20.sol",
 		EmbedJSON: testInfiniteRecursionERC20Json,
 	}
+	// SmartContract_TestERC20TransferWithFee is a test contract
+	// which simulates malicious ERC20 behavior by adding fee to the transfer() function
+	SmartContract_TestERC20TransferWithFee = CompiledEvmContract{
+		Name:      "TestERC20TransferWithFee.sol",
+		EmbedJSON: testERC20TransferWithFee,
+	}
 )
 
 func init() {
@@ -141,6 +149,7 @@ func init() {
 	SmartContract_TestERC20TransferThenPrecompileSend.MustLoad()
 	SmartContract_TestPrecompileSelfCallRevert.MustLoad()
 	SmartContract_TestInfiniteRecursionERC20.MustLoad()
+	SmartContract_TestERC20TransferWithFee.MustLoad()
 }
 
 type CompiledEvmContract struct {

x/evm/embeds/embeds_test.go

@@ -20,5 +20,6 @@ func TestLoadContracts(t *testing.T) {
 		embeds.SmartContract_TestNativeSendThenPrecompileSendJson.MustLoad()
 		embeds.SmartContract_TestERC20TransferThenPrecompileSend.MustLoad()
 		embeds.SmartContract_TestInfiniteRecursionERC20.MustLoad()
+		embeds.SmartContract_TestERC20TransferWithFee.MustLoad()
 	})
 }

x/evm/keeper/funtoken_from_erc20_test.go

@@ -452,6 +452,89 @@ func (s *FunTokenFromErc20Suite) TestFunTokenInfiniteRecursionERC20() {
 	s.Require().ErrorContains(err, "execution reverted")
 }
 
+// TestSendERC20WithFee creates a funtoken from a malicious contract which charges a 10% fee on any transfer.
+// Test ensures that after sending ERC20 token to coin and back, all bank coins are burned.
+func (s *FunTokenFromErc20Suite) TestSendERC20WithFee() {
+	deps := evmtest.NewTestDeps()
+
+	s.T().Log("Deploy ERC20")
+	metadata := keeper.ERC20Metadata{
+		Name:   "erc20name",
+		Symbol: "TOKEN",
+	}
+	deployResp, err := evmtest.DeployContract(
+		&deps, embeds.SmartContract_TestERC20TransferWithFee,
+		metadata.Name, metadata.Symbol,
+	)
+	s.Require().NoError(err)
+
+	s.T().Log("CreateFunToken for the ERC20 with fee")
+	s.Require().NoError(testapp.FundAccount(
+		deps.App.BankKeeper,
+		deps.Ctx,
+		deps.Sender.NibiruAddr,
+		deps.EvmKeeper.FeeForCreateFunToken(deps.Ctx),
+	))
+
+	resp, err := deps.EvmKeeper.CreateFunToken(
+		sdk.WrapSDKContext(deps.Ctx),
+		&evm.MsgCreateFunToken{
+			FromErc20: &eth.EIP55Addr{
+				Address: deployResp.ContractAddr,
+			},
+			Sender: deps.Sender.NibiruAddr.String(),
+		},
+	)
+	s.Require().NoError(err, "erc20 %s", deployResp.ContractAddr)
+	bankDemon := resp.FuntokenMapping.BankDenom
+
+	randomAcc := testutil.AccAddress()
+
+	deps.ResetGasMeter()
+
+	s.T().Log("send erc20 tokens to Bank")
+	_, err = deps.EvmKeeper.CallContract(
+		deps.Ctx,
+		embeds.SmartContract_FunToken.ABI,
+		deps.Sender.EthAddr,
+		&precompile.PrecompileAddr_FunToken,
+		true,
+		evmtest.FunTokenGasLimitSendToEvm,
+		"sendToBank",
+		deployResp.ContractAddr,
+		big.NewInt(100),
+		randomAcc.String(),
+	)
+	s.Require().NoError(err)
+
+	s.T().Log("check balances")
+	evmtest.AssertERC20BalanceEqual(s.T(), deps, deployResp.ContractAddr, deps.Sender.EthAddr, big.NewInt(900))
+	evmtest.AssertERC20BalanceEqual(s.T(), deps, deployResp.ContractAddr, deployResp.ContractAddr, big.NewInt(10))
+	evmtest.AssertERC20BalanceEqual(s.T(), deps, deployResp.ContractAddr, evm.EVM_MODULE_ADDRESS, big.NewInt(90))
+	s.Require().Equal(sdk.NewInt(90), deps.App.BankKeeper.GetBalance(deps.Ctx, randomAcc, bankDemon).Amount)
+
+	deps.ResetGasMeter()
+
+	s.T().Log("send Bank tokens back to erc20")
+	_, err = deps.EvmKeeper.ConvertCoinToEvm(sdk.WrapSDKContext(deps.Ctx),
+		&evm.MsgConvertCoinToEvm{
+			ToEthAddr: eth.EIP55Addr{
+				Address: deps.Sender.EthAddr,
+			},
+			Sender:   randomAcc.String(),
+			BankCoin: sdk.NewCoin(bankDemon, sdk.NewInt(90)),
+		},
+	)
+	s.Require().NoError(err)
+
+	s.T().Log("check balances")
+	evmtest.AssertERC20BalanceEqual(s.T(), deps, deployResp.ContractAddr, deps.Sender.EthAddr, big.NewInt(981))
+	evmtest.AssertERC20BalanceEqual(s.T(), deps, deployResp.ContractAddr, deployResp.ContractAddr, big.NewInt(19))
+	evmtest.AssertERC20BalanceEqual(s.T(), deps, deployResp.ContractAddr, evm.EVM_MODULE_ADDRESS, big.NewInt(0))
+	s.Require().True(deps.App.BankKeeper.GetBalance(deps.Ctx, randomAcc, bankDemon).Amount.Equal(sdk.NewInt(0)))
+	s.Require().True(deps.App.BankKeeper.GetBalance(deps.Ctx, evm.EVM_MODULE_ADDRESS_NIBI, bankDemon).Amount.Equal(sdk.NewInt(0)))
+}
+
 type FunTokenFromErc20Suite struct {
 	suite.Suite
 }

x/evm/keeper/msg_server.go

@@ -603,14 +603,14 @@ func (k Keeper) convertCoinToEvmBornERC20(
 
 	// 2 | EVM sends ERC20 tokens to the "to" account.
 	// This should never fail due to the EVM account lacking ERc20 fund because
-	// the an account must have sent the EVM module ERC20 tokens in the mapping
+	// the account must have sent the EVM module ERC20 tokens in the mapping
 	// in order to create the coins originally.
 	//
 	// Said another way, if an asset is created as an ERC20 and some amount is
 	// converted to its Bank Coin representation, a balance of the ERC20 is left
 	// inside the EVM module account in order to convert the coins back to
 	// ERC20s.
-	actualSentAmount, _, err := k.ERC20().Transfer(
+	_, _, err := k.ERC20().Transfer(
 		erc20Addr,
 		evm.EVM_MODULE_ADDRESS,
 		recipient,
@@ -625,8 +625,7 @@ func (k Keeper) convertCoinToEvmBornERC20(
 	// TxMsg, the Bank Coins were minted. Consequently, to preserve an invariant
 	// on the sum of the FunToken's bank and ERC20 supply, we burn the coins here
 	// in the BC → ERC20 conversion.
-	burnCoin := sdk.NewCoin(coin.Denom, sdk.NewIntFromBigInt(actualSentAmount))
-	err = k.Bank.BurnCoins(ctx, evm.ModuleName, sdk.NewCoins(burnCoin))
+	err = k.Bank.BurnCoins(ctx, evm.ModuleName, sdk.NewCoins(coin))
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to burn coins")
 	}
@@ -636,7 +635,7 @@ func (k Keeper) convertCoinToEvmBornERC20(
 		Sender:               sender.String(),
 		Erc20ContractAddress: funTokenMapping.Erc20Addr.String(),
 		ToEthAddr:            recipient.String(),
-		BankCoin:             burnCoin,
+		BankCoin:             coin,
 	})
 
 	return &evm.MsgConvertCoinToEvmResponse{}, nil