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.

CHANGELOG.md

@@ -79,6 +79,10 @@ Ethereum transactions, such as in the case of an EthereumTx that influences the
 `StateDB`, then calls a precompile that also changes non-EVM state, and then EVM
 reverts inside of a try-catch.
 - [#2098](https://github.com/NibiruChain/nibiru/pull/2098) - test(evm): statedb tests for race conditions within funtoken precompile
+- [#2090](https://github.com/NibiruChain/nibiru/pull/2090) - fix(evm): Account
+for (1) ERC20 transfers with tokens that return false success values instead of
+throwing an error and (2) ERC20 transfers with other operations that don't bring
+about the expected resulting balance for the transfer recipient.
 
 #### Nibiru EVM | Before Audit 1 - 2024-10-18
 

x/evm/embeds/artifacts/contracts/IFunToken.sol/IFunToken.json

@@ -22,7 +22,13 @@
         }
       ],
       "name": "bankSend",
-      "outputs": [],
+      "outputs": [
+        {
+          "internalType": "uint256",
+          "name": "sentAmount",
+          "type": "uint256"
+        }
+      ],
       "stateMutability": "nonpayable",
       "type": "function"
     }

x/evm/embeds/contracts/IFunToken.sol

@@ -5,11 +5,18 @@ pragma solidity >=0.8.19;
 /// coins to a Nibiru bech32 address using the "FunToken" mapping between the
 /// ERC20 and bank.
 interface IFunToken {
-  /// @dev bankSend sends ERC20 tokens as coins to a Nibiru base account
-  /// @param erc20 the address of the ERC20 token contract
-  /// @param amount the amount of tokens to send
-  /// @param to the receiving Nibiru base account address as a string
-  function bankSend(address erc20, uint256 amount, string memory to) external;
+    /// @dev bankSend sends ERC20 tokens as coins to a Nibiru base account
+    /// @param erc20 - the address of the ERC20 token contract
+    /// @param amount - the amount of tokens to send
+    /// @param to - the receiving Nibiru base account address as a string
+    /// @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.
+    function bankSend(
+        address erc20,
+        uint256 amount,
+        string memory to
+    ) external returns (uint256 sentAmount);
 }
 
 address constant FUNTOKEN_PRECOMPILE_ADDRESS = 0x0000000000000000000000000000000000000800;

x/evm/keeper/erc20.go

@@ -73,23 +73,53 @@
 func (e erc20Calls) Transfer(
 	contract, from, to gethcommon.Address, amount *big.Int,
 	ctx sdk.Context,
-) (out bool, err error) {
+) (balanceIncrease *big.Int, err error) {
+	recipientBalanceBefore, err := e.BalanceOf(contract, to, ctx)
+	if err != nil {
+		return balanceIncrease, errors.Wrap(err, "failed to retrieve recipient balance")
+	}
+
 	input, err := e.ABI.Pack("transfer", to, amount)
 	if err != nil {
-		return false, fmt.Errorf("failed to pack ABI args: %w", err)
+		return balanceIncrease, fmt.Errorf("failed to pack ABI args: %w", err)
 	}
+
 	resp, _, err := e.CallContractWithInput(ctx, from, &contract, true, input)
 	if err != nil {
-		return false, err
+		return balanceIncrease, err
 	}
 
 	var erc20Bool ERC20Bool
 	err = e.ABI.UnpackIntoInterface(&erc20Bool, "transfer", resp.Ret)
 	if err != nil {
-		return false, err
+		return balanceIncrease, err
+	}
+
+	// Handle the case of success=false: https://github.com/NibiruChain/nibiru/issues/2080
+	success := erc20Bool.Value
+	if !success {
+		return balanceIncrease, fmt.Errorf("transfer executed but returned success=false")
+	}
+
+	recipientBalanceAfter, err := e.BalanceOf(contract, to, ctx)
+	if err != nil {
+		return balanceIncrease, errors.Wrap(err, "failed to retrieve recipient balance")
+	}
+
+	balanceIncrease = new(big.Int).Sub(recipientBalanceAfter, recipientBalanceBefore)
+
+	// For flexibility with fee on transfer tokens and other types of deductions,
+	// we cannot assume that the amount received by the recipient is equal to
+	// the call "amount". Instead, verify that the recipient got tokens and
+	// return the amount.
+	if balanceIncrease.Sign() <= 0 {
+		return balanceIncrease, fmt.Errorf(
+			"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(),
+		)
 	}
 
-	return erc20Bool.Value, nil
+	return balanceIncrease, err
 }
 
 // BalanceOf retrieves the balance of an ERC20 token for a specific account.

