fix!: prevent denom DOS (#931)

* Merge pull request from GHSA-chqw-ff63-95r8

* squash commit of multisig fix + everything involving denom fix

* rebuild proto

* fix todos

---------

Co-authored-by: Jehan Tremback <hi@jehan.email>

* regen proto

* fix cherrypick issues

* lint

* cleans

* gosec

* restore param, remove tech debt from tests

* ibc denom as const

* add check for consumer reward denom already registered

* lint

* remove unneeded expect

---------

Co-authored-by: Jehan Tremback <hi@jehan.email>
Co-authored-by: Marius Poke <marius.poke@posteo.de>

app/provider/app.go

@@ -155,13 +155,14 @@ var (
 
 	// module account permissions
 	maccPerms = map[string][]string{
-		authtypes.FeeCollectorName:     nil,
-		distrtypes.ModuleName:          nil,
-		minttypes.ModuleName:           {authtypes.Minter},
-		stakingtypes.BondedPoolName:    {authtypes.Burner, authtypes.Staking},
-		stakingtypes.NotBondedPoolName: {authtypes.Burner, authtypes.Staking},
-		govtypes.ModuleName:            {authtypes.Burner},
-		ibctransfertypes.ModuleName:    {authtypes.Minter, authtypes.Burner},
+		authtypes.FeeCollectorName:        nil,
+		distrtypes.ModuleName:             nil,
+		minttypes.ModuleName:              {authtypes.Minter},
+		stakingtypes.BondedPoolName:       {authtypes.Burner, authtypes.Staking},
+		stakingtypes.NotBondedPoolName:    {authtypes.Burner, authtypes.Staking},
+		govtypes.ModuleName:               {authtypes.Burner},
+		ibctransfertypes.ModuleName:       {authtypes.Minter, authtypes.Burner},
+		providertypes.ConsumerRewardsPool: nil,
 	}
 )
 
@@ -308,12 +309,12 @@ func New(
 		maccPerms,
 	)
 
-	// Remove the fee-pool from the group of blocked recipient addresses in bank
+	// Remove the ConsumerRewardsPool from the group of blocked recipient addresses in bank
 	// this is required for the provider chain to be able to receive tokens from
 	// the consumer chain
 	bankBlockedAddrs := app.ModuleAccountAddrs()
 	delete(bankBlockedAddrs, authtypes.NewModuleAddress(
-		authtypes.FeeCollectorName).String())
+		providertypes.ConsumerRewardsPool).String())
 
 	app.BankKeeper = bankkeeper.NewBaseKeeper(
 		appCodec,
@@ -409,6 +410,8 @@ func New(
 		app.SlashingKeeper,
 		app.AccountKeeper,
 		app.EvidenceKeeper,
+		app.DistrKeeper,
+		app.BankKeeper,
 		authtypes.FeeCollectorName,
 	)
 
@@ -777,6 +780,11 @@ func (app *App) GetTestDistributionKeeper() testutil.TestDistributionKeeper {
 	return app.DistrKeeper
 }
 
+// GetTestAccountKeeper implements the ProviderApp interface.
+func (app *App) GetTestAccountKeeper() testutil.TestAccountKeeper {
+	return app.AccountKeeper
+}
+
 // TestingApp functions
 
 // GetBaseApp implements the TestingApp interface.

proto/interchain_security/ccv/consumer/v1/consumer.proto

@@ -57,6 +57,13 @@ message Params {
   // can opt out of running the consumer chain without being punished. For example, a
   // value of 0.05 means that the validators in the bottom 5% of the set can opt out
   string soft_opt_out_threshold = 10;
+
+  // Reward denoms. These are the denominations which are allowed to be sent to the provider as rewards.
+  repeated string reward_denoms = 11;
+
+  // Provider-originated reward denoms. These are denoms coming from the provider 
+  // which are allowed to be used as rewards. e.g. "uatom"
+  repeated string provider_reward_denoms = 12;
 }
 
 // LastTransmissionBlockHeight is the last time validator holding

proto/interchain_security/ccv/provider/v1/provider.proto

@@ -11,6 +11,7 @@ import "ibc/core/client/v1/client.proto";
 import "ibc/lightclients/tendermint/v1/tendermint.proto";
 import "tendermint/crypto/keys.proto";
 import "cosmos/evidence/v1beta1/evidence.proto";
+import "cosmos/base/v1beta1/coin.proto";
 
 // ConsumerAdditionProposal is a governance proposal on the provider chain to spawn a new consumer chain.
 // If it passes, then all validators on the provider chain are expected to validate the consumer chain at spawn time
@@ -136,6 +137,10 @@ message Params {
   // The maximum amount of throttled slash or vsc matured packets 
   // that can be queued for a single consumer before the provider chain halts.
   int64 max_throttled_packets = 8;
+
+  // The fee required to be paid to add a reward denom
+  cosmos.base.v1beta1.Coin consumer_reward_denom_registration_fee = 9
+      [(gogoproto.nullable) = false];
 }
 
 message HandshakeMetadata {

proto/interchain_security/ccv/provider/v1/query.proto

@@ -68,6 +68,12 @@ service Query {
       returns (QueryThrottledConsumerPacketDataResponse) {
     option (google.api.http).get = "/interchain_security/ccv/provider/pending_consumer_packets";
   }
+
+  // QueryRegisteredConsumerRewardDenoms returns a list of consumer reward denoms that are registered
+  rpc QueryRegisteredConsumerRewardDenoms(QueryRegisteredConsumerRewardDenomsRequest)
+      returns (QueryRegisteredConsumerRewardDenomsResponse) {
+    option (google.api.http).get = "/interchain_security/ccv/provider/registered_consumer_reward_denoms";
+  }
 }
 
 message QueryConsumerGenesisRequest { string chain_id = 1; }
@@ -169,3 +175,9 @@ message ThrottledPacketDataWrapper {
     interchain_security.ccv.v1.VSCMaturedPacketData vsc_matured_packet = 2;
   }
 }
+
+message QueryRegisteredConsumerRewardDenomsRequest {}
+
+message QueryRegisteredConsumerRewardDenomsResponse {
+  repeated string denoms = 1;
+}

proto/interchain_security/ccv/provider/v1/tx.proto

@@ -11,6 +11,7 @@ import "google/protobuf/any.proto";
 // Msg defines the Msg service.
 service Msg {
   rpc AssignConsumerKey(MsgAssignConsumerKey) returns (MsgAssignConsumerKeyResponse);
+  rpc RegisterConsumerRewardDenom(MsgRegisterConsumerRewardDenom) returns (MsgRegisterConsumerRewardDenomResponse);
 }
 
 message MsgAssignConsumerKey {
@@ -27,4 +28,18 @@ message MsgAssignConsumerKey {
   string consumer_key = 3;
 }
 
-message MsgAssignConsumerKeyResponse {}
+message MsgAssignConsumerKeyResponse {}
+
+// MsgRegisterConsumerRewardDenom allows an account to register 
+// a consumer reward denom, i.e., add it to the list of denoms 
+// accepted by the provider as rewards.
+message MsgRegisterConsumerRewardDenom {
+  option (gogoproto.equal)           = false;
+  option (gogoproto.goproto_getters) = false;
+
+  string denom = 1;
+  string depositor = 2;
+}
+
+// MsgRegisterConsumerRewardDenomResponse defines the Msg/RegisterConsumerRewardDenom response type.
+message MsgRegisterConsumerRewardDenomResponse {}

tests/difference/core/driver/setup.go

@@ -514,6 +514,8 @@ func (b *Builder) createConsumerGenesis(client *ibctmtypes.ClientState) *consume
 		consumertypes.DefaultHistoricalEntries,
 		b.initState.UnbondingC,
 		"0", // disable soft opt-out
+		[]string{},
+		[]string{},
 	)
 	return consumertypes.NewInitialGenesisState(client, providerConsState, valUpdates, params)
 }

