Repository: celestiaorg/celestia-app

Issue:
Questions about MsgExec inside MsgExec
## Context

https://github.com/celestiaorg/celestia-app/pull/3555#discussion_r1635492368
https://github.com/celestiaorg/celestia-app/issues/3586

## Problem

It is possible to nest an authz MsgExec message inside another authz MsgExec message. 

## Questions

1. Is this actually problematic?
2. Are there valid use cases to nest a MsgExec inside another MsgExec?
3. How many layers of nesting do valid use cases use? 
4. Should we reject transactions (i.e. via a new antehandler) that have multiple layers of nesting?



Hints from issue comments:
@rootulp: My opinions:

>Is this actually problematic?

It can be problematic because nesting messages inside MsgExec circumvents a lot of existing antehandlers that only check the top level messages in the tx.

>Should we reject transactions (i.e. via a new antehandler) that have multiple layers of nesting?

Yes. 

Solution patch:
diff --git a/app/ante/ante.go b/app/ante/ante.go
index c82313b6..57656476 100644
--- a/app/ante/ante.go
+++ b/app/ante/ante.go
@@ -62,6 +62,9 @@ func NewAnteHandler(
 		// account sequence number of the signer.
 		// Note: does not consume gas from the gas meter.
 		ante.NewSigVerificationDecorator(accountKeeper, signModeHandler),
+		// Ensure that the tx does not contain a MsgExec with a nested MsgExec
+		// or MsgPayForBlobs.
+		NewMsgExecDecorator(),
 		// Ensure that the tx's gas limit is > the gas consumed based on the blob size(s).
 		// Contract: must be called after all decorators that consume gas.
 		// Note: does not consume gas from the gas meter.
diff --git a/app/ante/msg_exec.go b/app/ante/msg_exec.go
new file mode 100644
index 00000000..a21d8a8a
--- /dev/null
+++ b/app/ante/msg_exec.go
@@ -0,0 +1,43 @@
+package ante
+
+import (
+	blobtypes "github.com/celestiaorg/celestia-app/v3/x/blob/types"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
+	"github.com/cosmos/cosmos-sdk/x/authz"
+)
+
+var _ sdk.AnteDecorator = MsgExecDecorator{}
+
+// MsgExecDecorator ensures that the tx does not contain a MsgExec with a
+// nested MsgExec or MsgPayForBlobs.
+type MsgExecDecorator struct{}
+
+func NewMsgExecDecorator() *MsgExecDecorator {
+	return &MsgExecDecorator{}
+}
+
+func (mgk MsgExecDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) {
+	// Only apply this decorator in checkTx.
+	if !ctx.IsCheckTx() {
+		return next(ctx, tx, simulate)
+	}
+
+	for _, msg := range tx.GetMsgs() {
+		if msgExec, ok := msg.(*authz.MsgExec); ok {
+			nestedMsgs, err := msgExec.GetMessages()
+			if err != nil {
+				return ctx, err
+			}
+			for _, nestedMsg := range nestedMsgs {
+				if _, ok := nestedMsg.(*authz.MsgExec); ok {
+					return ctx, sdkerrors.ErrNotSupported.Wrapf("MsgExec inside MsgExec is not supported")
+				}
+				if _, ok := nestedMsg.(*blobtypes.MsgPayForBlobs); ok {
+					return ctx, sdkerrors.ErrNotSupported.Wrapf("MsgPayForBlobs inside MsgExec is not supported")
+				}
+			}
+		}
+	}
+	return next(ctx, tx, simulate)
+}



Test patch:
diff --git a/app/ante/msg_exec_test.go b/app/ante/msg_exec_test.go
new file mode 100644
index 00000000..5c644d0b
--- /dev/null
+++ b/app/ante/msg_exec_test.go
@@ -0,0 +1,76 @@
+package ante_test
+
+import (
+	"testing"
+
+	"github.com/celestiaorg/celestia-app/v3/app"
+	"github.com/celestiaorg/celestia-app/v3/app/ante"
+	"github.com/celestiaorg/celestia-app/v3/app/encoding"
+	blobtypes "github.com/celestiaorg/celestia-app/v3/x/blob/types"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
+	"github.com/cosmos/cosmos-sdk/x/authz"
+	banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
+	"github.com/stretchr/testify/require"
+	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
+)
+
+func TestMsgExecDecorator(t *testing.T) {
+	msgExec := authz.NewMsgExec(sdk.AccAddress{}, []sdk.Msg{&banktypes.MsgSend{}})
+	nestedMsgExec := authz.NewMsgExec(sdk.AccAddress{}, []sdk.Msg{&msgExec})
+	nestedMsgPayForBlobs := authz.NewMsgExec(sdk.AccAddress{}, []sdk.Msg{&blobtypes.MsgPayForBlobs{}})
+
+	tests := []struct {
+		name      string
+		msg       sdk.Msg
+		isCheckTx bool
+		wantErr   error
+	}{
+		{
+			name:      "Accept msgExec",
+			msg:       &msgExec,
+			isCheckTx: true,
+			wantErr:   nil,
+		},
+		{
+			name:      "Reject nestedMsgExec",
+			msg:       &nestedMsgExec,
+			isCheckTx: true,
+			wantErr:   sdkerrors.ErrNotSupported,
+		},
+		{
+			name:      "Reject nestedMsgPayForBlobs",
+			msg:       &nestedMsgPayForBlobs,
+			isCheckTx: true,
+			wantErr:   sdkerrors.ErrNotSupported,
+		},
+		{
+			name:      "Accept nestedMsgExec if isCheckTx is false",
+			msg:       &nestedMsgPayForBlobs,
+			isCheckTx: false,
+			wantErr:   nil,
+		},
+	}
+
+	decorator := ante.NewMsgExecDecorator()
+	cdc := encoding.MakeConfig(app.ModuleEncodingRegisters...)
+	anteHandler := sdk.ChainAnteDecorators(decorator)
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			// Setup
+			ctx := sdk.NewContext(nil, tmproto.Header{}, tc.isCheckTx, nil)
+			txBuilder := cdc.TxConfig.NewTxBuilder()
+			require.NoError(t, txBuilder.SetMsgs(tc.msg))
+
+			// Run the ante handler
+			_, err := anteHandler(ctx, txBuilder.GetTx(), false)
+			if tc.wantErr != nil {
+				require.Error(t, err)
+				require.ErrorIs(t, err, tc.wantErr)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}