[evm] Prevent invalid FunToken mappings in createFunTokenFromERC20 even though they are harmless.

[Unique-Divine]

Purpose / Abstract

This issue takes a bit of nuance and context to understand. I thought about it while filling out the audit scope doc for Code4rena since it asks about compatibility expectations with different EIPs (Etherum Improvement Proposals).

Versions Impacted: Main branch as of 2024-10-05 (commit: 37913c4)

Key Takeaway

I argue that it is possible to create FunToken mappings that are unusable due
to having an ERC20Addr for a contract that only partially implements the ERC20
standard.

This doesn't present a security issue because transaction message calls that use
the mapping, such as ConvertCoinToEvm, will simply fail to be executed. With
that said, it still seems like bad practice to allow the creation of these
invalid mappings in state even if they can't do damage.

The rest of this description outlines a proof for this argument, and an explanation on how to correct it is in the "Recommended Solution" section.

[FunToken definition]

import (
	github_com_NibiruChain_nibiru_v2_eth "github.com/NibiruChain/nibiru/v2/eth"
)

// FunToken is a fungible token mapping between a bank coin and a corresponding
// ERC-20 smart contract. Bank coins here refer to tokens like NIBI, IBC
// coins (ICS-20), and token factory coins, which are each represented by the
// "Coin" type in Golang.
type FunToken struct {
	// Hexadecimal address of the ERC20 token to which the `FunToken` maps
	Erc20Addr github_com_NibiruChain_nibiru_v2_eth.EIP55Addr 
	// bank_denom: Coin denomination in the Bank Module.
	BankDenom string 
	// True if the `FunToken` mapping was created from an existing bank coin and
	// the ERC-20 contract gets deployed by the module account. False if the
	// mapping was created from an externally owned ERC-20 contract.
	IsMadeFromCoin bool 
}

ERC20 Methods Used with FunTokens

There are two code paths when creating a FunToken mapping. Those are creating
the mapping from an existing Bank Coin and creating the mapping from an existing
ERC20 (marked by the FunToken.isMadeFromCoin field). When creating from a coin,
we have (isMadeFromCoin=true) and the ERC20 deployed is guaranteed to be valid
because the contrac embedded in the binary is used (x/evm/embeds/ERC20Minter.sol).

However when creating the FunToken mapping from an ERC20 address, the contract
already exists and only certain methods are called.

[Expand Call Digram for "createFunTokenFromERC20"]

sequenceDiagram
    participant User
    participant Nibiru Chain
    participant FunToken System
    participant ERC20 Contract
    participant Bank Module

    User->>Nibiru Chain: Request to create FunToken from ERC20
    Nibiru Chain->>FunToken System: Initiate createFunTokenFromERC20
    
    FunToken System->>ERC20 Contract: Call name()
    ERC20 Contract-->>FunToken System: Return token name
    
    FunToken System->>ERC20 Contract: Call symbol()
    ERC20 Contract-->>FunToken System: Return token symbol
    
    FunToken System->>ERC20 Contract: Call decimals()
    ERC20 Contract-->>FunToken System: Return token decimals
    
    FunToken System->>Bank Module: Create new bank coin denom
    Bank Module-->>FunToken System: Confirm denom creation
    
    FunToken System->>Nibiru Chain: Store FunToken mapping
    Nibiru Chain-->>User: Confirm FunToken creation

Loading

The comprehensive list of methods from the ERC20 specification that are used with FunToken mappings is as follows:

ERC20 Method	Usage	Called in createFunTokenFromERC20
name()	Used when creating a FunToken mapping to get the ERC20 token's name	✅
symbol()	Used when creating a FunToken mapping to get the ERC20 token's symbol	✅
decimals()	Used when creating a FunToken mapping to get the ERC20 token's decimal places	✅
transferFrom(sender, recipient, amount)	Used in the FunToken precompile's bankSend function to move ERC20 tokens from the user to the EVM module account	😢
transfer(recipient, amount)	Used in ConvertCoinToEvm for existing ERC20s to transfer tokens from the EVM module account to the recipient	😢
mint(recipient, amount)*	Used in ConvertCoinToEvm for native coins wrapped as ERC20s to create new tokens	😢
balanceOf(account)	Likely used to check balances before and after transfers	😢

ConvertCoinToEvm sends a coin with a valid "FunToken" mapping to the given recipient address ("to_eth_addr") in the corresponding ERC20 representation.

[Expand Call Digram for "ConvertCoinToEvm"]

sequenceDiagram
    participant User
    participant ConvertCoinToEvm
    participant Bank Module
    participant EVM Module
    participant ERC20 Contract

    User->>ConvertCoinToEvm: Convert Bank Coins to ERC20
    ConvertCoinToEvm->>Bank Module: SendCoinsFromAccountToModule
    Bank Module-->>ConvertCoinToEvm: Coins transferred to module
    
    alt is native coin
        ConvertCoinToEvm->>ERC20 Contract: mint(recipient, amount)
    else is existing ERC20
        ConvertCoinToEvm->>ERC20 Contract: transfer(recipient, amount)
    end
    
    ERC20 Contract-->>ConvertCoinToEvm: ERC20 tokens transferred
    ConvertCoinToEvm->>Bank Module: BurnCoins
    Bank Module-->>ConvertCoinToEvm: Coins burned
    ConvertCoinToEvm-->>User: Conversion complete

Loading

What this all means

In the current version of Nibiru EVM (commit: 37913c4), as long as the contract used for FunToken.Erc20Addr during
creatFunTokenFromERC20 implements name, symbol, and decimals and follows
the exact ABI of the ERC20 specification for these methods, it is possible to create a FunToken
mapping in state. However if the contract used is missing transferFrom,
transfer, mint, or balanceOf, its usage will fail during
ConvertCoinToEvm or the precompile method FunToken.bankSend

Recommended Solution

Validate that the other methods required from the ERC20 standard are implemented
during the creation of FunToken mappings. Likely, there are functions exported
from go-ethereum that make this easy to do.