From ef8f54b4244bc6dd2533117badfe1fc61edcd1ed Mon Sep 17 00:00:00 2001 From: yihuang Date: Tue, 5 Dec 2023 16:51:03 +0800 Subject: [PATCH] Problem: missing validation on nft-transfer message fields (#1019) * Problem: missing validation on nft-transfer message fields Solution: - add some validation in check-tx first * Update CHANGELOG.md Signed-off-by: yihuang * Update app/ante.go Signed-off-by: yihuang * Update app/ante.go Signed-off-by: yihuang * Update app/ante.go Signed-off-by: yihuang * Update app/ante.go Signed-off-by: yihuang * add test * Update integration_tests/test_nft_transfer.py Signed-off-by: yihuang * Update integration_tests/test_nft_transfer.py Signed-off-by: yihuang * fix lint --------- Signed-off-by: yihuang --- CHANGELOG.md | 1 + app/ante.go | 52 ++++++++++++++++++++++++++ integration_tests/test_nft_transfer.py | 22 +++++++++++ 3 files changed, 75 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 534e8dbd9..573bfccbc 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 +- [#1019](https://github.com/crypto-org-chain/chain-main/pull/1019) Limit the length of NFTTransfer fields. ### Bug Fixes diff --git a/app/ante.go b/app/ante.go index 593f28805..bf1849a8e 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,53 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) { return sdk.ChainAnteDecorators(anteDecorators...), nil } + +const ( + // values chosen arbitrarily + MaxClassIDLength = 2048 + MaxTokenIds = 256 + MaxTokenIDLength = 2048 + 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) { + // avoid breaking consensus + 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, newsdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "class id length must be less than %d", MaxClassIDLength) + } + + if len(transfer.TokenIds) > 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, newsdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "token id length must be less than %d", MaxTokenIDLength) + } + } + + if len(transfer.Receiver) > MaximumReceiverLength { + return ctx, newsdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "receiver length must be less than %d", MaximumReceiverLength) + } + } + + return next(ctx, tx, simulate) +} diff --git a/integration_tests/test_nft_transfer.py b/integration_tests/test_nft_transfer.py index 2408b9b9b..6cf915043 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(