Problem: missing validation on nft-transfer message fields (crypto-or…

…g-chain#1019) * Problem: missing validation on nft-transfer message fields Solution: - add some validation in check-tx first * Update CHANGELOG.md Signed-off-by: yihuang <huang@crypto.com> * Update app/ante.go Signed-off-by: yihuang <huang@crypto.com> * Update app/ante.go Signed-off-by: yihuang <huang@crypto.com> * Update app/ante.go Signed-off-by: yihuang <huang@crypto.com> * Update app/ante.go Signed-off-by: yihuang <huang@crypto.com> * add test * Update integration_tests/test_nft_transfer.py Signed-off-by: yihuang <huang@crypto.com> * Update integration_tests/test_nft_transfer.py Signed-off-by: yihuang <huang@crypto.com> * fix lint --------- Signed-off-by: yihuang <huang@crypto.com>
esavaris75 · Dec 5, 2023 · ef8f54b · ef8f54b
1 parent d71c53c
commit ef8f54b
Show file tree

Hide file tree

Showing 3 changed files with 75 additions and 0 deletions.
diff --git 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
@@ -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
@@ -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(