x/evm/keeper/erc20_test.go

@@ -34,7 +34,8 @@ func (s *Suite) TestERC20Calls() {
 
 	s.T().Log("Transfer - Not enough funds")
 	{
-		_, err := deps.EvmKeeper.ERC20().Transfer(contract, deps.Sender.EthAddr, evm.EVM_MODULE_ADDRESS, big.NewInt(9_420), deps.Ctx)
+		amt := big.NewInt(9_420)
+		_, err := deps.EvmKeeper.ERC20().Transfer(contract, deps.Sender.EthAddr, evm.EVM_MODULE_ADDRESS, amt, deps.Ctx)
 		s.ErrorContains(err, "ERC20: transfer amount exceeds balance")
 		// balances unchanged
 		evmtest.AssertERC20BalanceEqual(s.T(), deps, contract, deps.Sender.EthAddr, big.NewInt(0))
@@ -43,10 +44,16 @@ func (s *Suite) TestERC20Calls() {
 
 	s.T().Log("Transfer - Success (sanity check)")
 	{
-		_, err := deps.EvmKeeper.ERC20().Transfer(contract, evm.EVM_MODULE_ADDRESS, deps.Sender.EthAddr, big.NewInt(9_420), deps.Ctx)
+		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())
 	}
 
 	s.T().Log("Burn tokens - Allowed as non-owner")

x/evm/keeper/msg_server.go

@@ -491,30 +491,35 @@
 	fungibleTokenMapping := funTokens[0]
 
 	if fungibleTokenMapping.IsMadeFromCoin {
-		return k.convertCoinNativeCoin(ctx, sender, msg.ToEthAddr.Address, msg.BankCoin, fungibleTokenMapping)
+		return k.convertCoinToEvmBornCoin(
+			ctx, sender, msg.ToEthAddr.Address, msg.BankCoin, fungibleTokenMapping,
+		)
 	} else {
-		return k.convertCoinNativeERC20(ctx, sender, msg.ToEthAddr.Address, msg.BankCoin, fungibleTokenMapping)
+		return k.convertCoinToEvmBornERC20(
+			ctx, sender, msg.ToEthAddr.Address, msg.BankCoin, fungibleTokenMapping,
+		)
 	}
 }
 
-// Converts a native coin to an ERC20 token.
-// EVM module owns the ERC-20 contract and can mint the ERC-20 tokens.
-func (k Keeper) convertCoinNativeCoin(
+// Converts Bank Coins for FunToken mapping that was born from a coin
+// (IsMadeFromCoin=true) into the ERC20 tokens. EVM module owns the ERC-20
+// contract and can mint the ERC-20 tokens.
+func (k Keeper) convertCoinToEvmBornCoin(
 	ctx sdk.Context,
 	sender sdk.AccAddress,
 	recipient gethcommon.Address,
 	coin sdk.Coin,
 	funTokenMapping evm.FunToken,
 ) (*evm.MsgConvertCoinToEvmResponse, error) {
-	// Step 1: Escrow bank coins with EVM module account
+	// Step 1: Send Bank Coins to the EVM module
 	err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, sender, evm.ModuleName, sdk.NewCoins(coin))
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to send coins to module account")
 	}
 
 	erc20Addr := funTokenMapping.Erc20Addr.Address
 
-	// Step 2: mint ERC-20 tokens for recipient
+	// Step 2: Mint ERC20 tokens to the recipient
 	evmResp, err := k.CallContract(
 		ctx,
 		embeds.SmartContract_ERC20Minter.ABI,
@@ -542,93 +547,64 @@
 	return &evm.MsgConvertCoinToEvmResponse{}, nil
 }
 
-// Converts a coin that was originally an ERC20 token, and that was converted to a bank coin, back to an ERC20 token.
-// EVM module does not own the ERC-20 contract and cannot mint the ERC-20 tokens.
-// EVM module has escrowed tokens in the first conversion from ERC-20 to bank coin.
-func (k Keeper) convertCoinNativeERC20(
+// Converts a coin that was originally an ERC20 token, and that was converted to
+// a bank coin, back to an ERC20 token. EVM module does not own the ERC-20
+// contract and cannot mint the ERC-20 tokens. EVM module has escrowed tokens in
+// the first conversion from ERC-20 to bank coin.
+func (k Keeper) convertCoinToEvmBornERC20(
 	ctx sdk.Context,
 	sender sdk.AccAddress,
 	recipient gethcommon.Address,
 	coin sdk.Coin,
 	funTokenMapping evm.FunToken,
 ) (*evm.MsgConvertCoinToEvmResponse, error) {
 	erc20Addr := funTokenMapping.Erc20Addr.Address
-
-	recipientBalanceBefore, err := k.ERC20().BalanceOf(erc20Addr, recipient, ctx)
-	if err != nil {
-		return nil, errors.Wrap(err, "failed to retrieve balance")
-	}
-	if recipientBalanceBefore == nil {
-		return nil, fmt.Errorf("failed to retrieve balance, balance is nil")
-	}
-
-	// Escrow Coins on module account
+	// 1 | Caller transfers Bank Coins to be converted to ERC20 tokens.
 	if err := k.bankKeeper.SendCoinsFromAccountToModule(
 		ctx,
 		sender,
 		evm.ModuleName,
 		sdk.NewCoins(coin),
 	); err != nil {
-		return nil, errors.Wrap(err, "failed to escrow coins")
+		return nil, errors.Wrap(err, "error sending Bank Coins to the EVM")
 	}
 
-	// verify that the EVM module account has enough escrowed ERC-20 to transfer
-	// should never fail, because the coins were minted from the escrowed tokens, but check just in case
-	evmModuleBalance, err := k.ERC20().BalanceOf(
-		erc20Addr,
-		evm.EVM_MODULE_ADDRESS,
-		ctx,
-	)
-	if err != nil {
-		return nil, errors.Wrap(err, "failed to retrieve balance")
-	}
-	if evmModuleBalance == nil {
-		return nil, fmt.Errorf("failed to retrieve balance, balance is nil")
-	}
-	if evmModuleBalance.Cmp(coin.Amount.BigInt()) < 0 {
-		return nil, fmt.Errorf("insufficient balance in EVM module account")
-	}
-
-	// unescrow ERC-20 tokens from EVM module address
-	res, err := k.ERC20().Transfer(
+	// 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
+	// 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(
 		erc20Addr,
 		evm.EVM_MODULE_ADDRESS,
 		recipient,
 		coin.Amount.BigInt(),
 		ctx,
 	)
 	if err != nil {
-		return nil, errors.Wrap(err, "failed to transfer ERC20 tokens")
-	}
-	if !res {
-		return nil, fmt.Errorf("failed to transfer ERC20 tokens")
-	}
-
-	// Check expected Receiver balance after transfer execution
-	recipientBalanceAfter, err := k.ERC20().BalanceOf(erc20Addr, recipient, ctx)
-	if err != nil {
-		return nil, errors.Wrap(err, "failed to retrieve balance")
-	}
-	if recipientBalanceAfter == nil {
-		return nil, fmt.Errorf("failed to retrieve balance, balance is nil")
+		return nil, errors.Wrap(err, "failed to transfer ERC-20 tokens")
 	}
 
-	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
-	err = k.bankKeeper.BurnCoins(ctx, evm.ModuleName, sdk.NewCoins(coin))
+	// 3 | In the FunToken ERC20 → BC conversion process that preceded this
+	// 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.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,
 	})
 
 	return &evm.MsgConvertCoinToEvmResponse{}, nil

x/evm/precompile/funtoken.go

@@ -143,20 +143,19 @@
 
 	// Caller transfers ERC20 to the EVM account
 	transferTo := evm.EVM_MODULE_ADDRESS
-	_, err = p.evmKeeper.ERC20().Transfer(erc20, caller, transferTo, amount, ctx)
+	gotAmount, 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)
+		return nil, fmt.Errorf("error in ERC20.transfer from caller to EVM account: %w", err)
 	}
 
 	// EVM account mints FunToken.BankDenom to module account
