From 840d290d2b9b30169a59153a120a8ea74dd1accd Mon Sep 17 00:00:00 2001 From: Marius Poke Date: Tue, 5 Sep 2023 15:05:56 +0200 Subject: [PATCH] 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 --- CHANGELOG.md | 3 + tests/integration/distribution.go | 147 +++++++++++++++++++++++++- testutil/integration/debug_test.go | 4 + x/ccv/consumer/keeper/distribution.go | 116 +++++++++++--------- x/ccv/types/params.go | 4 +- 5 files changed, 219 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b145ff740e..353d7226f1 100644 --- 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 index 5d5c50220a..65b82eecd6 100644 --- 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 index 828c9b6810..6b9415440e 100644 --- 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 index 5fe416ea2b..a2c19b495d 100644 --- 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 index e8ea0a8765..2b553e8366 100644 --- 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"