tests/e2e/actions.go

@@ -1160,6 +1160,36 @@ func (tr TestRun) registerRepresentative(
 	wg.Wait()
 }
 
+type registerConsumerRewardDenomAction struct {
+	chain chainID
+	from  validatorID
+	denom string
+}
+
+func (tr TestRun) registerConsumerRewardDenom(action registerConsumerRewardDenomAction, verbose bool) {
+	//#nosec G204 -- Bypass linter warning for spawning subprocess with cmd arguments.
+	bz, err := exec.Command("docker", "exec", tr.containerConfig.instanceName, tr.chainConfigs[action.chain].binaryName,
+		"tx", "provider", "register-consumer-reward-denom", action.denom,
+
+		`--from`, `validator`+fmt.Sprint(action.from),
+		`--chain-id`, string(action.chain),
+		`--home`, tr.getValidatorHome(action.chain, action.from),
+		`--node`, tr.getValidatorNode(action.chain, action.from),
+		`--gas`, "9000000",
+		`--keyring-backend`, `test`,
+		`-b`, `block`,
+		`-y`,
+	).CombinedOutput()
+
+	if verbose {
+		fmt.Println("redelegate cmd:", string(bz))
+	}
+
+	if err != nil {
+		log.Fatal(err, "\n", string(bz))
+	}
+}
+
 // Creates an additional node on selected chain
 // by copying an existing validator's home folder
 //

