feat(coredaos): reject proposals that delegate x/coredaos MsgUpdateParams via authz

CHANGELOG.md

@@ -18,6 +18,7 @@
 
 - Migrate `x/coredaos` away from atomone `x/gov` wrapper [#353](https://github.com/atomone-hub/atomone/pull/353)
 - Move oversightDAO update bundling prevention from the ante to a coredaos gov hook [#354](https://github.com/atomone-hub/atomone/pull/354)
+- Reject proposals that delegate `x/coredaos` `MsgUpdateParams` via authz [358](https://github.com/atomone-hub/atomone/pull/358)
 
 ## v4.0.0
 

x/coredaos/keeper/hooks.go

@@ -8,6 +8,7 @@ import (
 
 	cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/cosmos/cosmos-sdk/x/authz"
 	govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
 
@@ -104,19 +105,61 @@ func (k Keeper) GovHooks() Hooks {
 	return Hooks{k}
 }
 
-// AfterProposalSubmission rejects a proposal that bundles a coredaos MsgUpdateParams
-// changing the oversight DAO address together with other messages. Self-executing
-// authz.MsgExec wrappers are rejected upstream in gov's SubmitProposal, so only
-// top-level messages need inspection here.
+// AfterProposalSubmission enforces two coredaos invariants on a submitted proposal:
+//
+//  1. coredaos MsgUpdateParams may never be delegated via authz. Its authority is the
+//     governance account, so delegating it to any other account would hand governance-only
+//     power to that account, which is unconsitutional as it provides control over core DAOs
+//     to a delegate. Inspecting only top-level MsgGrant messages catches every such
+//     delegation, because a gov->grantee grant for this message can arise nowhere else:
+//     - It can only be created inside a governance proposal. A MsgGrant is signed by its
+//     granter, and here the granter must be the gov account (authz keys the grant by the
+//     executed message's signer, which is MsgUpdateParams.Authority == gov). The gov
+//     module account has no key and cannot sign a transaction, so no standalone tx can
+//     create the grant; proposal execution is the only path, and it is what this hook sees.
+//     - Within a proposal it must be a top-level message. A MsgGrant nested inside an
+//     authz.MsgExec is either self-executing (grantee == gov), which gov's SubmitProposal
+//     already rejects, or requires a pre-existing grant to be dispatched, i.e. is circular.
+//
+//  2. A proposal that changes the oversight DAO address may not be bundled with other
+//     messages. Only top-level messages are inspected: a MsgUpdateParams hidden below the top
+//     level (e.g. inside a non-self-executing authz.MsgExec) could only ever take effect via
+//     an authz grant, which invariant (1) has already made impossible to create.
 func (h Hooks) AfterProposalSubmission(ctx context.Context, proposalID uint64) error {
-	params := h.k.GetParams(ctx)
-	if params.OversightDaoAddress == "" {
-		return nil
-	}
 	proposal, err := h.k.govKeeper.Proposals.Get(ctx, proposalID)
 	if err != nil {
 		return nil // proposal not found; nothing to enforce
 	}
+
+	updateParamsTypeURL := sdk.MsgTypeURL(&types.MsgUpdateParams{})
+
+	// (1) Reject any authz grant that would delegate coredaos MsgUpdateParams. This is
+	// unconditional: it does not depend on whether an oversight DAO is currently set, because
+	// MsgUpdateParams governs every coredaos parameter.
+	for _, anyMsg := range proposal.Messages {
+		var msg sdk.Msg
+		if err := h.k.cdc.UnpackAny(anyMsg, &msg); err != nil {
+			continue
+		}
+		grant, ok := msg.(*authz.MsgGrant)
+		if !ok {
+			continue
+		}
+		var authorization authz.Authorization
+		if err := h.k.cdc.UnpackAny(grant.Grant.Authorization, &authorization); err != nil {
+			continue
+		}
+		if authorization.MsgTypeURL() == updateParamsTypeURL {
+			return errorsmod.Wrap(atomoneerrors.ErrUnauthorized,
+				"coredaos MsgUpdateParams authority cannot be delegated via authz")
+		}
+	}
+
+	// (2) Reject bundling an oversight-DAO address change with other messages.
+	params := h.k.GetParams(ctx)
+	if params.OversightDaoAddress == "" {
+		return nil
+	}
 	if len(proposal.Messages) <= 1 {
 		return nil // bundling requires more than one message
 	}

x/coredaos/keeper/hooks_test.go

@@ -8,9 +8,12 @@ import (
 
 	tmproto "github.com/cometbft/cometbft/proto/tendermint/types"
 
+	codectypes "github.com/cosmos/cosmos-sdk/codec/types"
 	simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdktx "github.com/cosmos/cosmos-sdk/types/tx"
+	"github.com/cosmos/cosmos-sdk/x/authz"
+	banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
 	govv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1"
 
 	"github.com/atomone-hub/atomone/app/helpers"
@@ -107,3 +110,116 @@ func TestGovHookWiredRejectsBundledOversightChange(t *testing.T) {
 		require.NotContains(t, err.Error(), "cannot be bundled")
 	}
 }
+
+// TestGovHookRejectsUpdateParamsDelegation checks the constitutional invariant that coredaos
+// MsgUpdateParams may never be delegated via authz. The check targets the grant creation (the
+// only way a gov->X grant for this msg can come to exist) and is unconditional: it holds even
+// when no oversight DAO is set, because MsgUpdateParams governs all coredaos params.
+func TestGovHookRejectsUpdateParamsDelegation(t *testing.T) {
+	app := helpers.Setup(t)
+	ctx := app.NewUncachedContext(true, tmproto.Header{Time: time.Now()})
+	hooks := app.CoreDaosKeeper.GovHooks()
+
+	govAddr := govModuleAddr()
+	grantee := simtestutil.CreateRandomAccounts(1)[0]
+	extDuration := time.Hour
+
+	store := func(id uint64, msgs ...sdk.Msg) {
+		anys, err := sdktx.SetMsgs(msgs)
+		require.NoError(t, err)
+		require.NoError(t, app.GovKeeper.SetProposal(ctx, govv1.Proposal{Id: id, Messages: anys, Status: govv1.StatusVotingPeriod}))
+	}
+
+	updateParamsGrant, err := authz.NewMsgGrant(sdk.MustAccAddressFromBech32(govAddr), grantee,
+		authz.NewGenericAuthorization(sdk.MsgTypeURL(&types.MsgUpdateParams{})), nil)
+	require.NoError(t, err)
+	benignGrant, err := authz.NewMsgGrant(sdk.MustAccAddressFromBech32(govAddr), grantee,
+		authz.NewGenericAuthorization(sdk.MsgTypeURL(&banktypes.MsgSend{})), nil)
+	require.NoError(t, err)
+
+	// Oversight DAO left unset on purpose: the non-delegation rule is unconditional.
+	require.NoError(t, app.CoreDaosKeeper.Params.Set(ctx, types.Params{VotingPeriodExtensionDuration: &extDuration}))
+
+	store(1, updateParamsGrant)
+	require.ErrorContains(t, hooks.AfterProposalSubmission(ctx, 1), "cannot be delegated")
+
+	// Delegating an unrelated message from gov must still be allowed.
+	store(2, benignGrant)
+	require.NoError(t, hooks.AfterProposalSubmission(ctx, 2))
+}
+
+// TestGovHookRejectsNestedAuthzOversightTakeover is the end-to-end regression test for the
+// nested-authz.MsgExec bypass: a proposal that (1) grants the attacker gov's authority over
+// coredaos.MsgUpdateParams and (2) hides a malicious MsgUpdateParams two authz.MsgExec layers
+// deep. The nested change never appears as a top-level MsgUpdateParams, so it evaded the
+// bundling guard; it is now rejected at submission because the required grant (message 1)
+// cannot be created.
+func TestGovHookRejectsNestedAuthzOversightTakeover(t *testing.T) {
+	app := helpers.Setup(t)
+	ctx := app.NewUncachedContext(true, tmproto.Header{Time: time.Now()})
+
+	govAddr := govModuleAddr()
+	extDuration := time.Hour
+	original := simtestutil.CreateRandomAccounts(1)[0]
+	attacker := simtestutil.CreateRandomAccounts(1)[0]
+	require.NoError(t, app.CoreDaosKeeper.Params.Set(ctx, types.Params{
+		OversightDaoAddress: original.String(), VotingPeriodExtensionDuration: &extDuration,
+	}))
+
+	// Message 1: gov -> attacker grant for coredaos.MsgUpdateParams (the un-hideable linchpin).
+	govGrantsAttacker, err := authz.NewMsgGrant(sdk.MustAccAddressFromBech32(govAddr), attacker,
+		authz.NewGenericAuthorization(sdk.MsgTypeURL(&types.MsgUpdateParams{})), nil)
+	require.NoError(t, err)
+
+	// Message 2: MsgExec(gov) -> MsgExec(attacker) -> MsgUpdateParams{OversightDaoAddress: attacker}.
+	malicious := &types.MsgUpdateParams{Authority: govAddr, Params: types.Params{OversightDaoAddress: attacker.String(), VotingPeriodExtensionDuration: &extDuration}}
+	maliciousAny, err := codectypes.NewAnyWithValue(malicious)
+	require.NoError(t, err)
+	innerExec := &authz.MsgExec{Grantee: attacker.String(), Msgs: []*codectypes.Any{maliciousAny}}
+	innerAny, err := codectypes.NewAnyWithValue(innerExec)
+	require.NoError(t, err)
+	outerExec := &authz.MsgExec{Grantee: govAddr, Msgs: []*codectypes.Any{innerAny}}
+
+	_, err = app.GovKeeper.SubmitProposal(ctx, []sdk.Msg{govGrantsAttacker, outerExec}, "", "title", "summary", attacker)
+	require.Error(t, err, "nested-authz oversight takeover must be rejected at submission")
+	require.ErrorContains(t, err, "cannot be delegated")
+
+	// The oversight DAO must be untouched.
+	require.Equal(t, original.String(), app.CoreDaosKeeper.GetParams(ctx).OversightDaoAddress)
+}
+
+// TestSelfExecWrappedGrantRejectedUpstream covers the one path the coredaos hook does NOT
+// inspect directly: the delegating MsgGrant hidden inside a self-executing authz.MsgExec
+// (grantee == gov at every layer). The coredaos hook only checks top-level MsgGrant messages,
+// so completeness here relies on gov's SubmitProposal rejecting any self-executing MsgExec that
+// reaches a gov-signed leaf (ContainsSelfExecAsAuthority) — and the leaf MsgGrant is gov-signed
+// (granter == gov). This test pins that the upstream guard fires for both single and double
+// wrapping, which is what makes invariant (1) in AfterProposalSubmission sufficient.
+func TestSelfExecWrappedGrantRejectedUpstream(t *testing.T) {
+	app := helpers.Setup(t)
+	ctx := app.NewUncachedContext(true, tmproto.Header{Time: time.Now()})
+
+	govAddr := govModuleAddr()
+	grantee := simtestutil.CreateRandomAccounts(1)[0]
+	proposer := simtestutil.CreateRandomAccounts(1)[0]
+
+	grant, err := authz.NewMsgGrant(sdk.MustAccAddressFromBech32(govAddr), grantee,
+		authz.NewGenericAuthorization(sdk.MsgTypeURL(&types.MsgUpdateParams{})), nil)
+	require.NoError(t, err)
+	grantAny, err := codectypes.NewAnyWithValue(grant)
+	require.NoError(t, err)
+
+	// Single self-exec wrap: MsgExec(gov){ MsgGrant(gov->grantee) }.
+	selfExec := &authz.MsgExec{Grantee: govAddr, Msgs: []*codectypes.Any{grantAny}}
+	_, err = app.GovKeeper.SubmitProposal(ctx, []sdk.Msg{selfExec}, "", "title", "summary", proposer)
+	require.Error(t, err, "self-exec-wrapped grant must be rejected at submission")
+	require.ErrorContains(t, err, "self-executing")
+
+	// Double self-exec wrap: MsgExec(gov){ MsgExec(gov){ MsgGrant(gov->grantee) } }.
+	innerAny, err := codectypes.NewAnyWithValue(selfExec)
+	require.NoError(t, err)
+	doubleExec := &authz.MsgExec{Grantee: govAddr, Msgs: []*codectypes.Any{innerAny}}
+	_, err = app.GovKeeper.SubmitProposal(ctx, []sdk.Msg{doubleExec}, "", "title", "summary", proposer)
+	require.Error(t, err, "double self-exec-wrapped grant must be rejected at submission")
+	require.ErrorContains(t, err, "self-executing")
+}