fix!: validate MsgTransfer before calling Transfer() (#1244)

* validate MsgTransfer * add TestSendRewardsToProvider * update DefaultConsumerUnbondingPeriod to 14 days * update changelog * fix linter * fix test * apply review suggestions * update changelog
cosmos · Sep 5, 2023 · 840d290 · 840d290
1 parent 6da7fef
commit 840d290
Show file tree

Hide file tree

Showing 5 changed files with 219 additions and 55 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,7 @@
 
 Add an entry to the unreleased provider section whenever merging a PR to main that is not targeted at a specific release. These entries will eventually be included in a provider release.
 
+* (feature!) [#1244](https://github.com/cosmos/interchain-security/pull/1244) Update the default consumer unbonding period to 2 weeks.
 * (deps) [#1259](https://github.com/cosmos/interchain-security/pull/1259) Bump [cosmos-sdk](https://github.com/cosmos/cosmos-sdk) to [v0.47.5](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.5).
 * (deps!) [#1258](https://github.com/cosmos/interchain-security/pull/1258) Bump [ibc-go](https://github.com/cosmos/ibc-go) to [v7.3.0](https://github.com/cosmos/ibc-go/releases/tag/v7.3.0).
 * (deps) [#1258](https://github.com/cosmos/interchain-security/pull/1258) Bump [cosmos-sdk](https://github.com/cosmos/cosmos-sdk) to [v0.47.4](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.4).
@@ -14,6 +15,8 @@ Add an entry to the unreleased provider section whenever merging a PR to main th
 
 Add an entry to the unreleased consumer section whenever merging a PR to main that is not targeted at a specific release. These entries will eventually be included in a consumer release.
 
+* (feature!) [#1244](https://github.com/cosmos/interchain-security/pull/1244) Update the default consumer unbonding period to 2 weeks.
+* (fix!) [#1244](https://github.com/cosmos/interchain-security/pull/1244) Validate token transfer messages before calling `Transfer()`.
 * (deps) [#1259](https://github.com/cosmos/interchain-security/pull/1259) Bump [cosmos-sdk](https://github.com/cosmos/cosmos-sdk) to [v0.47.5](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.5).
 * (deps!) [#1258](https://github.com/cosmos/interchain-security/pull/1258) Bump [ibc-go](https://github.com/cosmos/ibc-go) to [v7.3.0](https://github.com/cosmos/ibc-go/releases/tag/v7.3.0).
 * (deps) [#1258](https://github.com/cosmos/interchain-security/pull/1258) Bump [cosmos-sdk](https://github.com/cosmos/cosmos-sdk) to [v0.47.4](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.4).

diff --git a/tests/integration/distribution.go b/tests/integration/distribution.go
@@ -9,6 +9,8 @@ import (
 	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
 	govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
 
+	icstestingutils "github.com/cosmos/interchain-security/v3/testutil/integration"
+	consumerkeeper "github.com/cosmos/interchain-security/v3/x/ccv/consumer/keeper"
 	consumertypes "github.com/cosmos/interchain-security/v3/x/ccv/consumer/types"
 	providertypes "github.com/cosmos/interchain-security/v3/x/ccv/provider/types"
 	ccv "github.com/cosmos/interchain-security/v3/x/ccv/types"
@@ -338,8 +340,151 @@ func (s *CCVTestSuite) TestEndBlockRD() {
 	}
 }
 
+// TestSendRewardsToProvider is effectively a unit test for SendRewardsToProvider(),
+// but is written as an integration test to avoid excessive mocking.
+func (s *CCVTestSuite) TestSendRewardsToProvider() {
+	testCases := []struct {
+		name           string
+		setup          func(sdk.Context, *consumerkeeper.Keeper, icstestingutils.TestBankKeeper)
+		expError       bool
+		tokenTransfers int
+	}{
+		{
+			name: "successful token transfer",
+			setup: func(ctx sdk.Context, keeper *consumerkeeper.Keeper, bankKeeper icstestingutils.TestBankKeeper) {
+				s.SetupTransferChannel()
+
+				// register a consumer reward denom
+				params := keeper.GetConsumerParams(ctx)
+				params.RewardDenoms = []string{sdk.DefaultBondDenom}
+				keeper.SetParams(ctx, params)
+
+				// send coins to the pool which is used for collect reward distributions to be sent to the provider
+				err := bankKeeper.SendCoinsFromAccountToModule(
+					ctx,
+					s.consumerChain.SenderAccount.GetAddress(),
+					consumertypes.ConsumerToSendToProviderName,
+					sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))),
+				)
+				s.Require().NoError(err)
+			},
+			expError:       false,
+			tokenTransfers: 1,
+		},
+		{
+			name: "no transfer channel",
+			setup: func(ctx sdk.Context, keeper *consumerkeeper.Keeper, bankKeeper icstestingutils.TestBankKeeper) {
+			},
+			expError:       false,
+			tokenTransfers: 0,
+		},
+		{
+			name: "no reward denom",
+			setup: func(ctx sdk.Context, keeper *consumerkeeper.Keeper, bankKeeper icstestingutils.TestBankKeeper) {
+				s.SetupTransferChannel()
+			},
+			expError:       false,
+			tokenTransfers: 0,
+		},
+		{
+			name: "reward balance is zero",
+			setup: func(ctx sdk.Context, keeper *consumerkeeper.Keeper, bankKeeper icstestingutils.TestBankKeeper) {
+				s.SetupTransferChannel()
+
+				// register a consumer reward denom
+				params := keeper.GetConsumerParams(ctx)
+				params.RewardDenoms = []string{"uatom"}
+				keeper.SetParams(ctx, params)
+
+				denoms := keeper.AllowedRewardDenoms(ctx)
+				s.Require().Len(denoms, 1)
+			},
+			expError:       false,
+			tokenTransfers: 0,
+		},
+		{
+			name: "no distribution transmission channel",
+			setup: func(ctx sdk.Context, keeper *consumerkeeper.Keeper, bankKeeper icstestingutils.TestBankKeeper) {
+				s.SetupTransferChannel()
+
+				// register a consumer reward denom
+				params := keeper.GetConsumerParams(ctx)
+				params.RewardDenoms = []string{sdk.DefaultBondDenom}
+				params.DistributionTransmissionChannel = ""
+				keeper.SetParams(ctx, params)
+
+				// send coins to the pool which is used for collect reward distributions to be sent to the provider
+				err := bankKeeper.SendCoinsFromAccountToModule(
+					ctx,
+					s.consumerChain.SenderAccount.GetAddress(),
+					consumertypes.ConsumerToSendToProviderName,
+					sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))),
+				)
+				s.Require().NoError(err)
+			},
+			expError:       false,
+			tokenTransfers: 0,
+		},
+		{
+			name: "no recipient address",
+			setup: func(ctx sdk.Context, keeper *consumerkeeper.Keeper, bankKeeper icstestingutils.TestBankKeeper) {
+				s.SetupTransferChannel()
+
+				// register a consumer reward denom
+				params := keeper.GetConsumerParams(ctx)
+				params.RewardDenoms = []string{sdk.DefaultBondDenom}
+				params.ProviderFeePoolAddrStr = ""
+				keeper.SetParams(ctx, params)
+
+				// send coins to the pool which is used for collect reward distributions to be sent to the provider
+				err := bankKeeper.SendCoinsFromAccountToModule(
+					ctx,
+					s.consumerChain.SenderAccount.GetAddress(),
+					consumertypes.ConsumerToSendToProviderName,
+					sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))),
+				)
+				s.Require().NoError(err)
+			},
+			expError:       true,
+			tokenTransfers: 0,
+		},
+	}
+
+	for _, tc := range testCases {
+		s.SetupTest()
+
+		// ccv channels setup
+		s.SetupCCVChannel(s.path)
+		bondAmt := sdk.NewInt(10000000)
+		delAddr := s.providerChain.SenderAccount.GetAddress()
+		delegate(s, delAddr, bondAmt)
+		s.providerChain.NextBlock()
+
+		// customized setup
+		consumerCtx := s.consumerCtx()
+		consumerKeeper := s.consumerApp.GetConsumerKeeper()
+		tc.setup(consumerCtx, &consumerKeeper, s.consumerApp.GetTestBankKeeper())
+
+		// call SendRewardsToProvider
+		err := s.consumerApp.GetConsumerKeeper().SendRewardsToProvider(consumerCtx)
+		if tc.expError {
+			s.Require().Error(err)
+		} else {
+			s.Require().NoError(err)
+		}
+
+		// check whether the amount of token transfers is as expected
+		commitments := s.consumerApp.GetIBCKeeper().ChannelKeeper.GetAllPacketCommitmentsAtChannel(
+			consumerCtx,
+			transfertypes.PortID,
+			s.consumerApp.GetConsumerKeeper().GetDistributionTransmissionChannel(consumerCtx),
+		)
+		s.Require().Len(commitments, tc.tokenTransfers, "unexpected amount of token transfers; test: %s", tc.name)
+	}
+}
+
 // getEscrowBalance gets the current balances in the escrow account holding the transferred tokens to the provider
-func (s CCVTestSuite) getEscrowBalance() sdk.Coins { //nolint:govet // we copy locks for this test
+func (s *CCVTestSuite) getEscrowBalance() sdk.Coins {
 	consumerBankKeeper := s.consumerApp.GetTestBankKeeper()
 	transChanID := s.consumerApp.GetConsumerKeeper().GetDistributionTransmissionChannel(s.consumerCtx())
 	escAddr := transfertypes.GetEscrowAddress(transfertypes.PortID, transChanID)

diff --git a/testutil/integration/debug_test.go b/testutil/integration/debug_test.go
@@ -85,6 +85,10 @@ func TestEndBlockRD(t *testing.T) {
 	runCCVTestByName(t, "TestEndBlockRD")
 }
 
+func TestSendRewardsToProvider(t *testing.T) {
+	runCCVTestByName(t, "TestSendRewardsToProvider")
+}
+
 //
 // Expired client tests
 //

diff --git a/x/ccv/consumer/keeper/distribution.go b/x/ccv/consumer/keeper/distribution.go
@@ -103,62 +103,74 @@ func (k Keeper) shouldSendRewardsToProvider(ctx sdk.Context) bool {
 // all the block rewards allocated for the provider
 func (k Keeper) SendRewardsToProvider(ctx sdk.Context) error {
 	// empty out the toSendToProviderTokens address
-	ch := k.GetDistributionTransmissionChannel(ctx)
-	transferChannel, found := k.channelKeeper.GetChannel(ctx, transfertypes.PortID, ch)
-	if found && transferChannel.State == channeltypes.OPEN {
-		tstProviderAddr := k.authKeeper.GetModuleAccount(ctx,
-			types.ConsumerToSendToProviderName).GetAddress()
-		providerAddr := k.GetProviderFeePoolAddrStr(ctx)
-		timeoutHeight := clienttypes.ZeroHeight()
-		transferTimeoutPeriod := k.GetTransferTimeoutPeriod(ctx)
-		timeoutTimestamp := uint64(ctx.BlockTime().Add(transferTimeoutPeriod).UnixNano())
-
-		sentCoins := sdk.NewCoins()
-		var allBalances sdk.Coins
-		// iterate over all whitelisted reward denoms
-		for _, denom := range k.AllowedRewardDenoms(ctx) {
-			// get the balance of the denom in the toSendToProviderTokens address
-			balance := k.bankKeeper.GetBalance(ctx, tstProviderAddr, denom)
-			allBalances = allBalances.Add(balance)
-
-			// if the balance is not zero,
-			if !balance.IsZero() {
-				packetTransfer := &transfertypes.MsgTransfer{
-					SourcePort:       transfertypes.PortID,
-					SourceChannel:    ch,
-					Token:            balance,
-					Sender:           tstProviderAddr.String(), // consumer address to send from
-					Receiver:         providerAddr,             // provider fee pool address to send to
-					TimeoutHeight:    timeoutHeight,            // timeout height disabled
-					TimeoutTimestamp: timeoutTimestamp,
-					Memo:             "consumer chain rewards distribution",
-				}
-				_, err := k.ibcTransferKeeper.Transfer(ctx, packetTransfer)
-				if err != nil {
-					return err
-				}
-				sentCoins = sentCoins.Add(balance)
+	sourceChannelID := k.GetDistributionTransmissionChannel(ctx)
+	transferChannel, found := k.channelKeeper.GetChannel(ctx, transfertypes.PortID, sourceChannelID)
+	if !found || transferChannel.State != channeltypes.OPEN {
+		k.Logger(ctx).Info("WARNING: cannot send rewards to provider;",
+			"transmission channel not in OPEN state", "channelID", sourceChannelID)
+		return nil
+	}
+
+	// get params for sending rewards
+	toSendToProviderAddr := k.authKeeper.GetModuleAccount(ctx,
+		types.ConsumerToSendToProviderName).GetAddress() // sender address
+	providerAddr := k.GetProviderFeePoolAddrStr(ctx) // recipient address
+	timeoutHeight := clienttypes.ZeroHeight()
+	timeoutTimestamp := uint64(ctx.BlockTime().Add(k.GetTransferTimeoutPeriod(ctx)).UnixNano())
+
+	sentCoins := sdk.NewCoins()
+	var allBalances sdk.Coins
+	// iterate over all whitelisted reward denoms
+	for _, denom := range k.AllowedRewardDenoms(ctx) {
+		// get the balance of the denom in the toSendToProviderTokens address
+		balance := k.bankKeeper.GetBalance(ctx, toSendToProviderAddr, denom)
+		allBalances = allBalances.Add(balance)
+
+		// if the balance is not zero,
+		if !balance.IsZero() {
+			packetTransfer := &transfertypes.MsgTransfer{
+				SourcePort:       transfertypes.PortID,
+				SourceChannel:    sourceChannelID,
+				Token:            balance,
+				Sender:           toSendToProviderAddr.String(), // consumer address to send from
+				Receiver:         providerAddr,                  // provider fee pool address to send to
+				TimeoutHeight:    timeoutHeight,                 // timeout height disabled
+				TimeoutTimestamp: timeoutTimestamp,
+				Memo:             "consumer chain rewards distribution",
 			}
-		}
 
-		k.Logger(ctx).Info("sent block rewards to provider",
-			"total fee pool", allBalances.String(),
-			"sent", sentCoins.String(),
-		)
-		currentHeight := ctx.BlockHeight()
-		ctx.EventManager().EmitEvent(
-			sdk.NewEvent(
-				ccv.EventTypeFeeDistribution,
-				sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName),
-				sdk.NewAttribute(ccv.AttributeDistributionCurrentHeight, strconv.Itoa(int(currentHeight))),
-				sdk.NewAttribute(ccv.AttributeDistributionNextHeight, strconv.Itoa(int(currentHeight+k.GetBlocksPerDistributionTransmission(ctx)))),
-				sdk.NewAttribute(ccv.AttributeDistributionFraction, (k.GetConsumerRedistributionFrac(ctx))),
-				sdk.NewAttribute(ccv.AttributeDistributionTotal, allBalances.String()),
-				sdk.NewAttribute(ccv.AttributeDistributionToProvider, sentCoins.String()),
-			),
-		)
+			// validate MsgTransfer before calling Transfer()
+			err := packetTransfer.ValidateBasic()
+			if err != nil {
+				return err
+			}
+
+			_, err = k.ibcTransferKeeper.Transfer(ctx, packetTransfer)
+			if err != nil {
+				return err
+			}
+
+			sentCoins = sentCoins.Add(balance)
+		}
 	}
 
+	k.Logger(ctx).Info("sent block rewards to provider",
+		"total fee pool", allBalances.String(),
+		"sent", sentCoins.String(),
+	)
+	currentHeight := ctx.BlockHeight()
+	ctx.EventManager().EmitEvent(
+		sdk.NewEvent(
+			ccv.EventTypeFeeDistribution,
+			sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName),
+			sdk.NewAttribute(ccv.AttributeDistributionCurrentHeight, strconv.Itoa(int(currentHeight))),
+			sdk.NewAttribute(ccv.AttributeDistributionNextHeight, strconv.Itoa(int(currentHeight+k.GetBlocksPerDistributionTransmission(ctx)))),
+			sdk.NewAttribute(ccv.AttributeDistributionFraction, (k.GetConsumerRedistributionFrac(ctx))),
+			sdk.NewAttribute(ccv.AttributeDistributionTotal, allBalances.String()),
+			sdk.NewAttribute(ccv.AttributeDistributionToProvider, sentCoins.String()),
+		),
+	)
+
 	return nil
 }
 

diff --git a/x/ccv/types/params.go b/x/ccv/types/params.go
@@ -32,10 +32,10 @@ const (
 	// (and for consistency with other protobuf schemas defined for ccv).
 	DefaultHistoricalEntries = int64(stakingtypes.DefaultHistoricalEntries)
 
-	// In general, the default unbonding period on the consumer is one day less
+	// In general, the default unbonding period on the consumer is one week less
 	// than the default unbonding period on the provider, where the provider uses
 	// the staking module default.
-	DefaultConsumerUnbondingPeriod = stakingtypes.DefaultUnbondingTime - 24*time.Hour
+	DefaultConsumerUnbondingPeriod = stakingtypes.DefaultUnbondingTime - 7*24*time.Hour
 
 	// By default, the bottom 5% of the validator set can opt out of validating consumer chains
 	DefaultSoftOptOutThreshold = "0.05"