From d69e1e078fa9907b5e9639697cebc196acb63d18 Mon Sep 17 00:00:00 2001 From: Daniel Burckhardt Date: Tue, 4 Jan 2022 12:01:58 +0100 Subject: [PATCH 1/4] impr(erc20): Add Informal Audit IF-ETHERMINT-06 protection --- x/erc20/keeper/msg_server.go | 127 +++++++++++++++++++++++++++++++---- x/erc20/types/errors.go | 17 ++--- x/erc20/types/interfaces.go | 1 + 3 files changed, 124 insertions(+), 21 deletions(-) diff --git a/x/erc20/keeper/msg_server.go b/x/erc20/keeper/msg_server.go index 1139b013d9..66037a2347 100644 --- a/x/erc20/keeper/msg_server.go +++ b/x/erc20/keeper/msg_server.go @@ -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" @@ -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) @@ -44,14 +49,12 @@ 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 } @@ -59,7 +62,10 @@ func (k Keeper) ConvertCoin(goCtx context.Context, msg *types.MsgConvertCoin) (* // 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) @@ -89,17 +95,16 @@ 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 func (k Keeper) convertCoinNativeCoin( @@ -113,6 +118,8 @@ func (k Keeper) convertCoinNativeCoin( coins := sdk.Coins{msg.Coin} erc20 := contracts.ERC20BurnableAndMintableContract.ABI contract := pair.GetERC20Contract() + balanceCoin := k.bankKeeper.GetBalance(ctx, sender, msg.Coin.Denom) + balanceToken := k.balanceOf(ctx, erc20, contract, receiver) // Escrow Coins on module account if err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, sender, types.ModuleName, coins); err != nil { @@ -125,6 +132,22 @@ func (k Keeper) convertCoinNativeCoin( return nil, err } + // Check expected Sender balance after transfer execution + balanceCoinAfter := k.bankKeeper.GetBalance(ctx, sender, pair.Denom) + if ok := balanceCoinAfter.IsEqual(balanceCoin.Sub(coins[0])); !ok { + return nil, sdkerrors.Wrap( + types.ErrInvalidConversionBalance, "invalid coin balance ", + ) + } + // Check expected Receiver balance after transfer execution + tokens := msg.Coin.Amount.BigInt() + balanceTokenAfter := k.balanceOf(ctx, erc20, contract, receiver) + if r := balanceTokenAfter.Cmp(big.NewInt(0).Add(balanceToken, tokens)); r != 0 { + return nil, sdkerrors.Wrap( + types.ErrInvalidConversionBalance, "invalid token balance ", + ) + } + ctx.EventManager().EmitEvents( sdk.Events{ sdk.NewEvent( @@ -141,7 +164,8 @@ 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 @@ -156,6 +180,8 @@ func (k Keeper) convertCoinNativeERC20( coins := sdk.Coins{msg.Coin} erc20 := contracts.ERC20BurnableAndMintableContract.ABI contract := pair.GetERC20Contract() + balanceCoin := k.bankKeeper.GetBalance(ctx, sender, pair.Denom) + balanceToken := k.balanceOf(ctx, erc20, contract, receiver) // Escrow Coins on module account if err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, sender, types.ModuleName, coins); err != nil { @@ -184,6 +210,22 @@ func (k Keeper) convertCoinNativeERC20( return nil, sdkerrors.Wrap(err, "failed to burn coins") } + // Check expected Sender balance after transfer execution + balanceCoinAfter := k.bankKeeper.GetBalance(ctx, sender, pair.Denom) + if ok := balanceCoinAfter.IsEqual(balanceCoin.Sub(coins[0])); !ok { + return nil, sdkerrors.Wrap( + types.ErrInvalidConversionBalance, "invalid coin balance ", + ) + } + // Check expected Receiver balance after transfer execution + tokens := msg.Coin.Amount.BigInt() + balanceTokenAfter := k.balanceOf(ctx, erc20, contract, receiver) + if r := balanceTokenAfter.Cmp(big.NewInt(0).Add(balanceToken, tokens)); r != 0 { + return nil, sdkerrors.Wrap( + types.ErrInvalidConversionBalance, "invalid token balance ", + ) + } + ctx.EventManager().EmitEvents( sdk.Events{ sdk.NewEvent( @@ -214,6 +256,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()) @@ -226,6 +270,22 @@ func (k Keeper) convertERC20NativeCoin( return nil, err } + // Check expected Sender balance after transfer execution + balanceCoinAfter := k.bankKeeper.GetBalance(ctx, receiver, pair.Denom) + if ok := balanceCoinAfter.IsEqual(balanceCoin.Add(coins[0])); !ok { + return nil, sdkerrors.Wrap( + types.ErrInvalidConversionBalance, "invalid coin balance ", + ) + } + // Check expected Receiver balance after transfer execution + tokens := coins[0].Amount.BigInt() + balanceTokenAfter := k.balanceOf(ctx, erc20, contract, sender) + if r := balanceTokenAfter.Cmp(big.NewInt(0).Sub(balanceToken, tokens)); r != 0 { + return nil, sdkerrors.Wrap( + types.ErrInvalidConversionBalance, "invalid token balance ", + ) + } + ctx.EventManager().EmitEvents( sdk.Events{ sdk.NewEvent( @@ -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()) @@ -288,6 +350,22 @@ func (k Keeper) convertERC20NativeToken( return nil, err } + // Check expected Sender balance after transfer execution + balanceCoinAfter := k.bankKeeper.GetBalance(ctx, receiver, pair.Denom) + if ok := balanceCoinAfter.IsEqual(balanceCoin.Add(coins[0])); !ok { + return nil, sdkerrors.Wrap( + types.ErrInvalidConversionBalance, "invalid coin balance ", + ) + } + // Check expected Receiver balance after transfer execution + tokens := coins[0].Amount.BigInt() + balanceTokenAfter := k.balanceOf(ctx, erc20, contract, sender) + if r := balanceTokenAfter.Cmp(big.NewInt(0).Sub(balanceToken, tokens)); r != 0 { + return nil, sdkerrors.Wrap( + types.ErrInvalidConversionBalance, "invalid token balance ", + ) + } + ctx.EventManager().EmitEvents( sdk.Events{ sdk.NewEvent( @@ -303,3 +381,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 +} diff --git a/x/erc20/types/errors.go b/x/erc20/types/errors.go index 3f0e2cafcc..13606452dd 100644 --- a/x/erc20/types/errors.go +++ b/x/erc20/types/errors.go @@ -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") ) diff --git a/x/erc20/types/interfaces.go b/x/erc20/types/interfaces.go index 7a4c2e43f8..3de6b981a1 100644 --- a/x/erc20/types/interfaces.go +++ b/x/erc20/types/interfaces.go @@ -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 From eb1bd709de8bf03cd474d8da42da01aa7c03d900 Mon Sep 17 00:00:00 2001 From: Daniel Burckhardt Date: Tue, 4 Jan 2022 12:22:00 +0100 Subject: [PATCH 2/4] docs(erc20): Update spec with balance checks --- x/erc20/spec/03_state_transitions.md | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/x/erc20/spec/03_state_transitions.md b/x/erc20/spec/03_state_transitions.md index 4a25871599..e0834fb7b2 100644 --- a/x/erc20/spec/03_state_transitions.md +++ b/x/erc20/spec/03_state_transitions.md @@ -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 @@ -60,6 +64,9 @@ 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 + - Coin balance decreased by amount + - Token balance increased by amount #### 1.2 ERC20 to Coin @@ -68,6 +75,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 @@ -92,6 +102,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 @@ -101,3 +114,6 @@ 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 + - Coin balance decreased by amount + - Token balance increased by amount From 28e873358c76272ac1750781d589e1027a1152f9 Mon Sep 17 00:00:00 2001 From: Daniel Burckhardt Date: Tue, 4 Jan 2022 12:49:45 +0100 Subject: [PATCH 3/4] impr(erc20): Address comments and update spec --- x/erc20/keeper/msg_server.go | 85 +++++++++++++++------------- x/erc20/spec/03_state_transitions.md | 8 +-- 2 files changed, 48 insertions(+), 45 deletions(-) diff --git a/x/erc20/keeper/msg_server.go b/x/erc20/keeper/msg_server.go index 66037a2347..51156894ec 100644 --- a/x/erc20/keeper/msg_server.go +++ b/x/erc20/keeper/msg_server.go @@ -107,6 +107,7 @@ func (k Keeper) ConvertERC20( // 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, @@ -118,7 +119,6 @@ func (k Keeper) convertCoinNativeCoin( coins := sdk.Coins{msg.Coin} erc20 := contracts.ERC20BurnableAndMintableContract.ABI contract := pair.GetERC20Contract() - balanceCoin := k.bankKeeper.GetBalance(ctx, sender, msg.Coin.Denom) balanceToken := k.balanceOf(ctx, erc20, contract, receiver) // Escrow Coins on module account @@ -132,19 +132,14 @@ func (k Keeper) convertCoinNativeCoin( return nil, err } - // Check expected Sender balance after transfer execution - balanceCoinAfter := k.bankKeeper.GetBalance(ctx, sender, pair.Denom) - if ok := balanceCoinAfter.IsEqual(balanceCoin.Sub(coins[0])); !ok { - return nil, sdkerrors.Wrap( - types.ErrInvalidConversionBalance, "invalid coin balance ", - ) - } // Check expected Receiver balance after transfer execution tokens := msg.Coin.Amount.BigInt() balanceTokenAfter := k.balanceOf(ctx, erc20, contract, receiver) - if r := balanceTokenAfter.Cmp(big.NewInt(0).Add(balanceToken, tokens)); r != 0 { - return nil, sdkerrors.Wrap( - types.ErrInvalidConversionBalance, "invalid token balance ", + 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, ) } @@ -169,6 +164,7 @@ func (k Keeper) convertCoinNativeCoin( // - 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, @@ -180,7 +176,6 @@ func (k Keeper) convertCoinNativeERC20( coins := sdk.Coins{msg.Coin} erc20 := contracts.ERC20BurnableAndMintableContract.ABI contract := pair.GetERC20Contract() - balanceCoin := k.bankKeeper.GetBalance(ctx, sender, pair.Denom) balanceToken := k.balanceOf(ctx, erc20, contract, receiver) // Escrow Coins on module account @@ -210,19 +205,14 @@ func (k Keeper) convertCoinNativeERC20( return nil, sdkerrors.Wrap(err, "failed to burn coins") } - // Check expected Sender balance after transfer execution - balanceCoinAfter := k.bankKeeper.GetBalance(ctx, sender, pair.Denom) - if ok := balanceCoinAfter.IsEqual(balanceCoin.Sub(coins[0])); !ok { - return nil, sdkerrors.Wrap( - types.ErrInvalidConversionBalance, "invalid coin balance ", - ) - } // Check expected Receiver balance after transfer execution tokens := msg.Coin.Amount.BigInt() balanceTokenAfter := k.balanceOf(ctx, erc20, contract, receiver) - if r := balanceTokenAfter.Cmp(big.NewInt(0).Add(balanceToken, tokens)); r != 0 { - return nil, sdkerrors.Wrap( - types.ErrInvalidConversionBalance, "invalid token balance ", + 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, ) } @@ -245,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, @@ -269,20 +261,26 @@ func (k Keeper) convertERC20NativeCoin( if err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, receiver, coins); err != nil { return nil, err } - - // Check expected Sender balance after transfer execution + // Check expected Receiver balance after transfer execution balanceCoinAfter := k.bankKeeper.GetBalance(ctx, receiver, pair.Denom) - if ok := balanceCoinAfter.IsEqual(balanceCoin.Add(coins[0])); !ok { - return nil, sdkerrors.Wrap( - types.ErrInvalidConversionBalance, "invalid coin balance ", + 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 Receiver balance after transfer execution + + // Check expected Sender balance after transfer execution tokens := coins[0].Amount.BigInt() balanceTokenAfter := k.balanceOf(ctx, erc20, contract, sender) - if r := balanceTokenAfter.Cmp(big.NewInt(0).Sub(balanceToken, tokens)); r != 0 { - return nil, sdkerrors.Wrap( - types.ErrInvalidConversionBalance, "invalid token balance ", + 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, ) } @@ -306,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, @@ -350,19 +350,26 @@ func (k Keeper) convertERC20NativeToken( return nil, err } - // Check expected Sender balance after transfer execution + // Check expected Receiver balance after transfer execution balanceCoinAfter := k.bankKeeper.GetBalance(ctx, receiver, pair.Denom) - if ok := balanceCoinAfter.IsEqual(balanceCoin.Add(coins[0])); !ok { - return nil, sdkerrors.Wrap( - types.ErrInvalidConversionBalance, "invalid coin balance ", + 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 Receiver balance after transfer execution + + // Check expected Sencer balance after transfer execution tokens := coins[0].Amount.BigInt() balanceTokenAfter := k.balanceOf(ctx, erc20, contract, sender) - if r := balanceTokenAfter.Cmp(big.NewInt(0).Sub(balanceToken, tokens)); r != 0 { - return nil, sdkerrors.Wrap( - types.ErrInvalidConversionBalance, "invalid token balance ", + 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, ) } diff --git a/x/erc20/spec/03_state_transitions.md b/x/erc20/spec/03_state_transitions.md index e0834fb7b2..961e4dfdd2 100644 --- a/x/erc20/spec/03_state_transitions.md +++ b/x/erc20/spec/03_state_transitions.md @@ -64,9 +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 - - Coin balance decreased by amount - - Token balance increased by amount +4. Check if token balance increased by amount #### 1.2 ERC20 to Coin @@ -114,6 +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 - - Coin balance decreased by amount - - Token balance increased by amount +4. Check if token balance increased by amount From 53892a10c89fae39ebe10fd64b831e09675b2af2 Mon Sep 17 00:00:00 2001 From: Daniel Burckhardt Date: Tue, 4 Jan 2022 14:35:24 +0100 Subject: [PATCH 4/4] impr(erc20): Update CHANGELOG --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e72df4a43e..cae9088468 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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