diff --git a/CHANGELOG.md b/CHANGELOG.md index 2cf0ceb3..c9ba4fb5 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/x/coredaos/keeper/hooks.go b/x/coredaos/keeper/hooks.go index 3854257b..68728723 100644 --- a/x/coredaos/keeper/hooks.go +++ b/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 } diff --git a/x/coredaos/keeper/hooks_test.go b/x/coredaos/keeper/hooks_test.go index 51496c1c..d1a8746e 100644 --- a/x/coredaos/keeper/hooks_test.go +++ b/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") +}