diff --git a/CHANGELOG.md b/CHANGELOG.md index 926156e5..2cf0ceb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,8 @@ ### IMPROVEMENTS -- migrate `x/coredaos` away from atomone `x/gov` wrapper [#353](https://github.com/atomone-hub/atomone/pull/353) +- 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) ## v4.0.0 diff --git a/ante/ante.go b/ante/ante.go index adeaab4a..82aed13b 100644 --- a/ante/ante.go +++ b/ante/ante.go @@ -28,7 +28,6 @@ type HandlerOptions struct { PhotonKeeper *photonkeeper.Keeper TxFeeChecker ante.TxFeeChecker DynamicfeeKeeper *dynamicfeekeeper.Keeper - CoreDAOsKeeper CoredaosParamsGetter } func NewAnteHandler(opts HandlerOptions) (sdk.AnteHandler, error) { @@ -53,9 +52,6 @@ func NewAnteHandler(opts HandlerOptions) (sdk.AnteHandler, error) { if opts.DynamicfeeKeeper == nil { return nil, errorsmod.Wrap(atomoneerrors.ErrNotFound, "dynamicfee keeper is required for AnteHandler") } - if opts.CoreDAOsKeeper == nil { - return nil, errorsmod.Wrap(atomoneerrors.ErrNotFound, "coredaos keeper is required for AnteHandler") - } sigGasConsumer := opts.SigGasConsumer if sigGasConsumer == nil { @@ -87,7 +83,6 @@ func NewAnteHandler(opts HandlerOptions) (sdk.AnteHandler, error) { ), ), NewGovVoteDecorator(opts.Codec, opts.StakingKeeper), - NewGovSubmitProposalDecorator(opts.Codec, opts.CoreDAOsKeeper), ibcante.NewRedundantRelayDecorator(opts.IBCkeeper), } diff --git a/ante/gov_submit_proposal_ante.go b/ante/gov_submit_proposal_ante.go deleted file mode 100644 index 57f5d147..00000000 --- a/ante/gov_submit_proposal_ante.go +++ /dev/null @@ -1,132 +0,0 @@ -package ante - -import ( - "context" - - errorsmod "cosmossdk.io/errors" - - "github.com/cosmos/cosmos-sdk/codec" - codectypes "github.com/cosmos/cosmos-sdk/codec/types" - sdk "github.com/cosmos/cosmos-sdk/types" - sdkgovv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1" - - atomoneerrors "github.com/atomone-hub/atomone/types/errors" - coredaostypes "github.com/atomone-hub/atomone/x/coredaos/types" - govv1 "github.com/atomone-hub/atomone/x/gov/types/v1" -) - -// CoredaosParamsGetter is the interface required by GovSubmitProposalDecorator to -// retrieve the current coredaos params. -type CoredaosParamsGetter interface { - GetParams(ctx context.Context) coredaostypes.Params -} - -// GovSubmitProposalDecorator rejects any governance proposal whose message list -// contains a coredaos MsgUpdateParams that changes the oversight DAO address -// when that message is bundled together with other proposal messages. -type GovSubmitProposalDecorator struct { - cdc codec.BinaryCodec - coredaosKeeper CoredaosParamsGetter -} - -// NewGovSubmitProposalDecorator creates a new GovSubmitProposalDecorator. -func NewGovSubmitProposalDecorator(cdc codec.BinaryCodec, coredaosKeeper CoredaosParamsGetter) GovSubmitProposalDecorator { - return GovSubmitProposalDecorator{ - cdc: cdc, - coredaosKeeper: coredaosKeeper, - } -} - -func (g GovSubmitProposalDecorator) AnteHandle( - ctx sdk.Context, tx sdk.Tx, - simulate bool, next sdk.AnteHandler, -) (newCtx sdk.Context, err error) { - msgs := tx.GetMsgs() - if err = g.ValidateSubmitProposalMsgs(ctx, msgs); err != nil { - return ctx, err - } - return next(ctx, tx, simulate) -} - -// ValidateSubmitProposalMsgs checks that no submit-proposal message bundles a -// coredaos MsgUpdateParams that changes the oversight DAO address together with -// other proposal messages. It also inspects authz.MsgExec wrappers. -func (g GovSubmitProposalDecorator) ValidateSubmitProposalMsgs(ctx sdk.Context, msgs []sdk.Msg) error { - validateMsg := func(m sdk.Msg) error { - var proposalMsgs []*codectypes.Any - switch msg := m.(type) { - case *govv1.MsgSubmitProposal: - proposalMsgs = msg.GetMessages() - case *sdkgovv1.MsgSubmitProposal: - proposalMsgs = msg.GetMessages() - default: - return nil - } - return g.validateProposalMessages(ctx, proposalMsgs) - } - return iterateMsg(g.cdc, msgs, validateMsg) -} - -// addressChanged returns true if newAddr and currentAddr refer to -// different accounts. Comparison is done on the decoded bytes to -// make it case-insensitive -func addressChanged(newAddr, currentAddr string) (bool, error) { - if newAddr == "" && currentAddr == "" { - return false, nil - } - if newAddr == "" || currentAddr == "" { - return true, nil - } - newAccAddr, err := sdk.AccAddressFromBech32(newAddr) - if err != nil { - return false, err - } - currentAccAddr, err := sdk.AccAddressFromBech32(currentAddr) - if err != nil { - return false, err - } - return !newAccAddr.Equals(currentAccAddr), nil -} - -// validateProposalMessages inspects a list of proposal messages and returns an -// error if a coredaos MsgUpdateParams that changes the oversight DAO address is -// bundled with other messages. authz.MsgExec wrappers are expanded recursively -// so that nested oversight-DAO changes are also detected. -func (g GovSubmitProposalDecorator) validateProposalMessages(ctx sdk.Context, anyMsgs []*codectypes.Any) error { - effectiveMsgs, err := coredaostypes.FlattenAnyMsgs(g.cdc, anyMsgs) - if err != nil { - return err - } - - // Bundling is only possible when there is more than one effective message. - if len(effectiveMsgs) <= 1 { - return nil - } - - currentParams := g.coredaosKeeper.GetParams(ctx) - - for _, msg := range effectiveMsgs { - if msg == nil { - // Unpackable message — cannot be MsgUpdateParams, skip. - continue - } - updateParams, ok := msg.(*coredaostypes.MsgUpdateParams) - if !ok { - continue - } - changed, err := addressChanged(updateParams.Params.OversightDaoAddress, currentParams.OversightDaoAddress) - if err != nil { - return errorsmod.Wrap( - atomoneerrors.ErrUnauthorized, - "failed to compare Oversight DAO addresses: "+err.Error(), - ) - } - if changed { - return errorsmod.Wrap( - atomoneerrors.ErrUnauthorized, - "proposal that changes the Oversight DAO address cannot be bundled with other messages", - ) - } - } - return nil -} diff --git a/ante/gov_submit_proposal_ante_test.go b/ante/gov_submit_proposal_ante_test.go deleted file mode 100644 index 1ea834ba..00000000 --- a/ante/gov_submit_proposal_ante_test.go +++ /dev/null @@ -1,315 +0,0 @@ -package ante_test - -import ( - "strings" - "testing" - "time" - - "github.com/stretchr/testify/require" - - tmproto "github.com/cometbft/cometbft/proto/tendermint/types" - - simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims" - sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/cosmos/cosmos-sdk/x/authz" - sdkgovv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1" - - "github.com/atomone-hub/atomone/ante" - "github.com/atomone-hub/atomone/app/helpers" - coredaostypes "github.com/atomone-hub/atomone/x/coredaos/types" - govv1 "github.com/atomone-hub/atomone/x/gov/types/v1" -) - -// TestGovSubmitProposalDecoratorAtomOneV1 checks that the decorator rejects atomone -// gov v1 proposals that bundle a coredaos MsgUpdateParams changing the oversight -// DAO address with other messages. -func TestGovSubmitProposalDecoratorAtomOneV1(t *testing.T) { - atomoneApp := helpers.Setup(t) - ctx := atomoneApp.NewUncachedContext(true, tmproto.Header{}) - decorator := ante.NewGovSubmitProposalDecorator(atomoneApp.AppCodec(), atomoneApp.CoreDaosKeeper) - - addrs := simtestutil.CreateRandomAccounts(3) - currentOversightAddr := addrs[0].String() - newOversightAddr := addrs[1].String() - proposer := addrs[2].String() - extDuration := 7 * 24 * time.Hour - - // Set initial coredaos params with a known oversight DAO address. - err := atomoneApp.CoreDaosKeeper.Params.Set(ctx, coredaostypes.Params{ - OversightDaoAddress: currentOversightAddr, - VotingPeriodExtensionDuration: &extDuration, - }) - require.NoError(t, err) - - updateParamsChanging := &coredaostypes.MsgUpdateParams{ - Authority: proposer, - Params: coredaostypes.Params{ - OversightDaoAddress: newOversightAddr, // different from current - VotingPeriodExtensionDuration: &extDuration, - }, - } - updateParamsSame := &coredaostypes.MsgUpdateParams{ - Authority: proposer, - Params: coredaostypes.Params{ - OversightDaoAddress: currentOversightAddr, // same as current - VotingPeriodExtensionDuration: &extDuration, - }, - } - updateParamsSameUppercase := &coredaostypes.MsgUpdateParams{ - Authority: proposer, - Params: coredaostypes.Params{ - OversightDaoAddress: strings.ToUpper(currentOversightAddr), // same address, uppercased - VotingPeriodExtensionDuration: &extDuration, - }, - } - otherMsg := govv1.NewMsgVote(addrs[2], 1, govv1.VoteOption_VOTE_OPTION_YES, "") - - tests := []struct { - name string - msgs []sdk.Msg - expectPass bool - }{ - { - name: "single MsgUpdateParams changing oversight DAO — allowed", - msgs: []sdk.Msg{mustNewAtomOneSubmitProposal(t, []sdk.Msg{updateParamsChanging}, proposer)}, - expectPass: true, - }, - { - name: "single MsgUpdateParams not changing oversight DAO — allowed", - msgs: []sdk.Msg{mustNewAtomOneSubmitProposal(t, []sdk.Msg{updateParamsSame}, proposer)}, - expectPass: true, - }, - { - name: "MsgUpdateParams (oversight change) bundled with other msg — rejected", - msgs: []sdk.Msg{mustNewAtomOneSubmitProposal(t, []sdk.Msg{updateParamsChanging, otherMsg}, proposer)}, - expectPass: false, - }, - { - name: "MsgUpdateParams (no oversight change) bundled with other msg — allowed", - msgs: []sdk.Msg{mustNewAtomOneSubmitProposal(t, []sdk.Msg{updateParamsSame, otherMsg}, proposer)}, - expectPass: true, - }, - { - name: "MsgUpdateParams (same address uppercased) bundled with other msg — allowed", - msgs: []sdk.Msg{mustNewAtomOneSubmitProposal(t, []sdk.Msg{updateParamsSameUppercase, otherMsg}, proposer)}, - expectPass: true, - }, - { - name: "multiple non-MsgUpdateParams msgs — allowed", - msgs: []sdk.Msg{mustNewAtomOneSubmitProposal(t, []sdk.Msg{otherMsg, otherMsg}, proposer)}, - expectPass: true, - }, - { - name: "non-submit-proposal message — allowed", - msgs: []sdk.Msg{otherMsg}, - expectPass: true, - }, - } - - for _, tc := range tests { - err := decorator.ValidateSubmitProposalMsgs(ctx, tc.msgs) - if tc.expectPass { - require.NoError(t, err, "expected %v to pass", tc.name) - } else { - require.Error(t, err, "expected %v to fail", tc.name) - } - } -} - -// TestGovSubmitProposalDecoratorSDKV1 checks that the decorator rejects cosmos SDK -// gov v1 proposals that bundle a coredaos MsgUpdateParams changing the oversight -// DAO address with other messages. -func TestGovSubmitProposalDecoratorSDKV1(t *testing.T) { - atomoneApp := helpers.Setup(t) - ctx := atomoneApp.NewUncachedContext(true, tmproto.Header{}) - decorator := ante.NewGovSubmitProposalDecorator(atomoneApp.AppCodec(), atomoneApp.CoreDaosKeeper) - - addrs := simtestutil.CreateRandomAccounts(3) - currentOversightAddr := addrs[0].String() - newOversightAddr := addrs[1].String() - proposer := addrs[2].String() - extDuration := 7 * 24 * time.Hour - - // Set initial coredaos params with a known oversight DAO address. - err := atomoneApp.CoreDaosKeeper.Params.Set(ctx, coredaostypes.Params{ - OversightDaoAddress: currentOversightAddr, - VotingPeriodExtensionDuration: &extDuration, - }) - require.NoError(t, err) - - updateParamsChanging := &coredaostypes.MsgUpdateParams{ - Authority: proposer, - Params: coredaostypes.Params{ - OversightDaoAddress: newOversightAddr, // different from current - VotingPeriodExtensionDuration: &extDuration, - }, - } - updateParamsSame := &coredaostypes.MsgUpdateParams{ - Authority: proposer, - Params: coredaostypes.Params{ - OversightDaoAddress: currentOversightAddr, // same as current - VotingPeriodExtensionDuration: &extDuration, - }, - } - updateParamsSameUppercase := &coredaostypes.MsgUpdateParams{ - Authority: proposer, - Params: coredaostypes.Params{ - OversightDaoAddress: strings.ToUpper(currentOversightAddr), // same address, uppercased - VotingPeriodExtensionDuration: &extDuration, - }, - } - otherMsg := govv1.NewMsgVote(addrs[2], 1, govv1.VoteOption_VOTE_OPTION_YES, "") - - tests := []struct { - name string - msgs []sdk.Msg - expectPass bool - }{ - { - name: "single MsgUpdateParams changing oversight DAO — allowed", - msgs: []sdk.Msg{mustNewSDKSubmitProposal(t, []sdk.Msg{updateParamsChanging}, proposer)}, - expectPass: true, - }, - { - name: "single MsgUpdateParams not changing oversight DAO — allowed", - msgs: []sdk.Msg{mustNewSDKSubmitProposal(t, []sdk.Msg{updateParamsSame}, proposer)}, - expectPass: true, - }, - { - name: "MsgUpdateParams (oversight change) bundled with other msg — rejected", - msgs: []sdk.Msg{mustNewSDKSubmitProposal(t, []sdk.Msg{updateParamsChanging, otherMsg}, proposer)}, - expectPass: false, - }, - { - name: "MsgUpdateParams (no oversight change) bundled with other msg — allowed", - msgs: []sdk.Msg{mustNewSDKSubmitProposal(t, []sdk.Msg{updateParamsSame, otherMsg}, proposer)}, - expectPass: true, - }, - { - name: "MsgUpdateParams (same address uppercased) bundled with other msg — allowed", - msgs: []sdk.Msg{mustNewSDKSubmitProposal(t, []sdk.Msg{updateParamsSameUppercase, otherMsg}, proposer)}, - expectPass: true, - }, - { - name: "multiple non-MsgUpdateParams msgs — allowed", - msgs: []sdk.Msg{mustNewSDKSubmitProposal(t, []sdk.Msg{otherMsg, otherMsg}, proposer)}, - expectPass: true, - }, - { - name: "non-submit-proposal message — allowed", - msgs: []sdk.Msg{otherMsg}, - expectPass: true, - }, - } - - for _, tc := range tests { - err := decorator.ValidateSubmitProposalMsgs(ctx, tc.msgs) - if tc.expectPass { - require.NoError(t, err, "expected %v to pass", tc.name) - } else { - require.Error(t, err, "expected %v to fail", tc.name) - } - } -} - -// TestGovSubmitProposalDecoratorAuthz checks that authz.MsgExec wrappers are -// expanded when inspecting proposal messages. -func TestGovSubmitProposalDecoratorAuthz(t *testing.T) { - atomoneApp := helpers.Setup(t) - ctx := atomoneApp.NewUncachedContext(true, tmproto.Header{}) - cdc := atomoneApp.AppCodec() - decorator := ante.NewGovSubmitProposalDecorator(cdc, atomoneApp.CoreDaosKeeper) - - addrs := simtestutil.CreateRandomAccounts(3) - currentOversightAddr := addrs[0].String() - newOversightAddr := addrs[1].String() - proposer := addrs[2].String() - extDuration := 7 * 24 * time.Hour - - err := atomoneApp.CoreDaosKeeper.Params.Set(ctx, coredaostypes.Params{ - OversightDaoAddress: currentOversightAddr, - VotingPeriodExtensionDuration: &extDuration, - }) - require.NoError(t, err) - - updateParamsChanging := &coredaostypes.MsgUpdateParams{ - Authority: proposer, - Params: coredaostypes.Params{ - OversightDaoAddress: newOversightAddr, - VotingPeriodExtensionDuration: &extDuration, - }, - } - otherMsg := govv1.NewMsgVote(addrs[2], 1, govv1.VoteOption_VOTE_OPTION_YES, "") - - // mustMsgExec wraps msgs in an authz.MsgExec. - mustMsgExec := func(grantee sdk.AccAddress, msgs ...sdk.Msg) *authz.MsgExec { - execMsg := authz.NewMsgExec(grantee, msgs) - return &execMsg - } - - tests := []struct { - name string - msgs []sdk.Msg - expectPass bool - }{ - { - name: "authz MsgExec inside proposal bundles oversight change — rejected", - msgs: []sdk.Msg{mustNewAtomOneSubmitProposal(t, []sdk.Msg{ - mustMsgExec(addrs[2], updateParamsChanging, otherMsg), - }, proposer)}, - expectPass: false, - }, - { - name: "authz MsgExec for oversight change alongside other proposal msg — rejected", - msgs: []sdk.Msg{mustNewAtomOneSubmitProposal(t, []sdk.Msg{ - mustMsgExec(addrs[2], updateParamsChanging), - otherMsg, - }, proposer)}, - expectPass: false, - }, - { - name: "authz MsgExec with single oversight change — allowed", - msgs: []sdk.Msg{mustNewAtomOneSubmitProposal(t, []sdk.Msg{ - mustMsgExec(addrs[2], updateParamsChanging), - }, proposer)}, - expectPass: true, - }, - { - name: "authz MsgExec wrapping submit-proposal that bundles — rejected", - msgs: []sdk.Msg{mustMsgExec(addrs[2], - mustNewAtomOneSubmitProposal(t, []sdk.Msg{updateParamsChanging, otherMsg}, proposer), - )}, - expectPass: false, - }, - { - name: "authz MsgExec wrapping valid single-message submit-proposal — allowed", - msgs: []sdk.Msg{mustMsgExec(addrs[2], - mustNewAtomOneSubmitProposal(t, []sdk.Msg{updateParamsChanging}, proposer), - )}, - expectPass: true, - }, - } - - for _, tc := range tests { - err := decorator.ValidateSubmitProposalMsgs(ctx, tc.msgs) - if tc.expectPass { - require.NoError(t, err, "expected %v to pass", tc.name) - } else { - require.Error(t, err, "expected %v to fail", tc.name) - } - } -} - -func mustNewAtomOneSubmitProposal(t *testing.T, msgs []sdk.Msg, proposer string) *govv1.MsgSubmitProposal { - t.Helper() - msg, err := govv1.NewMsgSubmitProposal(msgs, sdk.NewCoins(), proposer, "", "title", "summary") - require.NoError(t, err) - return msg -} - -func mustNewSDKSubmitProposal(t *testing.T, msgs []sdk.Msg, proposer string) *sdkgovv1.MsgSubmitProposal { - t.Helper() - msg, err := sdkgovv1.NewMsgSubmitProposal(msgs, sdk.NewCoins(), proposer, "", "title", "summary") - require.NoError(t, err) - return msg -} diff --git a/app/app.go b/app/app.go index c0ac68fa..7e6d9ff5 100644 --- a/app/app.go +++ b/app/app.go @@ -276,7 +276,6 @@ func NewAtomOneApp( // If TxFeeChecker is nil the default ante TxFeeChecker is used TxFeeChecker: nil, DynamicfeeKeeper: app.DynamicfeeKeeper, - CoreDAOsKeeper: app.CoreDaosKeeper, }, ) if err != nil { diff --git a/app/keepers/keepers.go b/app/keepers/keepers.go index 7b41038b..8dc3d2ea 100644 --- a/app/keepers/keepers.go +++ b/app/keepers/keepers.go @@ -298,6 +298,9 @@ func NewAppKeeper( // If evidence needs to be handled for the app, set routes in router here and seal appKeepers.EvidenceKeeper = *evidenceKeeper + // Register coredaos gov hooks (rejects bundling an oversight-DAO change with other messages). + appKeepers.GovKeeper = appKeepers.GovKeeper.SetHooks(appKeepers.CoreDaosKeeper.GovHooks()) + // register the staking hooks // NOTE: stakingKeeper above is passed by reference, so that it will contain these hooks appKeepers.StakingKeeper.SetHooks( diff --git a/go.mod b/go.mod index d381d2ba..89199376 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,7 @@ go 1.26.4 replace ( cosmossdk.io/x/feegrant => cosmossdk.io/x/feegrant v0.1.1 cosmossdk.io/x/upgrade => github.com/atomone-hub/cosmos-sdk/x/upgrade v0.1.5-atomone.2 - github.com/cosmos/cosmos-sdk => github.com/atomone-hub/cosmos-sdk v0.500.1 + github.com/cosmos/cosmos-sdk => github.com/atomone-hub/cosmos-sdk v0.500.2-0.20260629065826-e4628cda7908 github.com/cosmos/ibc-go/v10 => github.com/cosmos/ibc-go/v10 v10.7.0 ) diff --git a/go.sum b/go.sum index a680399e..07cce4e5 100644 --- a/go.sum +++ b/go.sum @@ -100,8 +100,8 @@ github.com/armon/go-metrics v0.0.0-20180917152333-f0300d1749da/go.mod h1:Q73ZrmV github.com/armon/go-metrics v0.4.1/go.mod h1:E6amYzXo6aW1tqzoZGT755KkbgrJsSdpwZ+3JqfkOG4= github.com/armon/go-radix v0.0.0-20180808171621-7fddfc383310/go.mod h1:ufUuZ+zHj4x4TnLV4JWEpy2hxWSpsRywHrMgIH9cCH8= github.com/aryann/difflib v0.0.0-20170710044230-e206f873d14a/go.mod h1:DAHtR1m6lCRdSC2Tm3DSWRPvIPr6xNKyeHdqDQSQT+A= -github.com/atomone-hub/cosmos-sdk v0.500.1 h1:1/sNalNS6blWUm7iLXzC+RnGFsxMLdXKs33rNjNk8DI= -github.com/atomone-hub/cosmos-sdk v0.500.1/go.mod h1:bV+SvzTTjmJ10xv0SaROxOZxp96Q4bmi8nnT2CpxkWc= +github.com/atomone-hub/cosmos-sdk v0.500.2-0.20260629065826-e4628cda7908 h1:xLsHINXjjqD8Um4amKb92dwUzYcokA+ne8SkG2HhBE8= +github.com/atomone-hub/cosmos-sdk v0.500.2-0.20260629065826-e4628cda7908/go.mod h1:bV+SvzTTjmJ10xv0SaROxOZxp96Q4bmi8nnT2CpxkWc= github.com/atomone-hub/cosmos-sdk/x/upgrade v0.1.5-atomone.2 h1:SVPsm3c8pymdbwNjKHF5vwM0B6x4dgkVTbhbYctwBoE= github.com/atomone-hub/cosmos-sdk/x/upgrade v0.1.5-atomone.2/go.mod h1:78DcDK3kSaNN7dObcV1FIypDDIfl5W5X1uDyrUyZmWY= github.com/aws/aws-lambda-go v1.13.3/go.mod h1:4UKl9IzQMoD+QF79YdCuzCwp8VbmG4VAQwij/eHl5CU= diff --git a/x/coredaos/keeper/hooks.go b/x/coredaos/keeper/hooks.go index cf473dff..3854257b 100644 --- a/x/coredaos/keeper/hooks.go +++ b/x/coredaos/keeper/hooks.go @@ -3,13 +3,15 @@ package keeper import ( "context" - sdkerrors "cosmossdk.io/errors" + errorsmod "cosmossdk.io/errors" "cosmossdk.io/math" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" sdk "github.com/cosmos/cosmos-sdk/types" + govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" + atomoneerrors "github.com/atomone-hub/atomone/types/errors" "github.com/atomone-hub/atomone/x/coredaos/types" ) @@ -83,14 +85,94 @@ func validateDelegation(params types.Params, delAddr sdk.AccAddress) error { if params.GetSteeringDaoAddress() != "" { steeringDaoAddr := sdk.MustAccAddressFromBech32(params.GetSteeringDaoAddress()) if delAddr.Equals(steeringDaoAddr) { - return sdkerrors.Wrap(types.ErrCannotStake, "Steering DAO cannot stake") + return errorsmod.Wrap(types.ErrCannotStake, "Steering DAO cannot stake") } } if params.GetOversightDaoAddress() != "" { oversightDaoAddr := sdk.MustAccAddressFromBech32(params.GetOversightDaoAddress()) if delAddr.Equals(oversightDaoAddr) { - return sdkerrors.Wrap(types.ErrCannotStake, "Oversight DAO cannot stake") + return errorsmod.Wrap(types.ErrCannotStake, "Oversight DAO cannot stake") } } return nil } + +var _ govtypes.GovHooks = Hooks{} + +// GovHooks returns the gov hooks for the coredaos keeper. +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. +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 + } + if len(proposal.Messages) <= 1 { + return nil // bundling requires more than one message + } + for _, anyMsg := range proposal.Messages { + var msg sdk.Msg + if err := h.k.cdc.UnpackAny(anyMsg, &msg); err != nil { + continue + } + updateParams, ok := msg.(*types.MsgUpdateParams) + if !ok { + continue + } + changed, err := oversightDaoAddressChanged(updateParams.Params.OversightDaoAddress, params.OversightDaoAddress) + if err != nil { + return errorsmod.Wrap(atomoneerrors.ErrUnauthorized, "failed to compare Oversight DAO addresses: "+err.Error()) + } + if changed { + return errorsmod.Wrap(atomoneerrors.ErrUnauthorized, + "proposal that changes the Oversight DAO address cannot be bundled with other messages") + } + } + return nil +} + +func (h Hooks) AfterProposalDeposit(ctx context.Context, proposalID uint64, depositorAddr sdk.AccAddress) error { + return nil +} + +func (h Hooks) AfterProposalVote(ctx context.Context, proposalID uint64, voterAddr sdk.AccAddress) error { + return nil +} + +func (h Hooks) AfterProposalFailedMinDeposit(ctx context.Context, proposalID uint64) error { + return nil +} + +func (h Hooks) AfterProposalVotingPeriodEnded(ctx context.Context, proposalID uint64) error { + return nil +} + +// oversightDaoAddressChanged returns true if newAddr and currentAddr decode to different +// accounts (case-insensitive). Empty addresses are handled explicitly. +func oversightDaoAddressChanged(newAddr, currentAddr string) (bool, error) { + if newAddr == "" && currentAddr == "" { + return false, nil + } + if newAddr == "" || currentAddr == "" { + return true, nil + } + newAccAddr, err := sdk.AccAddressFromBech32(newAddr) + if err != nil { + return false, err + } + currentAccAddr, err := sdk.AccAddressFromBech32(currentAddr) + if err != nil { + return false, err + } + return !newAccAddr.Equals(currentAccAddr), nil +} diff --git a/x/coredaos/keeper/hooks_test.go b/x/coredaos/keeper/hooks_test.go new file mode 100644 index 00000000..51496c1c --- /dev/null +++ b/x/coredaos/keeper/hooks_test.go @@ -0,0 +1,109 @@ +package keeper_test + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" + + tmproto "github.com/cometbft/cometbft/proto/tendermint/types" + + simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims" + sdk "github.com/cosmos/cosmos-sdk/types" + sdktx "github.com/cosmos/cosmos-sdk/types/tx" + govv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1" + + "github.com/atomone-hub/atomone/app/helpers" + "github.com/atomone-hub/atomone/x/coredaos/types" +) + +// TestGovHookAfterProposalSubmission exercises the coredaos AfterProposalSubmission hook +// directly against a real gov keeper. Proposals are stored via SetProposal (which does not +// fire hooks), so the hook can be invoked in isolation over message shapes that the wired +// submission path would itself reject. +func TestGovHookAfterProposalSubmission(t *testing.T) { + app := helpers.Setup(t) + ctx := app.NewUncachedContext(true, tmproto.Header{Time: time.Now()}) + hooks := app.CoreDaosKeeper.GovHooks() + + current := simtestutil.CreateRandomAccounts(1)[0].String() + other := simtestutil.CreateRandomAccounts(1)[0].String() + govAddr := govModuleAddr() + extDuration := time.Hour + + // changing alters the oversight DAO address; same keeps it unchanged. + changing := &types.MsgUpdateParams{Authority: govAddr, Params: types.Params{OversightDaoAddress: other, VotingPeriodExtensionDuration: &extDuration}} + same := &types.MsgUpdateParams{Authority: govAddr, Params: types.Params{OversightDaoAddress: current, VotingPeriodExtensionDuration: &extDuration}} + + setOversight := func(addr string) { + require.NoError(t, app.CoreDaosKeeper.Params.Set(ctx, types.Params{OversightDaoAddress: addr, VotingPeriodExtensionDuration: &extDuration})) + } + // store writes a proposal directly (no hook) so the hook can be invoked standalone. + 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})) + } + + tests := []struct { + name string + oversight string // "" => oversight DAO unset, hook disabled + msgs []sdk.Msg + wantErr bool + }{ + {"disabled when oversight DAO unset", "", []sdk.Msg{changing, same}, false}, + {"single oversight change, not bundled", current, []sdk.Msg{changing}, false}, + {"oversight change bundled with another msg", current, []sdk.Msg{changing, same}, true}, + {"bundled but no oversight change", current, []sdk.Msg{same, same}, false}, + } + for i, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + setOversight(tt.oversight) + id := uint64(i + 1) + store(id, tt.msgs...) + err := hooks.AfterProposalSubmission(ctx, id) + if tt.wantErr { + require.ErrorContains(t, err, "cannot be bundled") + return + } + require.NoError(t, err) + }) + } + + // A non-existent proposal is a no-op (nothing to enforce). + setOversight(current) + require.NoError(t, hooks.AfterProposalSubmission(ctx, 9999)) +} + +// TestGovHookWiredRejectsBundledOversightChange checks that the coredaos hook is actually +// WIRED into the gov keeper (via SetHooks in app wiring) and fires during a real +// SubmitProposal. Nothing else guards the wiring, so removing the SetHooks call would only +// be caught here. It submits through the SDK gov keeper's SubmitProposal (which invokes +// AfterProposalSubmission and, being the keeper method, skips the msgServer deposit gate). +func TestGovHookWiredRejectsBundledOversightChange(t *testing.T) { + app := helpers.Setup(t) + ctx := app.NewUncachedContext(true, tmproto.Header{Time: time.Now()}) + + current := simtestutil.CreateRandomAccounts(1)[0].String() + other := simtestutil.CreateRandomAccounts(1)[0].String() + govAddr := govModuleAddr() + extDuration := time.Hour + require.NoError(t, app.CoreDaosKeeper.Params.Set(ctx, types.Params{OversightDaoAddress: current, VotingPeriodExtensionDuration: &extDuration})) + + // Both messages are signed by the gov module account (required of proposal messages). + changing := &types.MsgUpdateParams{Authority: govAddr, Params: types.Params{OversightDaoAddress: other, VotingPeriodExtensionDuration: &extDuration}} + same := &types.MsgUpdateParams{Authority: govAddr, Params: types.Params{OversightDaoAddress: current, VotingPeriodExtensionDuration: &extDuration}} + proposer := simtestutil.CreateRandomAccounts(1)[0] + + // Bundling an oversight-DAO change with another message must be rejected by the hook. + _, err := app.GovKeeper.SubmitProposal(ctx, []sdk.Msg{changing, same}, "", "title", "summary", proposer) + require.Error(t, err) + require.ErrorContains(t, err, "cannot be bundled") + + // A single (non-bundled) oversight change must not trip the hook. It may still error for + // unrelated reasons, so only assert the hook did not fire. + _, err = app.GovKeeper.SubmitProposal(ctx, []sdk.Msg{changing}, "", "title", "summary", proposer) + if err != nil { + require.NotContains(t, err.Error(), "cannot be bundled") + } +} diff --git a/x/coredaos/keeper/msg_server.go b/x/coredaos/keeper/msg_server.go index d2e6effa..ee4d3ed7 100644 --- a/x/coredaos/keeper/msg_server.go +++ b/x/coredaos/keeper/msg_server.go @@ -395,12 +395,12 @@ func (ms MsgServer) VetoProposal(goCtx context.Context, msg *types.MsgVetoPropos // Check if the proposal contains a change of the oversight DAO address. // If so, vetoing the proposal would create a scenario where the current oversight DAO can prevent its own replacement. - // authz.MsgExec wrappers are expanded recursively so the check cannot be bypassed by wrapping MsgUpdateParams. - effectiveMsgs, err := types.FlattenAnyMsgs(ms.k.cdc, proposal.Messages) - if err != nil { - return nil, err - } - for _, flatMsg := range effectiveMsgs { + // Self-executing authz.MsgExec wrappers are rejected at submission, so only top-level messages need inspection. + for _, anyMsg := range proposal.Messages { + var flatMsg sdk.Msg + if err := ms.k.cdc.UnpackAny(anyMsg, &flatMsg); err != nil { + continue + } updateParamsMsg, ok := flatMsg.(*types.MsgUpdateParams) if !ok { continue @@ -414,7 +414,6 @@ func (ms MsgServer) VetoProposal(goCtx context.Context, msg *types.MsgVetoPropos "current_oversight_dao_address", params.OversightDaoAddress, "new_oversight_dao_address", updateParamsMsg.Params.OversightDaoAddress, ) - return nil, types.ErrInvalidVeto.Wrapf("proposal with ID %d contains a change of the oversight DAO address, vetoing it would prevent the replacement of the current oversight DAO", proposal.Id) } } diff --git a/x/coredaos/keeper/msg_server_test.go b/x/coredaos/keeper/msg_server_test.go index ef16a233..39834fcb 100644 --- a/x/coredaos/keeper/msg_server_test.go +++ b/x/coredaos/keeper/msg_server_test.go @@ -12,11 +12,9 @@ import ( "cosmossdk.io/collections" "cosmossdk.io/math" - codectypes "github.com/cosmos/cosmos-sdk/codec/types" simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims" sdk "github.com/cosmos/cosmos-sdk/types" authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" - "github.com/cosmos/cosmos-sdk/x/authz" banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" govv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1" @@ -614,17 +612,19 @@ func TestMsgServerEndorseProposal(t *testing.T) { } func TestMsgServerExtendVotingPeriod(t *testing.T) { - testAcc := simtestutil.CreateRandomAccounts(2) + testAcc := simtestutil.CreateRandomAccounts(3) extenderAcc := testAcc[0].String() steeringDAOAcc := testAcc[1].String() + oversightDAOAcc := testAcc[2].String() tests := []struct { - name string - msg *types.MsgExtendVotingPeriod - expectedErr string - proposalState string - setSteeringDAO bool - assertExtended bool + name string + msg *types.MsgExtendVotingPeriod + expectedErr string + proposalState string + setSteeringDAO bool + setOversightDAO bool + assertExtended bool }{ { name: "empty msg", @@ -672,6 +672,15 @@ func TestMsgServerExtendVotingPeriod(t *testing.T) { setSteeringDAO: true, assertExtended: true, }, + { + name: "ok extended by oversight DAO", + msg: &types.MsgExtendVotingPeriod{ + Extender: oversightDAOAcc, + }, + proposalState: "voting", + setOversightDAO: true, + assertExtended: true, + }, { name: "proposal not in voting period", msg: &types.MsgExtendVotingPeriod{ @@ -701,6 +710,9 @@ func TestMsgServerExtendVotingPeriod(t *testing.T) { if tt.setSteeringDAO { params.SteeringDaoAddress = steeringDAOAcc } + if tt.setOversightDAO { + params.OversightDaoAddress = oversightDAOAcc + } require.NoError(t, app.CoreDaosKeeper.Params.Set(ctx, params)) var origEndTime time.Time @@ -846,24 +858,6 @@ func TestMsgServerVetoProposal(t *testing.T) { proposalState: "voting-disable-oversight", setOversightDAO: true, }, - { - name: "veto proposal with change to oversight DAO address wrapped in authz.MsgExec", - msg: &types.MsgVetoProposal{ - Vetoer: oversightDAOAcc, - }, - expectedErr: "contains a change of the oversight DAO address, vetoing it would prevent the replacement of the current oversight DAO: oversight DAO cannot veto this proposal", - proposalState: "voting-change-oversight-wrapped", - setOversightDAO: true, - }, - { - name: "veto proposal with change to oversight DAO address double-wrapped in authz.MsgExec", - msg: &types.MsgVetoProposal{ - Vetoer: oversightDAOAcc, - }, - expectedErr: "contains a change of the oversight DAO address, vetoing it would prevent the replacement of the current oversight DAO: oversight DAO cannot veto this proposal", - proposalState: "voting-change-oversight-double-wrapped", - setOversightDAO: true, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -908,20 +902,6 @@ func TestMsgServerVetoProposal(t *testing.T) { case "voting-disable-oversight": p := submitProposalReal(t, app, ctx, []sdk.Msg{disableOversightMsg}, true) tt.msg.ProposalId = p.Id - case "voting-change-oversight-wrapped": - inner, err := codectypes.NewAnyWithValue(changeOversightMsg) - require.NoError(t, err) - exec := &authz.MsgExec{Grantee: govAddr, Msgs: []*codectypes.Any{inner}} - p := submitProposalReal(t, app, ctx, []sdk.Msg{exec}, true) - tt.msg.ProposalId = p.Id - case "voting-change-oversight-double-wrapped": - inner, err := codectypes.NewAnyWithValue(changeOversightMsg) - require.NoError(t, err) - execAny, err := codectypes.NewAnyWithValue(&authz.MsgExec{Grantee: govAddr, Msgs: []*codectypes.Any{inner}}) - require.NoError(t, err) - outer := &authz.MsgExec{Grantee: govAddr, Msgs: []*codectypes.Any{execAny}} - p := submitProposalReal(t, app, ctx, []sdk.Msg{outer}, true) - tt.msg.ProposalId = p.Id } if err := tt.msg.ValidateBasic(); err != nil { diff --git a/x/coredaos/types/utils.go b/x/coredaos/types/utils.go deleted file mode 100644 index 0718d73a..00000000 --- a/x/coredaos/types/utils.go +++ /dev/null @@ -1,45 +0,0 @@ -package types - -import ( - errorsmod "cosmossdk.io/errors" - - "github.com/cosmos/cosmos-sdk/codec" - codectypes "github.com/cosmos/cosmos-sdk/codec/types" - sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/cosmos/cosmos-sdk/x/authz" - - atomoneerrors "github.com/atomone-hub/atomone/types/errors" -) - -const maxAuthzNestingDepth = 8 - -// FlattenAnyMsgs recursively unpacks Any-encoded messages, expanding -// authz.MsgExec wrappers so that all leaf messages are returned in a flat -// slice. Entries that cannot be unpacked are represented as nil. -func FlattenAnyMsgs(cdc codec.BinaryCodec, anyMsgs []*codectypes.Any) ([]sdk.Msg, error) { - return flattenAnyMsgsWithDepth(cdc, anyMsgs, 0) -} - -func flattenAnyMsgsWithDepth(cdc codec.BinaryCodec, anyMsgs []*codectypes.Any, depth int) ([]sdk.Msg, error) { - if depth > maxAuthzNestingDepth { - return nil, errorsmod.Wrap(atomoneerrors.ErrUnauthorized, "authz nesting depth exceeded") - } - var result []sdk.Msg - for _, anyMsg := range anyMsgs { - var msg sdk.Msg - if err := cdc.UnpackAny(anyMsg, &msg); err != nil { - result = append(result, nil) - continue - } - if execMsg, ok := msg.(*authz.MsgExec); ok { - subMsgs, err := flattenAnyMsgsWithDepth(cdc, execMsg.Msgs, depth+1) - if err != nil { - return nil, err - } - result = append(result, subMsgs...) - } else { - result = append(result, msg) - } - } - return result, nil -} diff --git a/x/coredaos/types/utils_test.go b/x/coredaos/types/utils_test.go deleted file mode 100644 index 12c3386e..00000000 --- a/x/coredaos/types/utils_test.go +++ /dev/null @@ -1,99 +0,0 @@ -package types_test - -import ( - "testing" - - "github.com/stretchr/testify/require" - - "github.com/cosmos/cosmos-sdk/codec" - codectypes "github.com/cosmos/cosmos-sdk/codec/types" - sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/cosmos/cosmos-sdk/x/authz" - - "github.com/atomone-hub/atomone/x/coredaos/types" -) - -// maxAuthzNestingDepth mirrors the unexported constant in utils.go. FlattenAnyMsgs -// rejects nestings strictly deeper than this value. -const maxAuthzNestingDepth = 8 - -func flattenTestCodec() codec.BinaryCodec { - registry := codectypes.NewInterfaceRegistry() - authz.RegisterInterfaces(registry) - types.RegisterInterfaces(registry) - return codec.NewProtoCodec(registry) -} - -// wrapMsgExec wraps inner in `depth` nested authz.MsgExec layers and returns the -// outermost message as an Any. depth == 0 returns inner unchanged. -func wrapMsgExec(t *testing.T, inner *codectypes.Any, depth int) *codectypes.Any { - t.Helper() - cur := inner - for i := 0; i < depth; i++ { - any, err := codectypes.NewAnyWithValue(&authz.MsgExec{ - Grantee: sdk.AccAddress("grantee").String(), - Msgs: []*codectypes.Any{cur}, - }) - require.NoError(t, err) - cur = any - } - return cur -} - -func mustPackMsg(t *testing.T, msg sdk.Msg) *codectypes.Any { - t.Helper() - any, err := codectypes.NewAnyWithValue(msg) - require.NoError(t, err) - return any -} - -func TestFlattenAnyMsgs(t *testing.T) { - cdc := flattenTestCodec() - - leaf := &types.MsgUpdateParams{ - Params: types.Params{OversightDaoAddress: "the-leaf-address"}, - } - - t.Run("bare message is returned as-is", func(t *testing.T) { - out, err := types.FlattenAnyMsgs(cdc, []*codectypes.Any{mustPackMsg(t, leaf)}) - require.NoError(t, err) - require.Len(t, out, 1) - updateParams, ok := out[0].(*types.MsgUpdateParams) - require.True(t, ok) - require.Equal(t, "the-leaf-address", updateParams.Params.OversightDaoAddress) - }) - - t.Run("multiple wrappers and bare messages are all flattened", func(t *testing.T) { - out, err := types.FlattenAnyMsgs(cdc, []*codectypes.Any{ - wrapMsgExec(t, mustPackMsg(t, leaf), 1), - wrapMsgExec(t, mustPackMsg(t, leaf), 2), - mustPackMsg(t, leaf), - }) - require.NoError(t, err) - require.Len(t, out, 3) - for _, m := range out { - updateParams, ok := m.(*types.MsgUpdateParams) - require.True(t, ok) - require.Equal(t, "the-leaf-address", updateParams.Params.OversightDaoAddress) - } - }) - - t.Run("nesting at the limit is accepted", func(t *testing.T) { - out, err := types.FlattenAnyMsgs(cdc, []*codectypes.Any{ - wrapMsgExec(t, mustPackMsg(t, leaf), maxAuthzNestingDepth), - }) - require.NoError(t, err) - require.Len(t, out, 1) - _, ok := out[0].(*types.MsgUpdateParams) - require.True(t, ok) - }) - - t.Run("nesting beyond the limit is rejected", func(t *testing.T) { - out, err := types.FlattenAnyMsgs(cdc, []*codectypes.Any{ - wrapMsgExec(t, mustPackMsg(t, leaf), maxAuthzNestingDepth+1), - }) - require.Error(t, err) - require.ErrorContains(t, err, "authz nesting depth exceeded") - require.Nil(t, out) - }) -}