diff --git a/CHANGELOG.md b/CHANGELOG.md index a75f6041..02caf760 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ - Migrate `x/gov` fork from Atom One to Atom One SDK [#248](https://github.com/atomone-hub/atomone/pull/248) - Add governors to `x/gov` module [#258](https://github.com/atomone-hub/atomone/pull/258) - Prevent Oversight DAO from vetoing proposals that include a change to the Oversight DAO address [#275](https://github.com/atomone-hub/atomone/pull/275) +- Prevent Oversight DAO change to be bundled in proposals [#316](https://github.com/atomone-hub/atomone/pull/316) ### STATE BREAKING diff --git a/ante/ante.go b/ante/ante.go index 8e095dfd..fa3a6c6d 100644 --- a/ante/ante.go +++ b/ante/ante.go @@ -28,6 +28,7 @@ type HandlerOptions struct { PhotonKeeper *photonkeeper.Keeper TxFeeChecker ante.TxFeeChecker DynamicfeeKeeper *dynamicfeekeeper.Keeper + CoreDAOsKeeper CoredaosParamsGetter } func NewAnteHandler(opts HandlerOptions) (sdk.AnteHandler, error) { @@ -52,6 +53,9 @@ 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 { @@ -83,6 +87,7 @@ 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_ante_utils.go b/ante/gov_ante_utils.go new file mode 100644 index 00000000..02ee2e90 --- /dev/null +++ b/ante/gov_ante_utils.go @@ -0,0 +1,57 @@ +package ante + +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" +) + +// iterateMsg calls fn for each message in msgs. For authz.MsgExec messages, +// fn is called for each inner message instead of the exec message itself. +// Returns ErrUnauthorized if an inner message cannot be unpacked. +func iterateMsg(cdc codec.BinaryCodec, msgs []sdk.Msg, fn func(sdk.Msg) error) error { + for _, m := range msgs { + if execMsg, ok := m.(*authz.MsgExec); ok { + for _, anyInner := range execMsg.Msgs { + var inner sdk.Msg + if err := cdc.UnpackAny(anyInner, &inner); err != nil { + return errorsmod.Wrap(atomoneerrors.ErrUnauthorized, "cannot unmarshal authz exec msgs") + } + if err := fn(inner); err != nil { + return err + } + } + continue + } + if err := fn(m); err != nil { + return err + } + } + return nil +} + +// flattenAnyMsgs recursively unpacks a list of 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 — they still +// count towards the total length, which callers use to detect bundling. +func flattenAnyMsgs(cdc codec.BinaryCodec, anyMsgs []*codectypes.Any) []sdk.Msg { + 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 { + result = append(result, flattenAnyMsgs(cdc, execMsg.Msgs)...) + } else { + result = append(result, msg) + } + } + return result +} diff --git a/ante/gov_submit_proposal_ante.go b/ante/gov_submit_proposal_ante.go new file mode 100644 index 00000000..977a3d46 --- /dev/null +++ b/ante/gov_submit_proposal_ante.go @@ -0,0 +1,129 @@ +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 := flattenAnyMsgs(g.cdc, anyMsgs) + + // 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 new file mode 100644 index 00000000..1ea834ba --- /dev/null +++ b/ante/gov_submit_proposal_ante_test.go @@ -0,0 +1,315 @@ +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/ante/gov_vote_ante.go b/ante/gov_vote_ante.go index e38c4bd9..e5f9d178 100644 --- a/ante/gov_vote_ante.go +++ b/ante/gov_vote_ante.go @@ -8,7 +8,6 @@ import ( "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/cosmos/cosmos-sdk/x/authz" sdkgovv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1" sdkgovv1beta1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1" stakingkeeper "github.com/cosmos/cosmos-sdk/x/staking/keeper" @@ -152,32 +151,5 @@ func (g GovVoteDecorator) ValidateVoteMsgs(ctx sdk.Context, msgs []sdk.Msg) erro return nil } - validAuthz := func(execMsg *authz.MsgExec) error { - for _, v := range execMsg.Msgs { - var innerMsg sdk.Msg - if err := g.cdc.UnpackAny(v, &innerMsg); err != nil { - return errorsmod.Wrap(atomoneerrors.ErrUnauthorized, "cannot unmarshal authz exec msgs") - } - if err := validMsg(innerMsg); err != nil { - return err - } - } - - return nil - } - - for _, m := range msgs { - if msg, ok := m.(*authz.MsgExec); ok { - if err := validAuthz(msg); err != nil { - return err - } - continue - } - - // validate normal msgs - if err := validMsg(m); err != nil { - return err - } - } - return nil + return iterateMsg(g.cdc, msgs, validMsg) } diff --git a/app/app.go b/app/app.go index 7e6d9ff5..c0ac68fa 100644 --- a/app/app.go +++ b/app/app.go @@ -276,6 +276,7 @@ func NewAtomOneApp( // If TxFeeChecker is nil the default ante TxFeeChecker is used TxFeeChecker: nil, DynamicfeeKeeper: app.DynamicfeeKeeper, + CoreDAOsKeeper: app.CoreDaosKeeper, }, ) if err != nil {