tests/e2e/config.go

@@ -223,7 +223,19 @@ func DefaultTestRun() TestRun {
 	}
 }
 
-func DemocracyTestRun() TestRun {
+func DemocracyTestRun(allowReward bool) TestRun {
+	consumerGenChanges := ".app_state.ccvconsumer.params.blocks_per_distribution_transmission = \"20\" | " +
+		".app_state.gov.voting_params.voting_period = \"10s\" | " +
+		".app_state.slashing.params.signed_blocks_window = \"2\" | " +
+		".app_state.slashing.params.min_signed_per_window = \"0.500000000000000000\" | " +
+		".app_state.slashing.params.downtime_jail_duration = \"2s\" | " +
+		".app_state.slashing.params.slash_fraction_downtime = \"0.010000000000000000\""
+
+	if allowReward {
+		// This allows the consumer chain to send rewards in the stake denom
+		consumerGenChanges += " | .app_state.ccvconsumer.params.reward_denoms = [\"stake\"]"
+	}
+
 	return TestRun{
 		name: "democracy",
 		containerConfig: ContainerConfig{
@@ -253,12 +265,7 @@ func DemocracyTestRun() TestRun {
 				binaryName:     "interchain-security-cdd",
 				ipPrefix:       "7.7.9",
 				votingWaitTime: 20,
-				genesisChanges: ".app_state.ccvconsumer.params.blocks_per_distribution_transmission = \"20\" | " +
-					".app_state.gov.voting_params.voting_period = \"10s\" | " +
-					".app_state.slashing.params.signed_blocks_window = \"2\" | " +
-					".app_state.slashing.params.min_signed_per_window = \"0.500000000000000000\" | " +
-					".app_state.slashing.params.downtime_jail_duration = \"2s\" | " +
-					".app_state.slashing.params.slash_fraction_downtime = \"0.010000000000000000\"",
+				genesisChanges: consumerGenChanges,
 			},
 		},
 		tendermintConfigOverride: `s/timeout_commit = "5s"/timeout_commit = "1s"/;` +

tests/e2e/main.go

@@ -44,7 +44,8 @@ func main() {
 
 	testRuns := []testRunWithSteps{
 		{DefaultTestRun(), happyPathSteps},
-		{DemocracyTestRun(), democracySteps},
+		{DemocracyTestRun(true), democracySteps},
+		{DemocracyTestRun(false), rewardDenomConsumerSteps},
 		{SlashThrottleTestRun(), slashThrottleSteps},
 	}
 	if includeMultiConsumer != nil && *includeMultiConsumer {
@@ -143,6 +144,8 @@ func (tr *TestRun) runStep(step Step, verbose bool) {
 		tr.waitForSlashThrottleDequeue(action, verbose)
 	case startHermesAction:
 		tr.startHermes(action, verbose)
+	case registerConsumerRewardDenomAction:
+		tr.registerConsumerRewardDenom(action, verbose)
 	default:
 		log.Fatalf("unknown action in testRun %s: %#v", tr.name, action)
 	}

tests/e2e/state.go

@@ -16,17 +16,18 @@ import (
 type State map[chainID]ChainState
 
 type ChainState struct {
-	ValBalances             *map[validatorID]uint
-	Proposals               *map[uint]Proposal
-	ValPowers               *map[validatorID]uint
-	RepresentativePowers    *map[validatorID]uint
-	Params                  *[]Param
-	Rewards                 *Rewards
-	ConsumerChains          *map[chainID]bool
-	AssignedKeys            *map[validatorID]string
-	ProviderKeys            *map[validatorID]string // validatorID: validator provider key
-	ConsumerChainQueueSizes *map[chainID]uint
-	GlobalSlashQueueSize    *uint
+	ValBalances                    *map[validatorID]uint
+	Proposals                      *map[uint]Proposal
+	ValPowers                      *map[validatorID]uint
+	RepresentativePowers           *map[validatorID]uint
+	Params                         *[]Param
+	Rewards                        *Rewards
+	ConsumerChains                 *map[chainID]bool
+	AssignedKeys                   *map[validatorID]string
+	ProviderKeys                   *map[validatorID]string // validatorID: validator provider key
+	ConsumerChainQueueSizes        *map[chainID]uint
+	GlobalSlashQueueSize           *uint
+	RegisteredConsumerRewardDenoms *[]string
 }
 
 type Proposal interface {
@@ -167,6 +168,11 @@ func (tr TestRun) getChainState(chain chainID, modelState ChainState) ChainState
 		chainState.ConsumerChainQueueSizes = &consumerChainQueueSizes
 	}
 
+	if modelState.RegisteredConsumerRewardDenoms != nil {
+		registeredConsumerRewardDenoms := tr.getRegisteredConsumerRewardDenoms(chain)
+		chainState.RegisteredConsumerRewardDenoms = &registeredConsumerRewardDenoms
+	}
+
 	return chainState
 }
 
@@ -640,6 +646,28 @@ func (tr TestRun) getConsumerChainPacketQueueSize(consumerChain chainID) uint {
 	return uint(size)
 }
 
+func (tr TestRun) getRegisteredConsumerRewardDenoms(chain chainID) []string {
+	//#nosec G204 -- Bypass linter warning for spawning subprocess with cmd arguments.
+	cmd := exec.Command("docker", "exec", tr.containerConfig.instanceName, tr.chainConfigs[chain].binaryName,
+
+		"query", "provider", "registered-consumer-reward-denoms",
+		`--node`, tr.getQueryNode(chain),
+		`-o`, `json`,
+	)
+	bz, err := cmd.CombinedOutput()
+	if err != nil {
+		log.Fatal(err, "\n", string(bz))
+	}
+
+	denoms := gjson.Get(string(bz), "denoms").Array()
+	rewardDenoms := make([]string, len(denoms))
+	for i, d := range denoms {
+		rewardDenoms[i] = d.String()
+	}
+
+	return rewardDenoms
+}
+
 func (tr TestRun) getValidatorNode(chain chainID, validator validatorID) string {
 	return "tcp://" + tr.getValidatorIP(chain, validator) + ":26658"
 }

tests/e2e/steps.go

@@ -45,6 +45,14 @@ var democracySteps = concatSteps(
 	stepsDemocracy("democ"),
 )
 
+var rewardDenomConsumerSteps = concatSteps(
+	// democracySteps requires a transfer channel
+	stepsStartChains([]string{"democ"}, true),
+	// delegation needs to happen so the first VSC packet can be delivered
+	stepsDelegate("democ"),
+	stepsRewardDenomConsumer("democ"),
+)
+
 var multipleConsumers = concatSteps(
 	stepsStartChains([]string{"consu", "densu"}, false),
 	stepsMultiConsumerDelegate("consu", "densu"),

tests/e2e/steps_democracy.go

@@ -1,5 +1,7 @@
 package main
 
+const consumerRewardDenom = "ibc/3C3D7B3BE4ECC85A0E5B52A3AEC3B7DFC2AA9CA47C37821E57020D6807043BE9"
+
 func stepsDemocracy(consumerName string) []Step {
 	return []Step{
 		{
@@ -105,6 +107,47 @@ func stepsDemocracy(consumerName string) []Step {
 				},
 			},
 		},
+		{
+			action: relayRewardPacketsToProviderAction{
+				consumerChain: chainID(consumerName),
+				providerChain: chainID("provi"),
+				port:          "transfer",
+				channel:       1,
+			},
+			state: State{
+				chainID("provi"): ChainState{
+					// Check that tokens are not distributed before the denom has been registered
+					Rewards: &Rewards{
+						IsRewarded: map[validatorID]bool{
+							validatorID("alice"): false,
+							validatorID("bob"):   false,
+							validatorID("carol"): false,
+						},
+						IsIncrementalReward: false,
+						IsNativeDenom:       false,
+					},
+					// Check that the denom is not registered on provider chain
+					RegisteredConsumerRewardDenoms: &[]string{},
+				},
+			},
+		},
+		{
+			action: registerConsumerRewardDenomAction{
+				chain: chainID("provi"),
+				from:  validatorID("bob"),
+				denom: consumerRewardDenom,
+			},
+			state: State{
+				chainID("provi"): ChainState{
+					// Check that the denom is registered on provider chain
+					RegisteredConsumerRewardDenoms: &[]string{consumerRewardDenom},
+					ValBalances: &map[validatorID]uint{
+						// make sure that bob's account was debited
+						validatorID("bob"): 9490000000,
+					},
+				},
+			},
+		},
 		{
 			action: relayRewardPacketsToProviderAction{
 				consumerChain: chainID(consumerName),