From d01eab04b98217290ec49f5277bb0acede1479ec Mon Sep 17 00:00:00 2001 From: zsystm Date: Thu, 25 Jul 2024 14:42:07 +0900 Subject: [PATCH] fix: wrong denom check at RegisterCoin problem: - `coinMetadata.Base` is used as the denom for the token pair - However, the check is incorrectly performed using `coinMetadata.Name`, so it fails to detect if the denom is already registered solution: check using `coinMetadata.Base`, not `.Name` more context: Unfortunately, there is already one case on the mainnet where multiple token pairs exist for the same denom(= ibc/13B6057538B93225F6EBACCB64574C49B2C1568C5AE6CCFE0A039D7DAC02BF29). However, this is not a major issue because only the latest token pair registered in the denom index is actually used. With this patch, such cases will no longer occur. --- x/erc20/keeper/evm_hooks_test.go | 17 +++--- x/erc20/keeper/proposals.go | 2 +- x/erc20/keeper/proposals_test.go | 95 +++++++++++++++++++------------- 3 files changed, 66 insertions(+), 48 deletions(-) diff --git a/x/erc20/keeper/evm_hooks_test.go b/x/erc20/keeper/evm_hooks_test.go index d788c5094..64d3eced2 100644 --- a/x/erc20/keeper/evm_hooks_test.go +++ b/x/erc20/keeper/evm_hooks_test.go @@ -17,15 +17,14 @@ import ( ) const ( - erc20Name = "Coin Token" - erc20Symbol = "CTKN" - erc20Decimals = uint8(18) - cosmosTokenBase = "acoin" - cosmosTokenDisplay = "coin" - cosmosDecimals = uint8(6) - defaultExponent = uint32(18) - zeroExponent = uint32(0) - ibcBase = "ibc/7F1D3FCF4AE79E1554D670D1AD949A9BA4E4A3C76C63093E17E446A46061A7A2" + erc20Name = "Coin Token" + erc20Symbol = "CTKN" + erc20Decimals = uint8(18) + cosmosTokenBase = "acoin" + cosmosDecimals = uint8(6) + defaultExponent = uint32(18) + zeroExponent = uint32(0) + ibcBase = "ibc/7F1D3FCF4AE79E1554D670D1AD949A9BA4E4A3C76C63093E17E446A46061A7A2" ) func (suite *KeeperTestSuite) setupRegisterCoin() (banktypes.Metadata, *types.TokenPair) { diff --git a/x/erc20/keeper/proposals.go b/x/erc20/keeper/proposals.go index e356709dd..91012538f 100644 --- a/x/erc20/keeper/proposals.go +++ b/x/erc20/keeper/proposals.go @@ -34,7 +34,7 @@ func (k Keeper) RegisterCoin( } // Check if denomination is already registered - if k.IsDenomRegistered(ctx, coinMetadata.Name) { + if k.IsDenomRegistered(ctx, coinMetadata.Base) { return nil, errorsmod.Wrapf( types.ErrTokenPairAlreadyExists, "coin denomination already registered: %s", coinMetadata.Name, ) diff --git a/x/erc20/keeper/proposals_test.go b/x/erc20/keeper/proposals_test.go index 64668ff41..5828a14c4 100644 --- a/x/erc20/keeper/proposals_test.go +++ b/x/erc20/keeper/proposals_test.go @@ -20,24 +20,32 @@ import ( inflationtypes "github.com/Canto-Network/Canto/v7/x/inflation/types" ) +// The following constants are based on query result of mainnet denoms metadata query. +const ( + gravitonIBCDenom = "ibc/FC9D92EC12BC974E8B6179D411351524CD5C2EBC3CE29D5BA856414FEFA47093" + gravitonDenom = "graviton" + gravitonName = "Graviton" + gravitonSymbol = "GRAV" +) + func (suite KeeperTestSuite) TestRegisterCoin() { metadata := banktypes.Metadata{ Description: "description", - Base: cosmosTokenBase, + Base: gravitonIBCDenom, // NOTE: Denom units MUST be increasing DenomUnits: []*banktypes.DenomUnit{ { - Denom: cosmosTokenBase, + Denom: gravitonIBCDenom, Exponent: 0, }, { - Denom: cosmosTokenDisplay, + Denom: gravitonDenom, Exponent: defaultExponent, }, }, - Name: cosmosTokenBase, - Symbol: erc20Symbol, - Display: cosmosTokenDisplay, + Name: gravitonName, + Symbol: gravitonSymbol, + Display: gravitonDenom, } testCases := []struct { @@ -46,7 +54,7 @@ func (suite KeeperTestSuite) TestRegisterCoin() { expPass bool }{ { - "conversion is disabled globally", + "fail: conversion is disabled globally", func() { params := types.DefaultParams() params.EnableErc20 = false @@ -55,41 +63,42 @@ func (suite KeeperTestSuite) TestRegisterCoin() { false, }, { - "denom already registered", + "fail: denom already registered", func() { regPair := types.NewTokenPair(tests.GenerateAddress(), metadata.Base, true, types.OWNER_MODULE) + suite.app.Erc20Keeper.SetTokenPair(suite.ctx, regPair) suite.app.Erc20Keeper.SetTokenPairIdByDenom(suite.ctx, regPair.Denom, regPair.GetID()) suite.Commit() }, false, }, { - "token doesn't have supply", + "fail :token doesn't have supply", func() { }, false, }, { - "metadata different that stored", + "fail: metadata different that stored", func() { - metadata.Base = cosmosTokenBase + metadata.Base = gravitonDenom validMetadata := banktypes.Metadata{ Description: "description", - Base: cosmosTokenBase, + Base: gravitonIBCDenom, // NOTE: Denom units MUST be increasing DenomUnits: []*banktypes.DenomUnit{ { - Denom: cosmosTokenBase, + Denom: gravitonIBCDenom, Exponent: 0, }, { - Denom: cosmosTokenDisplay, - Exponent: uint32(18), + Denom: gravitonDenom, + Exponent: defaultExponent, }, }, - Name: erc20Name, - Symbol: erc20Symbol, - Display: cosmosTokenDisplay, + Name: "different name", + Symbol: gravitonSymbol, + Display: gravitonDenom, } err := suite.app.BankKeeper.MintCoins(suite.ctx, inflationtypes.ModuleName, sdk.Coins{sdk.NewInt64Coin(validMetadata.Base, 1)}) @@ -108,16 +117,7 @@ func (suite KeeperTestSuite) TestRegisterCoin() { false, }, { - "evm denom registration - CANTO", - func() { - metadata.Base = "CANTO" - err := suite.app.BankKeeper.MintCoins(suite.ctx, inflationtypes.ModuleName, sdk.Coins{sdk.NewInt64Coin(metadata.Base, 1)}) - suite.Require().NoError(err) - }, - false, - }, - { - "evm denom registration - aCANTO", + "fail: evm denom registration - aCANTO", func() { metadata.Base = "aCANTO" err := suite.app.BankKeeper.MintCoins(suite.ctx, inflationtypes.ModuleName, sdk.Coins{sdk.NewInt64Coin(metadata.Base, 1)}) @@ -126,7 +126,7 @@ func (suite KeeperTestSuite) TestRegisterCoin() { false, }, { - "evm denom registration - wCANTO", + "fail: evm denom registration - wCANTO", func() { metadata.Base = "wCANTO" err := suite.app.BankKeeper.MintCoins(suite.ctx, inflationtypes.ModuleName, sdk.Coins{sdk.NewInt64Coin(metadata.Base, 1)}) @@ -137,16 +137,16 @@ func (suite KeeperTestSuite) TestRegisterCoin() { { "ok", func() { - metadata.Base = cosmosTokenBase + metadata.Base = gravitonIBCDenom err := suite.app.BankKeeper.MintCoins(suite.ctx, inflationtypes.ModuleName, sdk.Coins{sdk.NewInt64Coin(metadata.Base, 1)}) suite.Require().NoError(err) }, true, }, { - "force fail evm", + "fail: force fail evm", func() { - metadata.Base = cosmosTokenBase + metadata.Base = gravitonDenom err := suite.app.BankKeeper.MintCoins(suite.ctx, inflationtypes.ModuleName, sdk.Coins{sdk.NewInt64Coin(metadata.Base, 1)}) suite.Require().NoError(err) @@ -160,14 +160,33 @@ func (suite KeeperTestSuite) TestRegisterCoin() { false, }, { - "force delete module account evm", + "fail: force delete module account evm", + func() { + metadata.Base = gravitonIBCDenom + err := suite.app.BankKeeper.MintCoins(suite.ctx, inflationtypes.ModuleName, sdk.Coins{sdk.NewInt64Coin(metadata.Base, 1)}) + suite.Require().NoError(err) + }, + false, + }, + { + "fail: token pair already exists with same denom", func() { - metadata.Base = cosmosTokenBase + metadata.Base = gravitonIBCDenom err := suite.app.BankKeeper.MintCoins(suite.ctx, inflationtypes.ModuleName, sdk.Coins{sdk.NewInt64Coin(metadata.Base, 1)}) suite.Require().NoError(err) - acc := suite.app.AccountKeeper.GetAccount(suite.ctx, types.ModuleAddress.Bytes()) - suite.app.AccountKeeper.RemoveAccount(suite.ctx, acc) + tokenPair, err := suite.app.Erc20Keeper.RegisterCoin(suite.ctx, metadata) + suite.Require().NoError(err) + + // check token pair is stored + suite.Require().Equal(types.OWNER_MODULE, tokenPair.ContractOwner) + suite.Require().Equal(metadata.Base, tokenPair.Denom) + suite.Require().Equal(true, tokenPair.Enabled) + + // check indexes are stored + id := tokenPair.GetID() + suite.Require().Equal(id, suite.app.Erc20Keeper.GetTokenPairIdByDenom(suite.ctx, tokenPair.Denom)) + suite.Require().Equal(id, suite.app.Erc20Keeper.GetTokenPairIdByERC20Addr(suite.ctx, common.HexToAddress(tokenPair.Erc20Address))) }, false, }, @@ -183,9 +202,9 @@ func (suite KeeperTestSuite) TestRegisterCoin() { expPair := &types.TokenPair{ Erc20Address: "0x80b5a32E4F032B2a058b4F29EC95EEfEEB87aDcd", - Denom: "acoin", + Denom: gravitonIBCDenom, Enabled: true, - ContractOwner: 1, + ContractOwner: types.OWNER_MODULE, } if tc.expPass {