8000 impr(erc20): Add Informal Audit IF-ETHERMINT-06 protection by danburck · Pull Request #191 · evmos/evmos · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

impr(erc20): Add Informal Audit IF-ETHERMINT-06 protection #191

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

Merged
merged 4 commits into from
Jan 4, 2022
Merged

impr(erc20): Add Informal Audit IF-ETHERMINT-06 protection #191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d69e1e0
impr(erc20): Add Informal Audit IF-ETHERMINT-06 protection
danburck Jan 4, 2022
eb1bd70
docs(erc20): Update spec with balance checks
danburck Jan 4, 2022
28e8733
impr(erc20): Address comments and update spec
danburck Jan 4, 2022
53892a1
impr(erc20): Update CHANGELOG
danburck Jan 4, 2022
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
- Check if supply exists for a token before when submitting a `RegisterCoinProposal`, allowing users to create an ERC20 representation of an invalid Cosmos Coin.
- Sanitize the ERC20 token name when creating coin metadata on ER `RegisterERC20Proposal`.
- Fix coin metadata validation error when registering an ERC20 with 0 denom units.
- (erc20) [\#191](https://github.com/tharsis/evmos/pull/191) Add direct balance protection (IF-ETHERMINT-06).


## [v0.4.2] - 2021-12-11

Expand Down
134 changes: 121 additions & 13 deletions x/erc20/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@ package keeper
import (
"bytes"
"context"
"math/big"

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/ethereum/go-ethereum/accounts/abi"
"github.com/ethereum/go-ethereum/common"
evmtypes "github.com/tharsis/ethermint/x/evm/types"

Expand All @@ -17,7 +19,10 @@ var _ types.MsgServer = &Keeper{}

// ConvertCoin converts ERC20 tokens into Cosmos-native Coins for both
// Cosmos-native and ERC20 TokenPair Owners
func (k Keeper) ConvertCoin(goCtx context.Context, msg *types.MsgConvertCoin) (*types.MsgConvertCoinResponse, error) {
func (k Keeper) ConvertCoin(
goCtx context.Context,
msg *types.MsgConvertCoin,
) (*types.MsgConvertCoinResponse, error) {
ctx := sdk.UnwrapSDKContext(goCtx)
k.evmKeeper.WithContext(ctx)

Expand All @@ -44,22 +49,23 @@ func (k Keeper) ConvertCoin(goCtx context.Context, msg *types.MsgConvertCoin) (*
return nil, nil
}

// Check ownership
// Check ownership and execute conversion
switch {
case pair.IsNativeCoin():
// case 1.1
return k.convertCoinNativeCoin(ctx, pair, msg, receiver, sender)
return k.convertCoinNativeCoin(ctx, pair, msg, receiver, sender) // case 1.1
case pair.IsNativeERC20():
// case 2.2
return k.convertCoinNativeERC20(ctx, pair, msg, receiver, sender)
return k.convertCoinNativeERC20(ctx, pair, msg, receiver, sender) // case 2.2
default:
return nil, types.ErrUndefinedOwner
}
}

// ConvertERC20 converts ERC20 tokens into Cosmos-native Coins for both
// Cosmos-native and ERC20 TokenPair Owners
func (k Keeper) ConvertERC20(goCtx context.Context, msg *types.MsgConvertERC20) (*types.MsgConvertERC20Response, error) {
func (k Keeper) ConvertERC20(
goCtx context.Context,
msg *types.MsgConvertERC20,
) (*types.MsgConvertERC20Response, error) {
ctx := sdk.UnwrapSDKContext(goCtx)
k.evmKeeper.WithContext(ctx)

Expand Down Expand Up @@ -89,19 +95,19 @@ func (k Keeper) ConvertERC20(goCtx context.Context, msg *types.MsgConvertERC20)
// Check ownership
switch {
case pair.IsNativeCoin():
// case 1.2
return k.convertERC20NativeCoin(ctx, pair, msg, receiver, sender)
return k.convertERC20NativeCoin(ctx, pair, msg, receiver, sender) // case 1.2
case pair.IsNativeERC20():
// case 2.1
return k.convertERC20NativeToken(ctx, pair, msg, receiver, sender)
return k.convertERC20NativeToken(ctx, pair, msg, receiver, sender) // case 2.1
default:
return nil, types.ErrUndefinedOwner
}
}

// convertCoinNativeCoin handles the Coin conversion flow for a native coin token pair:
// convertCoinNativeCoin handles the Coin conversion flow for a native coin
// token pair:
// - Escrow Coins on module account (Coins are not burned)
// - Mint Tokens and send to receiver
// - Check if token balance increased by amount
func (k Keeper) convertCoinNativeCoin(
ctx sdk.Context,
pair types.TokenPair,
Expand All @@ -113,6 +119,7 @@ func (k Keeper) convertCoinNativeCoin(
coins := sdk.Coins{msg.Coin}
erc20 := contracts.ERC20BurnableAndMintableContract.ABI
contract := pair.GetERC20Contract()
balanceToken := k.balanceOf(ctx, erc20, contract, receiver)

// Escrow Coins on module account
if err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, sender, types.ModuleName, coins); err != nil {
Expand All @@ -125,6 +132,17 @@ func (k Keeper) convertCoinNativeCoin(
return nil, err
}

// Check expected Receiver balance after transfer execution
tokens := msg.Coin.Amount.BigInt()
balanceTokenAfter := k.balanceOf(ctx, erc20, contract, receiver)
exp := big.NewInt(0).Add(balanceToken, tokens)
if r := balanceTokenAfter.Cmp(exp); r != 0 {
return nil, sdkerrors.Wrapf(
types.ErrInvalidConversionBalance,
"invalid token balance - expected: %v, actual: %v", exp, balanceTokenAfter,
)
}

ctx.EventManager().EmitEvents(
sdk.Events{
sdk.NewEvent(
Expand All @@ -141,10 +159,12 @@ func (k Keeper) convertCoinNativeCoin(
return &types.MsgConvertCoinResponse{}, nil
}

// convertCoinNativeERC20 handles the Coin conversion flow for a native ERC20 token pair:
// convertCoinNativeERC20 handles the Coin conversion flow for a native ERC20
// token pair:
// - Escrow Coins on module account
// - Unescrow Tokens that have been previously escrowed with ConvertERC20 and send to receiver
// - Burn escrowed Coins
// - Check if token balance increased by amount
func (k Keeper) convertCoinNativeERC20(
ctx sdk.Context,
pair types.TokenPair,
Expand All @@ -156,6 +176,7 @@ func (k Keeper) convertCoinNativeERC20(
coins := sdk.Coins{msg.Coin}
erc20 := contracts.ERC20BurnableAndMintableContract.ABI
contract := pair.GetERC20Contract()
balanceToken := k.balanceOf(ctx, erc20, contract, receiver)

// Escrow Coins on module account
if err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, sender, types.ModuleName, coins); err != nil {
Expand Down Expand Up @@ -184,6 +205,17 @@ func (k Keeper) convertCoinNativeERC20(
return nil, sdkerrors.Wrap(err, "failed to burn coins")
}

// Check expected Receiver balance after transfer execution
tokens := msg.Coin.Amount.BigInt()
balanceTokenAfter := k.balanceOf(ctx, erc20, contract, receiver)
exp := big.NewInt(0).Add(balanceToken, tokens)
if r := balanceTokenAfter.Cmp(exp); r != 0 {
return nil, sdkerrors.Wrapf(
types.ErrInvalidConversionBalance,
"invalid token balance - expected: %v, actual: %v", exp, balanceTokenAfter,
)
}

ctx.EventManager().EmitEvents(
sdk.Events{
sdk.NewEvent(
Expand All @@ -203,6 +235,8 @@ func (k Keeper) convertCoinNativeERC20(
// convertERC20NativeCoin handles the erc20 conversion flow for a native coin token pair:
// - Burn escrowed tokens
// - Unescrow coins that have been previously escrowed with ConvertCoin
// - Check if coin balance increased by amount
// - Check if token balance decreased by amount
func (k Keeper) convertERC20NativeCoin(
ctx sdk.Context,
pair types.TokenPair,
Expand All @@ -214,6 +248,8 @@ func (k Keeper) convertERC20NativeCoin(
coins := sdk.Coins{sdk.Coin{Denom: pair.Denom, Amount: msg.Amount}}
erc20 := contracts.ERC20BurnableAndMintableContract.ABI
contract := pair.GetERC20Contract()
balanceCoin := k.bankKeeper.GetBalance(ctx, receiver, pair.Denom)
balanceToken := k.balanceOf(ctx, erc20, contract, sender)

// Burn escrowed tokens
_, err := k.CallEVM(ctx, erc20, types.ModuleAddress, contract, "burnCoins", sender, msg.Amount.BigInt())
Expand All @@ -225,6 +261,28 @@ func (k Keeper) convertERC20NativeCoin(
if err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, receiver, coins); err != nil {
return nil, err
}
// Check expected Receiver balance after transfer execution
balanceCoinAfter := k.bankKeeper.GetBalance(ctx, receiver, pair.Denom)
expCoin := balanceCoin.Add(coins[0])
if ok := balanceCoinAfter.IsEqual(expCoin); !ok {
return nil, sdkerrors.Wrapf(
types.ErrInvalidConversionBalance,
"invalid coin balance - expected: %v, actual: %v",
expCoin, balanceCoinAfter,
)
}

// Check expected Sender balance after transfer execution
tokens := coins[0].Amount.BigInt()
balanceTokenAfter := k.balanceOf(ctx, erc20, contract, sender)
expToken := big.NewInt(0).Sub(balanceToken, tokens)
if r := balanceTokenAfter.Cmp(expToken); r != 0 {
return nil, sdkerrors.Wrapf(
types.ErrInvalidConversionBalance,
"invalid token balance - expected: %v, actual: %v",
expToken, balanceTokenAfter,
)
}

ctx.EventManager().EmitEvents(
sdk.Events{
Expand All @@ -246,6 +304,8 @@ func (k Keeper) convertERC20NativeCoin(
// - Escrow tokens on module account (Don't burn as module is not contract owner)
// - Mint coins on module
// - Send minted coins to the receiver
// - Check if coin balance increased by amount
// - Check if token balance decreased by amount
func (k Keeper) convertERC20NativeToken(
ctx sdk.Context,
pair types.TokenPair,
Expand All @@ -257,6 +317,8 @@ func (k Keeper) convertERC20NativeToken(
coins := sdk.Coins{sdk.Coin{Denom: pair.Denom, Amount: msg.Amount}}
erc20 := contracts.ERC20BurnableAndMintableContract.ABI
contract := pair.GetERC20Contract()
balanceCoin := k.bankKeeper.GetBalance(ctx, receiver, pair.Denom)
balanceToken := k.balanceOf(ctx, erc20, contract, sender)

// Escrow tokens on module account
transferData, err := erc20.Pack("transfer", types.ModuleAddress, msg.Amount.BigInt())
Expand Down Expand Up @@ -288,6 +350,29 @@ func (k Keeper) convertERC20NativeToken(
return nil, err
}

// Check expected Receiver balance after transfer execution
balanceCoinAfter := k.bankKeeper.GetBalance(ctx, receiver, pair.Denom)
expCoin := balanceCoin.Add(coins[0])
if ok := balanceCoinAfter.IsEqual(expCoin); !ok {
return nil, sdkerrors.Wrapf(
types.ErrInvalidConversionBalance,
"invalid coin balance - expected: %v, actual: %v",
expCoin, balanceCoinAfter,
)
}

// Check expected Sencer balance after transfer execution
tokens := coins[0].Amount.BigInt()
balanceTokenAfter := k.balanceOf(ctx, erc20, contract, sender)
expToken := big.NewInt(0).Sub(balanceToken, tokens)
if r := balanceTokenAfter.Cmp(expToken); r != 0 {
return nil, sdkerrors.Wrapf(
types.ErrInvalidConversionBalance,
"invalid token balance - expected: %v, actual: %v",
expToken, balanceTokenAfter,
)
}

ctx.EventManager().EmitEvents(
sdk.Events{
sdk.NewEvent(
Expand All @@ -303,3 +388,26 @@ func (k Keeper) convertERC20NativeToken(

return &types.MsgConvertERC20Response{}, nil
}

func (k Keeper) balanceOf(
ctx sdk.Context,
abi abi.ABI,
contract, account common.Address,
) *big.Int {
res, err := k.CallEVM(ctx, abi, types.ModuleAddress, contract, "balanceOf", account)
if err != nil {
return nil
}

unpacked, _ := abi.Unpack("balanceOf", res.Ret)
if len(unpacked) == 0 {
return nil
}

balance, ok := unpacked[0].(*big.Int)
if !ok {
return nil
}

return balance
}
18 changes: 15 additions & 3 deletions x/erc20/spec/03_state_transitions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,13 @@ Conversion of a registered `TokenPair` can be done via:

#### Invariants

- Only the ModuleAccount should have the Minter Role on the ERC20. Otherwise, the user could unilaterally mint an infinite supply of the ERC20 token and then convert them to the native Coin
- The user and the ModuleAccount (owner) should be the only ones that have the Burn Role for a Cosmos Coin
- There shouldn't exist any native Cosmos Coin ERC20 Contract (eg Evmos, Atom, Osmo ERC20 contracts) that is not owned by the governance
- Only the ModuleAccount should have the Minter Role on the ERC20. Otherwise,
the user could unilaterally mint an infinite supply of the ERC20 token and
then convert them to the native Coin
- The user and the ModuleAccount (owner) should be the only ones that have the
Burn Role for a Cosmos Coin
- There shouldn't exist any native Cosmos Coin ERC20 Contract (eg Evmos, Atom,
Osmo ERC20 contracts) that is not owned by the governance
- Token/Coin supply is maintained at all times:
- Total Coin supply = Coins + Escrowed Coins
- Total Token supply = Escrowed Coins = Minted Tokens
Expand All @@ -60,6 +64,7 @@ Conversion of a registered `TokenPair` can be done via:
1. Escrow Cosmos coin by sending them to the erc20 module account
2. Call `mint()` ERC20 tokens from the `ModuleAccount` address
3. Send minted Tokens to recipient address
4. Check if token balance increased by amount

#### 1.2 ERC20 to Coin

Expand All @@ -68,6 +73,9 @@ Conversion of a registered `TokenPair` can be done via:
3. If token is a ERC20 && Token Owner is `ModuleAccount`
1. Call `burnCoins()` on ERC20 to burn ERC20 tokens from the user balance
2. Send Coins (previously escrowed see 1.1) from module to the recipient address.
4. Check if
- Coin balance increased by amount
- Token balance decreased by amount

### 2. Registered ERC20

Expand All @@ -92,6 +100,9 @@ Conversion of a registered `TokenPair` can be done via:
1. Escrow ERC20 token by sending them to the erc20 module account
2. Mint Cosmos coins of the corresponding token pair denomination
3. Send coins to the recipient address
4. Check if
- Coin balance increased by amount
- Token balance decreased by amount

#### 2.2 Coin to ERC20

Expand All @@ -101,3 +112,4 @@ Conversion of a registered `TokenPair` can be done via:
1. Escrow Cosmos Coins by sending them to the erc20 module account
2. Unlock escrowed ERC20 from the module address by sending it to the recipient
3. Burn escrowed Cosmos coins
4. Check if token balance increased by amount
17 changes: 9 additions & 8 deletions x/erc20/types/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,13 @@ import (

// errors
var (
ErrInvalidErc20Address = sdkerrors.Register(ModuleName, 2, "invalid erc20 address")
ErrUnmatchingCosmosDenom = sdkerrors.Register(ModuleName, 3, "unmatching cosmos denom")
ErrNotAllowedBridge = sdkerrors.Register(ModuleName, 4, "not allowed bridge")
ErrInternalEthMinting = sdkerrors.Register(ModuleName, 5, "internal ethereum minting error")
ErrWritingEthTxPayload = sdkerrors.Register(ModuleName, 6, "writing ethereum tx payload error")
ErrInternalTokenPair = sdkerrors.Register(ModuleName, 7, "internal ethereum token mapping error")
ErrUndefinedOwner = sdkerrors.Register(ModuleName, 8, "undefined owner of contract pair")
ErrSuicidedContract = sdkerrors.Register(ModuleName, 9, "suicided contract pair")
ErrInvalidErc20Address = sdkerrors.Register(ModuleName, 2, "invalid erc20 address")
ErrUnmatchingCosmosDenom = sdkerrors.Register(ModuleName, 3, "unmatching cosmos denom")
ErrNotAllowedBridge = sdkerrors.Register(ModuleName, 4, "not allowed bridge")
ErrInternalEthMinting = sdkerrors.Register(ModuleName, 5, "internal ethereum minting error")
ErrWritingEthTxPayload = sdkerrors.Register(ModuleName, 6, "writing ethereum tx payload error")
ErrInternalTokenPair = sdkerrors.Register(ModuleName, 7, "internal ethereum token mapping error")
ErrUndefinedOwner = sdkerrors.Register(ModuleName, 8, "undefined owner of contract pair")
ErrSuicidedContract = sdkerrors.Register(ModuleName, 9, "suicided contract pair")
ErrInvalidConversionBalance = sdkerrors.Register(ModuleName, 10, "invalid conversion balance")
)
1 change: 1 addition & 0 deletions x/erc20/types/interfaces.go
747E
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ type BankKeeper interface {
GetDenomMetaData(ctx sdk.Context, denom string) (banktypes.Metadata, bool)
SetDenomMetaData(ctx sdk.Context, denomMetaData banktypes.Metadata)
HasSupply(ctx sdk.Context, denom string) bool
GetBalance(ctx sdk.Context, addr sdk.AccAddress, denom string) sdk.Coin
}

// EVMKeeper defines the expected EVM keeper interface used on erc20
Expand Down
0