From 34650448bb4d3419befcab956c3a01d3a293ba3a Mon Sep 17 00:00:00 2001 From: HuangYi Date: Tue, 5 Dec 2023 14:44:17 +0800 Subject: [PATCH 01/10] Problem: missing validation on nft-transfer message fields Solution: - add some validation in check-tx first --- CHANGELOG.md | 1 + app/ante.go | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 534e8dbd9..7b0c7ff99 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - [#1004](https://github.com/crypto-org-chain/chain-main/pull/1004) Update rocksdb dependency to 8.1.1. - [#1009](https://github.com/crypto-org-chain/chain-main/pull/1009) Update memiavl to c575f4797ca4, update cosmos-sdk to v0.46.15. - [#1010](https://github.com/crypto-org-chain/chain-main/pull/1010) Bump librocksdb to 8.5.3 +- [#]() Limit the length of NFTTransfer fields. ### Bug Fixes diff --git a/app/ante.go b/app/ante.go index 593f28805..1152042b9 100644 --- a/app/ante.go +++ b/app/ante.go @@ -7,6 +7,7 @@ import ( "github.com/cosmos/cosmos-sdk/x/auth/ante" ibcante "github.com/cosmos/ibc-go/v5/modules/core/ante" "github.com/cosmos/ibc-go/v5/modules/core/keeper" + nfttypes "github.com/crypto-org-chain/chain-main/v4/x/nft-transfer/types" ) // HandlerOptions extend the SDK's AnteHandler options by requiring the IBC @@ -39,6 +40,7 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) { ante.NewValidateBasicDecorator(), ante.NewTxTimeoutHeightDecorator(), ante.NewValidateMemoDecorator(options.AccountKeeper), + NewValidateMsgTransferDecorator(), ante.NewConsumeGasForTxSizeDecorator(options.AccountKeeper), ante.NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, options.TxFeeChecker), // SetPubKeyDecorator must be called before all signature verification decorators @@ -52,3 +54,52 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) { return sdk.ChainAnteDecorators(anteDecorators...), nil } + +const ( + // values chosen arbitrarily + MaxClassIDLength = 256 + MaxTokenIds = 256 + MaxTokenIDLength = 256 + MaximumReceiverLength = 2048 +) + +// ValidateMsgTransferDecorator is a temporary decorator that limit the field length of MsgTransfer message. +type ValidateMsgTransferDecorator struct{} + +func NewValidateMsgTransferDecorator() ValidateMsgTransferDecorator { + return ValidateMsgTransferDecorator{} +} + +func (vtd ValidateMsgTransferDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { + if !ctx.IsCheckTx() { + return next(ctx, tx, simulate) + } + + msgs := tx.GetMsgs() + for _, msg := range msgs { + transfer, ok := msg.(*nfttypes.MsgTransfer) + if !ok { + continue + } + + if len(transfer.ClassId) > MaxClassIDLength { + return ctx, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "class id length must be less than %d", MaxClassIDLength) + } + + if len(transfer.TokenIds) > MaxClassIDLength { + return ctx, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "token id length must be less than %d", MaxClassIDLength) + } + + for _, tokenID := range transfer.TokenIds { + if len(tokenID) > MaxTokenIDLength { + return ctx, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "token id length must be less than %d", MaxTokenIDLength) + } + } + + if len(transfer.Receiver) > MaximumReceiverLength { + return ctx, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "receiver length must be less than %d", MaximumReceiverLength) + } + } + + return next(ctx, tx, simulate) +} From 2353e22278e610bf05367c82a06b54302ab11e8d Mon Sep 17 00:00:00 2001 From: yihuang Date: Tue, 5 Dec 2023 14:48:25 +0800 Subject: [PATCH 02/10] Update CHANGELOG.md Signed-off-by: yihuang --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b0c7ff99..573bfccbc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ - [#1004](https://github.com/crypto-org-chain/chain-main/pull/1004) Update rocksdb dependency to 8.1.1. - [#1009](https://github.com/crypto-org-chain/chain-main/pull/1009) Update memiavl to c575f4797ca4, update cosmos-sdk to v0.46.15. - [#1010](https://github.com/crypto-org-chain/chain-main/pull/1010) Bump librocksdb to 8.5.3 -- [#]() Limit the length of NFTTransfer fields. +- [#1019](https://github.com/crypto-org-chain/chain-main/pull/1019) Limit the length of NFTTransfer fields. ### Bug Fixes From 04da9915a855ad6f6b3be86eb0171b09bbae52d5 Mon Sep 17 00:00:00 2001 From: yihuang Date: Tue, 5 Dec 2023 14:48:47 +0800 Subject: [PATCH 03/10] Update app/ante.go Signed-off-by: yihuang --- app/ante.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/ante.go b/app/ante.go index 1152042b9..0986ceb7a 100644 --- a/app/ante.go +++ b/app/ante.go @@ -57,9 +57,9 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) { const ( // values chosen arbitrarily - MaxClassIDLength = 256 - MaxTokenIds = 256 - MaxTokenIDLength = 256 + MaxClassIDLength = 2048 + MaxTokenIds = 2048 + MaxTokenIDLength = 2048 MaximumReceiverLength = 2048 ) From 7c85b446ab05085a3a95a05016642d26982b8e4b Mon Sep 17 00:00:00 2001 From: yihuang Date: Tue, 5 Dec 2023 14:51:05 +0800 Subject: [PATCH 04/10] Update app/ante.go Signed-off-by: yihuang --- app/ante.go | 1 + 1 file changed, 1 insertion(+) diff --git a/app/ante.go b/app/ante.go index 0986ceb7a..93c505257 100644 --- a/app/ante.go +++ b/app/ante.go @@ -71,6 +71,7 @@ func NewValidateMsgTransferDecorator() ValidateMsgTransferDecorator { } func (vtd ValidateMsgTransferDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { + // avoid breaking consensus if !ctx.IsCheckTx() { return next(ctx, tx, simulate) } From 6b66e0b64c9bde008cd3c6601c7eabef1232b4e7 Mon Sep 17 00:00:00 2001 From: yihuang Date: Tue, 5 Dec 2023 14:51:52 +0800 Subject: [PATCH 05/10] Update app/ante.go Signed-off-by: yihuang --- app/ante.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/ante.go b/app/ante.go index 93c505257..a8047a8dd 100644 --- a/app/ante.go +++ b/app/ante.go @@ -87,8 +87,8 @@ func (vtd ValidateMsgTransferDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, s return ctx, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "class id length must be less than %d", MaxClassIDLength) } - if len(transfer.TokenIds) > MaxClassIDLength { - return ctx, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "token id length must be less than %d", MaxClassIDLength) + if len(transfer.TokenIds) > MaxTokenIds { + return ctx, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "token id length must be less than %d", MaxTokenIds) } for _, tokenID := range transfer.TokenIds { From 10569290b4c4f00d8cb1322ba32e675c24b84066 Mon Sep 17 00:00:00 2001 From: yihuang Date: Tue, 5 Dec 2023 15:14:10 +0800 Subject: [PATCH 06/10] Update app/ante.go Signed-off-by: yihuang --- app/ante.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/ante.go b/app/ante.go index a8047a8dd..a11985862 100644 --- a/app/ante.go +++ b/app/ante.go @@ -58,7 +58,7 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) { const ( // values chosen arbitrarily MaxClassIDLength = 2048 - MaxTokenIds = 2048 + MaxTokenIds = 256 MaxTokenIDLength = 2048 MaximumReceiverLength = 2048 ) From eaf3e136c819a33ffb336716b54c6f0fcea167c0 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Tue, 5 Dec 2023 15:12:17 +0800 Subject: [PATCH 07/10] add test --- integration_tests/test_nft_transfer.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/integration_tests/test_nft_transfer.py b/integration_tests/test_nft_transfer.py index 2408b9b9b..2c9e2f8ae 100644 --- a/integration_tests/test_nft_transfer.py +++ b/integration_tests/test_nft_transfer.py @@ -96,6 +96,28 @@ def test_nft_transfer(cluster): == "/chainmain.nft.v1.MsgMintNFT" ) + # nft transfer that's supposed to fail, exceeds max receiver length + rsp = json.loads( + cli_src.raw( + "tx", + "nft-transfer", + "transfer", + "nft", + src_channel, + 'a'*2049, + denomid, + tokenid, + "-y", + home=cli_src.data_dir, + from_=addr_src, + keyring_backend="test", + chain_id=cli_src.chain_id, + node=cli_src.node_rpc, + ) + ) + assert rsp["code"] != 0 + assert "receiver length must be less than 2048" in rsp["raw_log"] + # transfer nft on mid-destination chain rsp = json.loads( cli_src.raw( From dc70641ab07db88a0a5e72e0069b96cf75e2b50f Mon Sep 17 00:00:00 2001 From: yihuang Date: Tue, 5 Dec 2023 15:17:27 +0800 Subject: [PATCH 08/10] Update integration_tests/test_nft_transfer.py Signed-off-by: yihuang --- integration_tests/test_nft_transfer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration_tests/test_nft_transfer.py b/integration_tests/test_nft_transfer.py index 2c9e2f8ae..6beb86904 100644 --- a/integration_tests/test_nft_transfer.py +++ b/integration_tests/test_nft_transfer.py @@ -104,7 +104,7 @@ def test_nft_transfer(cluster): "transfer", "nft", src_channel, - 'a'*2049, + "a"*2049, denomid, tokenid, "-y", From 68ddb20538ad09c025dc8441f997b79fd7560c65 Mon Sep 17 00:00:00 2001 From: yihuang Date: Tue, 5 Dec 2023 15:40:47 +0800 Subject: [PATCH 09/10] Update integration_tests/test_nft_transfer.py Signed-off-by: yihuang --- integration_tests/test_nft_transfer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration_tests/test_nft_transfer.py b/integration_tests/test_nft_transfer.py index 6beb86904..6cf915043 100644 --- a/integration_tests/test_nft_transfer.py +++ b/integration_tests/test_nft_transfer.py @@ -104,7 +104,7 @@ def test_nft_transfer(cluster): "transfer", "nft", src_channel, - "a"*2049, + "a" * 2049, denomid, tokenid, "-y", From e5369f8370d32a786fde9837eecf4519f91212d2 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Tue, 5 Dec 2023 16:37:07 +0800 Subject: [PATCH 10/10] fix lint --- app/ante.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/ante.go b/app/ante.go index a11985862..bf1849a8e 100644 --- a/app/ante.go +++ b/app/ante.go @@ -71,7 +71,7 @@ func NewValidateMsgTransferDecorator() ValidateMsgTransferDecorator { } func (vtd ValidateMsgTransferDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { - // avoid breaking consensus + // avoid breaking consensus if !ctx.IsCheckTx() { return next(ctx, tx, simulate) } @@ -84,21 +84,21 @@ func (vtd ValidateMsgTransferDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, s } if len(transfer.ClassId) > MaxClassIDLength { - return ctx, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "class id length must be less than %d", MaxClassIDLength) + return ctx, newsdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "class id length must be less than %d", MaxClassIDLength) } if len(transfer.TokenIds) > MaxTokenIds { - return ctx, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "token id length must be less than %d", MaxTokenIds) + return ctx, newsdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "token id length must be less than %d", MaxTokenIds) } for _, tokenID := range transfer.TokenIds { if len(tokenID) > MaxTokenIDLength { - return ctx, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "token id length must be less than %d", MaxTokenIDLength) + return ctx, newsdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "token id length must be less than %d", MaxTokenIDLength) } } if len(transfer.Receiver) > MaximumReceiverLength { - return ctx, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "receiver length must be less than %d", MaximumReceiverLength) + return ctx, newsdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "receiver length must be less than %d", MaximumReceiverLength) } }