Democracy governance whitelisting #284

app/consumer-democracy/ante/forbidden_proposals_ante.go

@@ -0,0 +1,32 @@
+package ante
+
+import (
+	"fmt"
+
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
+)
+
+type ForbiddenProposalsDecorator struct {
+	IsProposalWhitelisted func(govtypes.Content) bool
+}
+
+func NewForbiddenProposalsDecorator(whiteListFn func(govtypes.Content) bool) ForbiddenProposalsDecorator {
+	return ForbiddenProposalsDecorator{IsProposalWhitelisted: whiteListFn}
+}
+
+func (decorator ForbiddenProposalsDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) {
+	currHeight := ctx.BlockHeight()
+
+	for _, msg := range tx.GetMsgs() {
+		submitProposalMgs, ok := msg.(*govtypes.MsgSubmitProposal)
+		//if the message is MsgSubmitProposal, check if proposal is whitelisted
+		if ok {
+			if !decorator.IsProposalWhitelisted(submitProposalMgs.GetContent()) {
+				return ctx, fmt.Errorf("tx contains unsupported proposal message types at height %d", currHeight)
+			}
+		}
+	}
+
+	return next(ctx, tx, simulate)
+}

app/consumer-democracy/ante/forbidden_proposals_ante_test.go

@@ -0,0 +1,97 @@
+package ante_test
+
+import (
+	"testing"
+
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
+	banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
+	govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
+	"github.com/cosmos/cosmos-sdk/x/params/types/proposal"
+	app "github.com/cosmos/interchain-security/app/consumer-democracy"
+	"github.com/cosmos/interchain-security/app/consumer-democracy/ante"
+	"github.com/stretchr/testify/require"
+	"github.com/tendermint/spm/cosmoscmd"
+)
+
+func TestForbiddenProposalsDecorator(t *testing.T) {
+	txCfg := cosmoscmd.MakeEncodingConfig(app.ModuleBasics).TxConfig
+
+	testCases := []struct {
+		name      string
+		ctx       sdk.Context
+		msgs      []sdk.Msg
+		expectErr bool
+	}{
+		{
+			name: "Allowed param change",
+			ctx:  sdk.Context{},
+			msgs: []sdk.Msg{
+				newParamChangeProposalMsg([]proposal.ParamChange{
+					//only subspace and key are relevant for testing
+					{Subspace: banktypes.ModuleName, Key: "SendEnabled", Value: ""},
+				}),
+			},
+			expectErr: false,
+		},
+		{
+			name: "Forbidden param change",
+			ctx:  sdk.Context{},
+			msgs: []sdk.Msg{
+				newParamChangeProposalMsg([]proposal.ParamChange{
+					{Subspace: authtypes.ModuleName, Key: "MaxMemoCharacters", Value: ""},
+				}),
+			},
+			expectErr: true,
+		},
+		{
+			name: "Allowed and forbidden param changes in the same msg",
+			ctx:  sdk.Context{},
+			msgs: []sdk.Msg{
+				newParamChangeProposalMsg([]proposal.ParamChange{
+					{Subspace: banktypes.ModuleName, Key: "SendEnabled", Value: ""},
+					{Subspace: authtypes.ModuleName, Key: "MaxMemoCharacters", Value: ""},
+				}),
+			},
+			expectErr: true,
+		},
+		{
+			name: "Allowed and forbidden param changes in different msg",
+			ctx:  sdk.Context{},
+			msgs: []sdk.Msg{
+				newParamChangeProposalMsg([]proposal.ParamChange{
+					{Subspace: banktypes.ModuleName, Key: "SendEnabled", Value: ""},
+				}),
+				newParamChangeProposalMsg([]proposal.ParamChange{
+					{Subspace: authtypes.ModuleName, Key: "MaxMemoCharacters", Value: ""},
+				}),
+			},
+			expectErr: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+
+		t.Run(tc.name, func(t *testing.T) {
+			handler := ante.NewForbiddenProposalsDecorator(app.IsProposalWhitelisted)
+
+			txBuilder := txCfg.NewTxBuilder()
+			require.NoError(t, txBuilder.SetMsgs(tc.msgs...))
+
+			_, err := handler.AnteHandle(tc.ctx, txBuilder.GetTx(), false,
+				func(ctx sdk.Context, _ sdk.Tx, _ bool) (sdk.Context, error) { return ctx, nil })
+			if tc.expectErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
+func newParamChangeProposalMsg(changes []proposal.ParamChange) *govtypes.MsgSubmitProposal {
+	paramChange := proposal.ParameterChangeProposal{Changes: changes}
+	msg, _ := govtypes.NewMsgSubmitProposal(&paramChange, sdk.NewCoins(), sdk.AccAddress{})
+	return msg
+}

app/consumer-democracy/ante_handler.go

@@ -6,6 +6,7 @@ import (
 	"github.com/cosmos/cosmos-sdk/x/auth/ante"
 	ibcante "github.com/cosmos/ibc-go/v3/modules/core/ante"
 	ibckeeper "github.com/cosmos/ibc-go/v3/modules/core/keeper"
+	democracyante "github.com/cosmos/interchain-security/app/consumer-democracy/ante"
 	consumerante "github.com/cosmos/interchain-security/app/consumer/ante"
 	ibcconsumerkeeper "github.com/cosmos/interchain-security/x/ccv/consumer/keeper"
 )
@@ -40,6 +41,7 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) {
 		ante.NewRejectExtensionOptionsDecorator(),
 		consumerante.NewMsgFilterDecorator(options.ConsumerKeeper),
 		consumerante.NewDisabledModulesDecorator("/cosmos.evidence", "/cosmos.slashing"),
+		democracyante.NewForbiddenProposalsDecorator(IsProposalWhitelisted),
 		ante.NewMempoolFeeDecorator(),
 		ante.NewValidateBasicDecorator(),
 		ante.NewTxTimeoutHeightDecorator(),

app/consumer-democracy/app.go

@@ -90,9 +90,10 @@ import (
 	ccvstakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
 	ccvstaking "github.com/cosmos/interchain-security/x/ccv/democracy/staking"
 
-	ccvgov "github.com/cosmos/cosmos-sdk/x/gov"
+	gov "github.com/cosmos/cosmos-sdk/x/gov"
 	ccvgovkeeper "github.com/cosmos/cosmos-sdk/x/gov/keeper"
 	ccvgovtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
+	ccvgov "github.com/cosmos/interchain-security/x/ccv/democracy/governance"
 
 	// add mint
 	ccvmint "github.com/cosmos/cosmos-sdk/x/mint"
@@ -129,7 +130,7 @@ var (
 		ccvstaking.AppModuleBasic{},
 		ccvmint.AppModuleBasic{},
 		ccvdistr.AppModuleBasic{},
-		ccvgov.NewAppModuleBasic(
+		gov.NewAppModuleBasic(
 			// TODO: eventually remove upgrade proposal handler and cancel proposal handler
 			paramsclient.ProposalHandler, ccvdistrclient.ProposalHandler, upgradeclient.ProposalHandler, upgradeclient.CancelProposalHandler,
 		),
@@ -477,7 +478,7 @@ func New(
 		capability.NewAppModule(appCodec, *app.CapabilityKeeper),
 		crisis.NewAppModule(&app.CrisisKeeper, skipGenesisInvariants),
 		feegrantmodule.NewAppModule(appCodec, app.AccountKeeper, app.BankKeeper, app.FeeGrantKeeper, app.interfaceRegistry),
-		ccvgov.NewAppModule(appCodec, app.GovKeeper, app.AccountKeeper, app.BankKeeper),
+		ccvgov.NewAppModule(appCodec, app.GovKeeper, app.AccountKeeper, app.BankKeeper, IsProposalWhitelisted),
 		ccvmint.NewAppModule(appCodec, app.MintKeeper, app.AccountKeeper),
 		slashing.NewAppModule(appCodec, app.SlashingKeeper, app.AccountKeeper, app.BankKeeper, app.ConsumerKeeper),
 		ccvdistr.NewAppModule(appCodec, app.DistrKeeper, app.AccountKeeper, app.BankKeeper, app.StakingKeeper, authtypes.FeeCollectorName),
@@ -580,7 +581,7 @@ func New(
 		bank.NewAppModule(appCodec, app.BankKeeper, app.AccountKeeper),
 		capability.NewAppModule(appCodec, *app.CapabilityKeeper),
 		feegrantmodule.NewAppModule(appCodec, app.AccountKeeper, app.BankKeeper, app.FeeGrantKeeper, app.interfaceRegistry),
-		ccvgov.NewAppModule(appCodec, app.GovKeeper, app.AccountKeeper, app.BankKeeper),
+		ccvgov.NewAppModule(appCodec, app.GovKeeper, app.AccountKeeper, app.BankKeeper, IsProposalWhitelisted),
 		ccvmint.NewAppModule(appCodec, app.MintKeeper, app.AccountKeeper),
 		ccvstaking.NewAppModule(appCodec, app.StakingKeeper, app.AccountKeeper, app.BankKeeper),
 		ccvdistr.NewAppModule(appCodec, app.DistrKeeper, app.AccountKeeper, app.BankKeeper, app.StakingKeeper, authtypes.FeeCollectorName),

app/consumer-democracy/proposals_whitelisting.go

@@ -0,0 +1,68 @@
+package app
+
+import (
+	banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
+	distrtypes "github.com/cosmos/cosmos-sdk/x/distribution/types"
+	govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
+	minttypes "github.com/cosmos/cosmos-sdk/x/mint/types"
+	"github.com/cosmos/cosmos-sdk/x/params/types/proposal"
+	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
+	ibctransfertypes "github.com/cosmos/ibc-go/v3/modules/apps/transfer/types"
+)
+
+func IsProposalWhitelisted(content govtypes.Content) bool {
+
+	switch c := content.(type) {
+	case *proposal.ParameterChangeProposal:
+		return isParamProposalWhitelisted(c.Changes)
+
+	default:
+		return false
+	}
+
+}
+
+func isParamProposalWhitelisted(paramChanges []proposal.ParamChange) bool {
+	for _, paramChange := range paramChanges {
+		_, found := WhitelistedParams[paramChangeKey{Subspace: paramChange.Subspace, Key: paramChange.Key}]
+		if !found {
+			return false
+		}
+	}
+	return true
+}
+
+type paramChangeKey struct {
+	Subspace, Key string
+}
+
+var WhitelistedParams = map[paramChangeKey]struct{}{
+	//bank
+	{Subspace: banktypes.ModuleName, Key: "SendEnabled"}: {},
+	//governance
+	{Subspace: govtypes.ModuleName, Key: "depositparams"}: {}, //min_deposit, max_deposit_period
+	{Subspace: govtypes.ModuleName, Key: "votingparams"}:  {}, //voting_period
+	{Subspace: govtypes.ModuleName, Key: "tallyparams"}:   {}, //quorum,threshold,veto_threshold
+	//staking
+	{Subspace: stakingtypes.ModuleName, Key: "UnbondingTime"}:     {},
+	{Subspace: stakingtypes.ModuleName, Key: "MaxValidators"}:     {},
+	{Subspace: stakingtypes.ModuleName, Key: "MaxEntries"}:        {},
+	{Subspace: stakingtypes.ModuleName, Key: "HistoricalEntries"}: {},
+	{Subspace: stakingtypes.ModuleName, Key: "BondDenom"}:         {},
+	//distribution
+	{Subspace: distrtypes.ModuleName, Key: "communitytax"}:        {},
+	{Subspace: distrtypes.ModuleName, Key: "baseproposerreward"}:  {},
+	{Subspace: distrtypes.ModuleName, Key: "bonusproposerreward"}: {},
+	{Subspace: distrtypes.ModuleName, Key: "withdrawaddrenabled"}: {},
+	//mint
+	{Subspace: minttypes.ModuleName, Key: "MintDenom"}:           {},
+	{Subspace: minttypes.ModuleName, Key: "InflationRateChange"}: {},
+	{Subspace: minttypes.ModuleName, Key: "InflationMax"}:        {},
+	{Subspace: minttypes.ModuleName, Key: "InflationMin"}:        {},
+	{Subspace: minttypes.ModuleName, Key: "GoalBonded"}:          {},
+	{Subspace: minttypes.ModuleName, Key: "BlocksPerYear"}:       {},
+	//ibc transfer
+	{Subspace: ibctransfertypes.ModuleName, Key: "SendEnabled"}:    {},
+	{Subspace: ibctransfertypes.ModuleName, Key: "ReceiveEnabled"}: {},
+	//TODO: interchain accounts are listed in spec but app does not include the module - check if something should be done
+}

tests/e2e/democracy_test.go

@@ -2,7 +2,10 @@ package e2e_test
 
 import (
 	"bytes"
+	"fmt"
+	"strconv"
 	"testing"
+	"time"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	transfertypes "github.com/cosmos/ibc-go/v3/modules/apps/transfer/types"
@@ -11,6 +14,13 @@ import (
 	channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
 	ibctesting "github.com/cosmos/ibc-go/v3/testing"
 
+	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
+	bankkeeper "github.com/cosmos/cosmos-sdk/x/bank/keeper"
+	govkeeper "github.com/cosmos/cosmos-sdk/x/gov/keeper"
+	govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
+	minttypes "github.com/cosmos/cosmos-sdk/x/mint/types"
+	"github.com/cosmos/cosmos-sdk/x/params/types/proposal"
+	proposaltypes "github.com/cosmos/cosmos-sdk/x/params/types/proposal"
 	appConsumer "github.com/cosmos/interchain-security/app/consumer-democracy"
 	appProvider "github.com/cosmos/interchain-security/app/provider"
 	"github.com/cosmos/interchain-security/testutil/simapp"
@@ -253,6 +263,119 @@ func (s *ConsumerDemocracyTestSuite) TestDemocracyRewarsDistribution() {
 	}
 }
 
+func (s *ConsumerDemocracyTestSuite) TestDemocracyGovernanceWhitelisting() {
+	govKeeper := s.consumerChain.App.(*appConsumer.App).GovKeeper
+	stakingKeeper := s.consumerChain.App.(*appConsumer.App).StakingKeeper
+	bankKeeper := s.consumerChain.App.(*appConsumer.App).BankKeeper
+	authKeeper := s.consumerChain.App.(*appConsumer.App).AccountKeeper
+	mintKeeper := s.consumerChain.App.(*appConsumer.App).MintKeeper
+	newAuthParamValue := uint64(128)
+	newMintParamValue := sdk.NewDecWithPrec(1, 1) // "0.100000000000000000"
+	allowedChange := proposal.ParamChange{Subspace: minttypes.ModuleName, Key: "InflationMax", Value: fmt.Sprintf("\"%s\"", newMintParamValue)}
+	forbiddenChange := proposal.ParamChange{Subspace: authtypes.ModuleName, Key: "MaxMemoCharacters", Value: fmt.Sprintf("\"%s\"", strconv.FormatUint(newAuthParamValue, 10))}
+	votingAccounts := s.consumerChain.SenderAccounts
+	bondDenom := stakingKeeper.BondDenom(s.consumerCtx())
+	depositAmount := govKeeper.GetDepositParams(s.consumerCtx()).MinDeposit
+	votingParams := govKeeper.GetVotingParams(s.consumerCtx())
+	votingParams.VotingPeriod = 3 * time.Second
+	govKeeper.SetVotingParams(s.consumerCtx(), votingParams)
+	s.consumerChain.NextBlock()
+	votersOldBalances := getAccountsBalances(s.consumerCtx(), bankKeeper, bondDenom, votingAccounts)
+
+	//submit proposal with forbidden and allowed changes
+	paramChange := proposaltypes.ParameterChangeProposal{Changes: []proposaltypes.ParamChange{allowedChange, forbiddenChange}}
+	err := submitProposalWithDepositAndVote(govKeeper, s.consumerCtx(), paramChange, votingAccounts, depositAmount)
+	s.Assert().NoError(err)
+	//set current header time to be equal or later than voting end time in order to process proposal from active queue,
+	//once the proposal is added to the chain
+	s.consumerChain.CurrentHeader.Time = s.consumerChain.CurrentHeader.Time.Add(votingParams.VotingPeriod)
+	s.consumerChain.NextBlock()
+	//at this moment, proposal is added, but not yet executed. we are saving old param values for comparison
+	oldAuthParamValue := authKeeper.GetParams(s.consumerCtx()).MaxMemoCharacters
+	oldMintParamValue := mintKeeper.GetParams(s.consumerCtx()).InflationMax
+	s.consumerChain.NextBlock()
+	//at this moment, proposal is executed or deleted if forbidden
+	currentAuthParamValue := authKeeper.GetParams(s.consumerCtx()).MaxMemoCharacters
+	currentMintParamValue := mintKeeper.GetParams(s.consumerCtx()).InflationMax
+	//check that parameters are not changed, since the proposal contained both forbidden and allowed changes
+	s.Assert().Equal(oldAuthParamValue, currentAuthParamValue)
+	s.Assert().NotEqual(newAuthParamValue, currentAuthParamValue)
+	s.Assert().Equal(oldMintParamValue, currentMintParamValue)
+	s.Assert().NotEqual(newMintParamValue, currentMintParamValue)
+	//deposit is refunded
+	s.Assert().Equal(votersOldBalances, getAccountsBalances(s.consumerCtx(), bankKeeper, bondDenom, votingAccounts))
+
+	//submit proposal with allowed changes
+	paramChange = proposaltypes.ParameterChangeProposal{Changes: []proposaltypes.ParamChange{allowedChange}}
+	err = submitProposalWithDepositAndVote(govKeeper, s.consumerCtx(), paramChange, votingAccounts, depositAmount)
+	s.Assert().NoError(err)
+	s.consumerChain.CurrentHeader.Time = s.consumerChain.CurrentHeader.Time.Add(votingParams.VotingPeriod)
+	s.consumerChain.NextBlock()
+	oldMintParamValue = mintKeeper.GetParams(s.consumerCtx()).InflationMax
+	s.consumerChain.NextBlock()
+	currentMintParamValue = mintKeeper.GetParams(s.consumerCtx()).InflationMax
+	//check that parameters are changed, since the proposal contained only allowed changes
+	s.Assert().Equal(newMintParamValue, currentMintParamValue)
+	s.Assert().NotEqual(oldMintParamValue, currentMintParamValue)
+	//deposit is refunded
+	s.Assert().Equal(votersOldBalances, getAccountsBalances(s.consumerCtx(), bankKeeper, bondDenom, votingAccounts))
+
+	//submit proposal with forbidden changes
+	paramChange = proposaltypes.ParameterChangeProposal{Changes: []proposaltypes.ParamChange{forbiddenChange}}
+	err = submitProposalWithDepositAndVote(govKeeper, s.consumerCtx(), paramChange, votingAccounts, depositAmount)
+	s.Assert().NoError(err)
+	s.consumerChain.CurrentHeader.Time = s.consumerChain.CurrentHeader.Time.Add(votingParams.VotingPeriod)
+	s.consumerChain.NextBlock()
+	oldAuthParamValue = authKeeper.GetParams(s.consumerCtx()).MaxMemoCharacters
+	s.consumerChain.NextBlock()
+	currentAuthParamValue = authKeeper.GetParams(s.consumerCtx()).MaxMemoCharacters
+	//check that parameters are not changed, since the proposal contained forbidden changes
+	s.Assert().Equal(oldAuthParamValue, currentAuthParamValue)
+	s.Assert().NotEqual(newAuthParamValue, currentAuthParamValue)
+	//deposit is refunded
+	s.Assert().Equal(votersOldBalances, getAccountsBalances(s.consumerCtx(), bankKeeper, bondDenom, votingAccounts))
+}
+
+func (s *ConsumerDemocracyTestSuite) TestDemocracyGovernanceWhitelistingKeys() {
+	paramKeeper := s.consumerChain.App.(*appConsumer.App).ParamsKeeper
+	for paramKey := range appConsumer.WhitelistedParams {
+		ss, ok := paramKeeper.GetSubspace(paramKey.Subspace)
+		s.Assert().True(ok)
+		hasKey := ss.Has(s.consumerCtx(), []byte(paramKey.Key))
+		s.Assert().True(hasKey, "Invalid key %s for subspace %s", paramKey.Key, paramKey.Subspace)
+	}
+}
+
+func submitProposalWithDepositAndVote(govKeeper govkeeper.Keeper, ctx sdk.Context, paramChange proposaltypes.ParameterChangeProposal,
+	accounts []ibctesting.SenderAccount, depositAmount sdk.Coins) error {
+	proposal, err := govKeeper.SubmitProposal(ctx, &paramChange)
+	if err != nil {
+		return err
+	}
+	_, err = govKeeper.AddDeposit(ctx, proposal.ProposalId, accounts[0].SenderAccount.GetAddress(), depositAmount) //proposal becomes active
+	if err != nil {
+		return err
+	}
+
+	for _, account := range accounts {
+		err = govKeeper.AddVote(ctx, proposal.ProposalId, account.SenderAccount.GetAddress(), govtypes.NewNonSplitVoteOption(govtypes.OptionYes))
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func getAccountsBalances(ctx sdk.Context, bankKeeper bankkeeper.Keeper, bondDenom string, accounts []ibctesting.SenderAccount) map[string]sdk.Int {
+	accountsBalances := map[string]sdk.Int{}
+	for _, acc := range accounts {
+		accountsBalances[string(acc.SenderAccount.GetAddress())] =
+			bankKeeper.GetBalance(ctx, acc.SenderAccount.GetAddress(), bondDenom).Amount
+	}
+
+	return accountsBalances
+}
+
 func (s *ConsumerDemocracyTestSuite) providerCtx() sdk.Context {
 	return s.providerChain.GetContext()
 }