-	amt := math.NewIntFromBigInt(amount)
-	coinToSend := sdk.NewCoin(funtoken.BankDenom, amt)
+	coinToSend := sdk.NewCoin(funtoken.BankDenom, math.NewIntFromBigInt(gotAmount))
 	if funtoken.IsMadeFromCoin {
 		// If the FunToken mapping was created from a bank coin, then the EVM account
 		// owns the ERC20 contract and was the original minter of the ERC20 tokens.
 		// Since we're sending them away and want accurate total supply tracking, the
 		// tokens need to be burned.
-		_, err = p.evmKeeper.ERC20().Burn(erc20, evm.EVM_MODULE_ADDRESS, amount, ctx)
+		_, err = p.evmKeeper.ERC20().Burn(erc20, evm.EVM_MODULE_ADDRESS, gotAmount, ctx)
 		if err != nil {
 			err = fmt.Errorf("ERC20.Burn: %w", err)
 			return
@@ -187,7 +186,7 @@
 
 	// TODO: UD-DEBUG: feat: Emit EVM events
 
-	return method.Outputs.Pack()
+	return method.Outputs.Pack(gotAmount)
 }
 
 func SafeMintCoins(

x/evm/precompile/funtoken_test.go

@@ -126,18 +126,32 @@ func (s *FuntokenSuite) TestHappyPath() {
 	input, err := embeds.SmartContract_FunToken.ABI.Pack(string(precompile.FunTokenMethod_BankSend), callArgs...)
 	s.NoError(err)
 
-	_, resp, err := evmtest.CallContractTx(
+	_, ethTxResp, err := evmtest.CallContractTx(
 		&deps,
 		precompile.PrecompileAddr_FunToken,
 		input,
 		deps.Sender,
 	)
 	s.Require().NoError(err)
-	s.Require().Empty(resp.VmError)
+	s.Require().Empty(ethTxResp.VmError)
 
-	evmtest.AssertERC20BalanceEqual(s.T(), deps, erc20, deps.Sender.EthAddr, big.NewInt(69_000))
-	evmtest.AssertERC20BalanceEqual(s.T(), deps, erc20, evm.EVM_MODULE_ADDRESS, big.NewInt(0))
+	evmtest.AssertERC20BalanceEqual(
+		s.T(), deps, erc20, deps.Sender.EthAddr, big.NewInt(69_000),
+	)
+	evmtest.AssertERC20BalanceEqual(
+		s.T(), deps, erc20, evm.EVM_MODULE_ADDRESS, big.NewInt(0),
+	)
 	s.Equal(sdk.NewInt(420).String(),
 		deps.App.BankKeeper.GetBalance(deps.Ctx, randomAcc, funtoken.BankDenom).Amount.String(),
 	)
+
+	s.T().Log("Parse the response contract addr and response bytes")
+	var sentAmt *big.Int
+	err = embeds.SmartContract_FunToken.ABI.UnpackIntoInterface(
+		&sentAmt,
+		string(precompile.FunTokenMethod_BankSend),
+		ethTxResp.Ret,
+	)
+	s.NoError(err)
+	s.Require().Equal("420", sentAmt.String())
 }