fix!: validate MsgTransfer before calling Transfer()

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).

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)

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
 //

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
 }
 

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"