From 6bbe9ea97f32bb2f7d34e9e9b498b14cd5df7cc0 Mon Sep 17 00:00:00 2001 From: Lucas Bertrand Date: Thu, 12 Dec 2024 08:19:31 +0100 Subject: [PATCH] fix: enforce checksum format for asset address in ZRC20 (#3278) * add checksum check * changelog * add migration script * add logs * Update pkg/crypto/evm_address.go Co-authored-by: Dmitry S <11892559+swift1337@users.noreply.github.com> * remove duplicated logs --------- Co-authored-by: Dmitry S <11892559+swift1337@users.noreply.github.com> --- changelog.md | 1 + pkg/crypto/address.go | 12 -- pkg/crypto/address_test.go | 37 ----- pkg/crypto/evm_address.go | 31 ++++ pkg/crypto/evm_address_test.go | 156 ++++++++++++++++++ testutil/sample/fungible.go | 1 + x/crosschain/types/message_whitelist_erc20.go | 22 ++- .../types/message_whitelist_erc20_test.go | 30 +++- x/fungible/keeper/migrator.go | 24 +++ x/fungible/migrations/v3/migrate.go | 30 ++++ x/fungible/migrations/v3/migrate_test.go | 76 +++++++++ x/fungible/module.go | 6 +- 12 files changed, 371 insertions(+), 55 deletions(-) delete mode 100644 pkg/crypto/address.go delete mode 100644 pkg/crypto/address_test.go create mode 100644 pkg/crypto/evm_address.go create mode 100644 pkg/crypto/evm_address_test.go create mode 100644 x/fungible/keeper/migrator.go create mode 100644 x/fungible/migrations/v3/migrate.go create mode 100644 x/fungible/migrations/v3/migrate_test.go diff --git a/changelog.md b/changelog.md index 791af078fe..14baffeb92 100644 --- a/changelog.md +++ b/changelog.md @@ -24,6 +24,7 @@ * [3225](https://github.com/zeta-chain/node/pull/3225) - use separate database file names for btc signet and testnet4 * [3242](https://github.com/zeta-chain/node/pull/3242) - set the `Receiver` of `MsgVoteInbound` to the address pulled from solana memo * [3253](https://github.com/zeta-chain/node/pull/3253) - fix solana inbound version 0 queries and move tss keysign prior to relayer key checking +* [3278](https://github.com/zeta-chain/node/pull/3278) - enforce checksum format for asset address in ZRC20 ## v23.0.0 diff --git a/pkg/crypto/address.go b/pkg/crypto/address.go deleted file mode 100644 index 90876a790f..0000000000 --- a/pkg/crypto/address.go +++ /dev/null @@ -1,12 +0,0 @@ -package crypto - -import ( - "github.com/ethereum/go-ethereum/common" - - "github.com/zeta-chain/node/pkg/constant" -) - -// IsEmptyAddress returns true if the address is empty -func IsEmptyAddress(address common.Address) bool { - return address == (common.Address{}) || address.Hex() == constant.EVMZeroAddress -} diff --git a/pkg/crypto/address_test.go b/pkg/crypto/address_test.go deleted file mode 100644 index db10c5c7cf..0000000000 --- a/pkg/crypto/address_test.go +++ /dev/null @@ -1,37 +0,0 @@ -package crypto - -import ( - "github.com/ethereum/go-ethereum/common" - "github.com/stretchr/testify/require" - "github.com/zeta-chain/node/pkg/constant" - "testing" -) - -func TestIsEmptyAddress(t *testing.T) { - tests := []struct { - name string - address common.Address - want bool - }{ - { - name: "empty address", - address: common.Address{}, - want: true, - }, - { - name: "zero address", - address: common.HexToAddress(constant.EVMZeroAddress), - want: true, - }, - { - name: "non empty address", - address: common.HexToAddress("0x1"), - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - require.EqualValues(t, tt.want, IsEmptyAddress(tt.address)) - }) - } -} diff --git a/pkg/crypto/evm_address.go b/pkg/crypto/evm_address.go new file mode 100644 index 0000000000..803bfca8fd --- /dev/null +++ b/pkg/crypto/evm_address.go @@ -0,0 +1,31 @@ +package crypto + +import ( + "strings" + + "github.com/ethereum/go-ethereum/common" + + "github.com/zeta-chain/node/pkg/constant" +) + +// IsEmptyAddress returns true if the address is empty +func IsEmptyAddress(address common.Address) bool { + return address == (common.Address{}) || address.Hex() == constant.EVMZeroAddress +} + +// IsEVMAddress returns true if the string is an EVM address +// independently of the checksum format +func IsEVMAddress(address string) bool { + return len(address) == 42 && strings.HasPrefix(address, "0x") && common.IsHexAddress(address) +} + +// IsChecksumAddress returns true if the EVM address string is a valid checksum address +// See https://eips.ethereum.org/EIPS/eip-55 +func IsChecksumAddress(address string) bool { + return address == common.HexToAddress(address).Hex() +} + +// ToChecksumAddress returns the checksum address of the given EVM address +func ToChecksumAddress(address string) string { + return common.HexToAddress(address).Hex() +} diff --git a/pkg/crypto/evm_address_test.go b/pkg/crypto/evm_address_test.go new file mode 100644 index 0000000000..307bdc6866 --- /dev/null +++ b/pkg/crypto/evm_address_test.go @@ -0,0 +1,156 @@ +package crypto + +import ( + "github.com/ethereum/go-ethereum/common" + "github.com/stretchr/testify/require" + "github.com/zeta-chain/node/pkg/constant" + "testing" +) + +func TestIsEmptyAddress(t *testing.T) { + tests := []struct { + name string + address common.Address + want bool + }{ + { + name: "empty address", + address: common.Address{}, + want: true, + }, + { + name: "zero address", + address: common.HexToAddress(constant.EVMZeroAddress), + want: true, + }, + { + name: "non empty address", + address: common.HexToAddress("0x1"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.EqualValues(t, tt.want, IsEmptyAddress(tt.address)) + }) + } +} + +func TestIsEVMAddress(t *testing.T) { + tests := []struct { + name string + address string + want bool + }{ + { + name: "EVM address", + address: "0x5a4f260A7D716c859A2736151cB38b9c58C32c64", + want: true, + }, + { + name: "EVM address with invalid checksum", + address: "0x5a4f260a7D716c859A2736151CB38b9c58C32c64", + want: true, + }, + { + name: "EVM address all lowercase", + address: "0x5a4f260a7d716c859a2736151cb38b9c58c32c64", + want: true, + }, + { + name: "EVM address all uppercase", + address: "0x5A4F260A7D716C859A2736151CB38B9C58C32C64", + want: true, + }, + { + name: "invalid EVM address", + address: "5a4f260A7D716c859A2736151cB38b9c58C32c64", + }, + { + name: "empty address", + address: "", + }, + { + name: "non EVM address", + address: "Gh9ZwEmdLJ8DscKNTkTqPbNwLNNBjuSzaG9Vp2KGtKJr", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.EqualValues(t, tt.want, IsEVMAddress(tt.address)) + }) + } +} + +func TestIsChecksumAddress(t *testing.T) { + tests := []struct { + name string + address string + want bool + }{ + { + name: "checksum address", + address: "0x5a4f260A7D716c859A2736151cB38b9c58C32c64", + want: true, + }, + { + name: "invalid checksum address", + address: "0x5a4f260a7D716c859A2736151CB38b9c58C32c64", + }, + { + name: "all lowercase", + address: "0x5a4f260a7d716c859a2736151cb38b9c58c32c64", + }, + { + name: "all uppercase", + address: "0x5A4F260A7D716C859A2736151CB38B9C58C32C64", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.EqualValues(t, tt.want, IsChecksumAddress(tt.address)) + }) + } +} + +func TestToChecksumAddress(t *testing.T) { + tests := []struct { + name string + address string + want string + }{ + { + name: "checksum address", + address: "0x5a4f260A7D716c859A2736151cB38b9c58C32c64", + want: "0x5a4f260A7D716c859A2736151cB38b9c58C32c64", + }, + { + name: "all lowercase", + address: "0x5a4f260a7d716c859a2736151cb38b9c58c32c64", + want: "0x5a4f260A7D716c859A2736151cB38b9c58C32c64", + }, + { + name: "all uppercase", + address: "0x5A4F260A7D716C859A2736151CB38B9C58C32C64", + want: "0x5a4f260A7D716c859A2736151cB38b9c58C32c64", + }, + { + name: "empty address returns null address", + address: "", + want: "0x0000000000000000000000000000000000000000", + }, + { + name: "non evm address returns null address", + address: "Gh9ZwEmdLJ8DscKNTkTqPbNwLNNBjuSzaG9Vp2KGtKJr", + want: "0x0000000000000000000000000000000000000000", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.EqualValues(t, tt.want, ToChecksumAddress(tt.address)) + }) + } +} diff --git a/testutil/sample/fungible.go b/testutil/sample/fungible.go index 7d4ae17577..4aecea486b 100644 --- a/testutil/sample/fungible.go +++ b/testutil/sample/fungible.go @@ -20,6 +20,7 @@ func ForeignCoins(t *testing.T, address string) types.ForeignCoins { Symbol: StringRandom(r, 32), CoinType: coin.CoinType_ERC20, GasLimit: r.Uint64(), + LiquidityCap: UintInRange(0, 10000000000), } } diff --git a/x/crosschain/types/message_whitelist_erc20.go b/x/crosschain/types/message_whitelist_erc20.go index d27492d7a5..be5ca0e9ca 100644 --- a/x/crosschain/types/message_whitelist_erc20.go +++ b/x/crosschain/types/message_whitelist_erc20.go @@ -4,7 +4,9 @@ import ( cosmoserrors "cosmossdk.io/errors" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + "github.com/pkg/errors" + "github.com/zeta-chain/node/pkg/crypto" "github.com/zeta-chain/node/x/fungible/types" ) @@ -50,10 +52,10 @@ func (msg *MsgWhitelistERC20) GetSignBytes() []byte { func (msg *MsgWhitelistERC20) ValidateBasic() error { _, err := sdk.AccAddressFromBech32(msg.Creator) if err != nil { - return cosmoserrors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid creator address (%s)", err) + return cosmoserrors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid creator address (%s)", err.Error()) } - if msg.Erc20Address == "" { - return cosmoserrors.Wrapf(sdkerrors.ErrInvalidAddress, "empty asset address") + if err := validateAssetAddress(msg.Erc20Address); err != nil { + return cosmoserrors.Wrapf(types.ErrInvalidAddress, "invalid asset address (%s)", err.Error()) } if msg.Decimals > 128 { return cosmoserrors.Wrapf(types.ErrInvalidDecimals, "invalid decimals (%d)", msg.Decimals) @@ -63,3 +65,17 @@ func (msg *MsgWhitelistERC20) ValidateBasic() error { } return nil } + +func validateAssetAddress(address string) error { + if address == "" { + return errors.New("empty asset address") + } + + // if the address is an evm address, check if it is in checksum format + if crypto.IsEVMAddress(address) && !crypto.IsChecksumAddress(address) { + return errors.New("evm address is not in checksum format") + } + + // currently no specific check is implemented for other address format + return nil +} diff --git a/x/crosschain/types/message_whitelist_erc20_test.go b/x/crosschain/types/message_whitelist_erc20_test.go index d5bf845272..4af25c215f 100644 --- a/x/crosschain/types/message_whitelist_erc20_test.go +++ b/x/crosschain/types/message_whitelist_erc20_test.go @@ -71,10 +71,36 @@ func TestMsgWhitelistERC20_ValidateBasic(t *testing.T) { error: true, }, { - name: "valid", + name: "evm asset address with invalid checksum format", msg: types.NewMsgWhitelistERC20( sample.AccAddress(), - sample.EthAddress().Hex(), + "0x5a4f260a7d716c859a2736151cb38b9c58c32c64", + 1, + "name", + "symbol", + 6, + 10, + ), + error: true, + }, + { + name: "valid message with evm asset address", + msg: types.NewMsgWhitelistERC20( + sample.AccAddress(), + "0x5a4f260A7D716c859A2736151cB38b9c58C32c64", + 1, + "name", + "symbol", + 6, + 10, + ), + error: false, + }, + { + name: "valid message with solana asset address", + msg: types.NewMsgWhitelistERC20( + sample.AccAddress(), + "Gh9ZwEmdLJ8DscKNTkTqPbNwLNNBjuSzaG9Vp2KGtKJr", 1, "name", "symbol", diff --git a/x/fungible/keeper/migrator.go b/x/fungible/keeper/migrator.go new file mode 100644 index 0000000000..011b284074 --- /dev/null +++ b/x/fungible/keeper/migrator.go @@ -0,0 +1,24 @@ +package keeper + +import ( + sdk "github.com/cosmos/cosmos-sdk/types" + + v3 "github.com/zeta-chain/node/x/fungible/migrations/v3" +) + +// Migrator is a struct for handling in-place store migrations. +type Migrator struct { + fungibleKeeper Keeper +} + +// NewMigrator returns a new Migrator. +func NewMigrator(keeper Keeper) Migrator { + return Migrator{ + fungibleKeeper: keeper, + } +} + +// Migrate2to3 migrates the store from consensus version 2 to 3 +func (m Migrator) Migrate2to3(ctx sdk.Context) error { + return v3.MigrateStore(ctx, m.fungibleKeeper) +} diff --git a/x/fungible/migrations/v3/migrate.go b/x/fungible/migrations/v3/migrate.go new file mode 100644 index 0000000000..f67ec43d99 --- /dev/null +++ b/x/fungible/migrations/v3/migrate.go @@ -0,0 +1,30 @@ +package v3 + +import ( + sdk "github.com/cosmos/cosmos-sdk/types" + + "github.com/zeta-chain/node/pkg/crypto" + "github.com/zeta-chain/node/x/fungible/types" +) + +type fungibleKeeper interface { + GetAllForeignCoins(ctx sdk.Context) (list []types.ForeignCoins) + SetForeignCoins(ctx sdk.Context, foreignCoins types.ForeignCoins) +} + +// MigrateStore migrates the x/fungible module state from the consensus version 2 to 3 +// It updates all existing address in ForeignCoin to use checksum format if the address is EVM type +func MigrateStore(ctx sdk.Context, fungibleKeeper fungibleKeeper) error { + fcs := fungibleKeeper.GetAllForeignCoins(ctx) + for _, fc := range fcs { + if fc.Asset != "" && crypto.IsEVMAddress(fc.Asset) && !crypto.IsChecksumAddress(fc.Asset) { + checksumAddress := crypto.ToChecksumAddress(fc.Asset) + ctx.Logger().Info("Patching zrc20 asset", "zrc20", fc.Symbol, "old", fc.Asset, "new", checksumAddress) + + fc.Asset = checksumAddress + fungibleKeeper.SetForeignCoins(ctx, fc) + } + } + + return nil +} diff --git a/x/fungible/migrations/v3/migrate_test.go b/x/fungible/migrations/v3/migrate_test.go new file mode 100644 index 0000000000..767c9d7eb2 --- /dev/null +++ b/x/fungible/migrations/v3/migrate_test.go @@ -0,0 +1,76 @@ +package v3_test + +import ( + "github.com/stretchr/testify/require" + keepertest "github.com/zeta-chain/node/testutil/keeper" + "github.com/zeta-chain/node/testutil/sample" + "github.com/zeta-chain/node/x/fungible/migrations/v3" + "github.com/zeta-chain/node/x/fungible/types" + "testing" +) + +func TestMigrateStore(t *testing.T) { + tests := []struct { + name string + assetList []string + expectedList []string + }{ + { + name: "no asset to update", + assetList: []string{}, + expectedList: []string{}, + }, + { + name: "assets to update", + assetList: []string{ + "", + "0x5a4f260a7d716c859a2736151cb38b9c58c32c64", // lowercase + "", + "0xc0ffee254729296a45a3885639AC7E10F9d54979", // checksum + "", + "", + "Gh9ZwEmdLJ8DscKNTkTqPbNwLNNBjuSzaG9Vp2KGtKJr", + "BrS9iNMC3y8J4QTmCz8VrGrYepdoxXYvKxcDMiixwLn5", + "0x999999CF1046E68E36E1AA2E0E07105EDDD1F08E", // uppercase + }, + expectedList: []string{ + "", + "0x5a4f260A7D716c859A2736151cB38b9c58C32c64", + "", + "0xc0ffee254729296a45a3885639AC7E10F9d54979", + "", + "", + "Gh9ZwEmdLJ8DscKNTkTqPbNwLNNBjuSzaG9Vp2KGtKJr", + "BrS9iNMC3y8J4QTmCz8VrGrYepdoxXYvKxcDMiixwLn5", + "0x999999cf1046e68e36E1aA2E0E07105eDDD1f08E", + }, + }, + } + + for _, tt := range tests { + k, ctx, _, _ := keepertest.FungibleKeeper(t) + // Arrange + + // set sample foreign coins + expectedForeignCoins := make([]types.ForeignCoins, len(tt.assetList)) + for i, asset := range tt.assetList { + expectedForeignCoins[i] = sample.ForeignCoins(t, sample.EthAddress().Hex()) + expectedForeignCoins[i].Asset = asset + k.SetForeignCoins(ctx, expectedForeignCoins[i]) + } + + // update for expected list + for i := range tt.assetList { + expectedForeignCoins[i].Asset = tt.expectedList[i] + } + + // Act + err := v3.MigrateStore(ctx, k) + require.NoError(t, err) + + // Assert + actualForeignCoins := k.GetAllForeignCoins(ctx) + require.ElementsMatch(t, expectedForeignCoins, actualForeignCoins) + } + +} diff --git a/x/fungible/module.go b/x/fungible/module.go index 8e87fdbd43..13b7e59e40 100644 --- a/x/fungible/module.go +++ b/x/fungible/module.go @@ -123,6 +123,10 @@ func (am AppModule) Name() string { func (am AppModule) RegisterServices(cfg module.Configurator) { types.RegisterMsgServer(cfg.MsgServer(), keeper.NewMsgServerImpl(am.keeper)) types.RegisterQueryServer(cfg.QueryServer(), am.keeper) + m := keeper.NewMigrator(am.keeper) + if err := cfg.RegisterMigration(types.ModuleName, 2, m.Migrate2to3); err != nil { + panic(err) + } } // RegisterInvariants registers the fungible module's invariants. @@ -153,7 +157,7 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONCodec) json.Raw } // ConsensusVersion implements ConsensusVersion. -func (AppModule) ConsensusVersion() uint64 { return 2 } +func (AppModule) ConsensusVersion() uint64 { return 3 } // BeginBlock executes all ABCI BeginBlock logic respective to the fungible module. func (am AppModule) BeginBlock(_ sdk.Context, _ abci.RequestBeginBlock) {}