From 8349b828121701a76c2fab6843107de22aaa36ef Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Tue, 3 Oct 2023 16:27:09 +0200 Subject: [PATCH] fix nits --- CHANGELOG.md | 58 +++++++++++++++++++ tests/e2e/actions.go | 3 - tests/e2e/actions_consumer_misbehaviour.go | 21 ------- tests/e2e/main.go | 2 - tests/e2e/state.go | 9 +-- .../testnet-scripts/sovereign-genesis.json | 2 +- tests/e2e/testnet-scripts/start-chain.sh | 1 - .../proto/tendermint/types/evidence.proto | 38 ------------ 8 files changed, 60 insertions(+), 74 deletions(-) delete mode 100644 third_party/proto/tendermint/types/evidence.proto diff --git a/CHANGELOG.md b/CHANGELOG.md index a6e4ec6928..4716179956 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -156,6 +156,64 @@ Upgrading a consumer from `v1.2.0-multiden` to `v2.0.0` will NOT require state m * (api) Add consumer QueryParams [#746](https://github.com/cosmos/interchain-security/pull/746) * (feature) New validation for keeper fields [#740](https://github.com/cosmos/interchain-security/pull/740) + +## v1.2.0-multiden + +The first release candidate for a fix built on top of v1.2.0, intended for consumers. This release adds a list of denoms on the consumer that are allowed to be sent to the provider as rewards. This prevents a potential DOS attack that was discovered during the audit of Replicated Security performed by Oak Security and funded by the Cosmos Hub community through Proposal 687. In an effort to move quickly, this release also includes a multisig fix that is effective only for provider. It shouldn't affect the consumer module. + +Note PRs were made in a private security repo. + +[full diff](https://github.com/cosmos/interchain-security/compare/v1.2.0...v1.2.0-multiden-rc0) + +## v1.1.0-multiden + +This release combines two fixes on top of v1.1.0, that we judged were urgent to get onto the Cosmos Hub before the launch of the first ICS consumer chain. This is an emergency release intended for providers. + +The first fix is to enable the use of multisigs and Ledger devices when assigning keys for consumer chains. The second is to prevent a possible DOS vector involving the reward distribution system. + +Note PRs were made in a private security repo. + +[full diff](https://github.com/cosmos/interchain-security/compare/v1.1.0...release/v1.1.0-multiden) + +### Multisig fix + +On April 25th (a week and a half ago), we began receiving reports that validators using multisigs and Ledger devices were getting errors reading Error: unable to resolve type URL /interchain_security.ccv.provider.v1.MsgAssignConsumerKey: tx parse error when attempting to assign consensus keys for consumer chains. + +We quickly narrowed the problem down to issues having to do with using the PubKey type directly in the MsgAssignConsumerKey transaction, and Amino (a deprecated serialization library still used in Ledger devices and multisigs) not being able to handle this. We attempted to fix this with the assistance of the Cosmos-SDK team, but after making no headway for a few days, we decided to simply use a JSON representation of the PubKey in the transaction. This is how it is usually represented anyway. We have verified that this fixes the problem. + +### Distribution fix + +The ICS distribution system works by allowing consumer chains to send rewards to a module address on the provider called the FeePoolAddress. From here they are automatically distributed to all validators and delegators through the distribution system that already exists to distribute staking rewards. The FeePoolAddress is usually blocked so that no tokens can be sent to it, but to enable ICS distribution we had to unblock it. + +We recently realized that unblocking the FeePoolAddress could enable an attacker to send a huge number of different denoms into the distribution system. The distribution system would then attempt to distribute them all, leading to out of memory errors. Fixing a similar attack vector that existed in the distribution system before ICS led us to this realization. + +To fix this problem, we have re-blocked the FeePoolAddress and created a new address called the ConsumerRewardsPool. Consumer chains now send rewards to this new address. There is also a new transaction type called RegisterConsumerRewardDenom. This transaction allows people to register denoms to be used as rewards from consumer chains. It costs 10 Atoms to run this transaction.The Atoms are transferred to the community pool. Only denoms registered with this command are then transferred to the FeePoolAddress and distributed out to delegators and validators. + +## v1.2.1 + +* (fix) Remove SPM [#812](https://github.com/cosmos/interchain-security/pull/812) +* (refactor) Key assignment type safety [#725](https://github.com/cosmos/interchain-security/pull/725) + +## v1.2.0 + +Date: April 13th, 2023 + +* (feat) Soft opt-out [#833](https://github.com/cosmos/interchain-security/pull/833) +* (fix) Correctly handle VSC packet with duplicate val updates on consumer [#846](https://github.com/cosmos/interchain-security/pull/846) +* (chore) bump: sdk v0.45.15-ics [#805](https://github.com/cosmos/interchain-security/pull/805) +* (api) add interchain security consumer QueryParams [#746](https://github.com/cosmos/interchain-security/pull/746) + +## v1.1.1 + +* (fix) Remove SPM [#812](https://github.com/cosmos/interchain-security/pull/812) +* (refactor) Key assignment type safety [#725](https://github.com/cosmos/interchain-security/pull/725) + +## v1.1.0 + +Date: March 24th, 2023 + +* (fix) StopConsumerChain not running in cachedContext [#802](https://github.com/cosmos/interchain-security/pull/802) + ## v1.0.0 Date: February 6th, 2023 diff --git a/tests/e2e/actions.go b/tests/e2e/actions.go index c1f11bb62d..9dac11a608 100644 --- a/tests/e2e/actions.go +++ b/tests/e2e/actions.go @@ -1291,9 +1291,6 @@ func (tr TestConfig) relayRewardPacketsToProvider( ) { blockPerDistribution, _ := strconv.ParseUint(strings.Trim(tr.getParam(action.ConsumerChain, Param{Subspace: "ccvconsumer", Key: "BlocksPerDistributionTransmission"}), "\""), 10, 64) currentBlock := uint64(tr.getBlockHeight(action.ConsumerChain)) - fmt.Println("blockPerDistribution", blockPerDistribution) - fmt.Println("currentBlock", currentBlock) - if currentBlock <= blockPerDistribution { tr.waitBlocks(action.ConsumerChain, uint(blockPerDistribution-currentBlock+1), 60*time.Second) } diff --git a/tests/e2e/actions_consumer_misbehaviour.go b/tests/e2e/actions_consumer_misbehaviour.go index 46e349b356..7d71d58d1d 100644 --- a/tests/e2e/actions_consumer_misbehaviour.go +++ b/tests/e2e/actions_consumer_misbehaviour.go @@ -2,7 +2,6 @@ package main import ( "bufio" - "fmt" "log" "os/exec" "strconv" @@ -97,23 +96,3 @@ func (tc TestConfig) updateLightClient( tc.waitBlocks(action.HostChain, 5, 30*time.Second) } - -type assertChainIsHaltedAction struct { - chain ChainID -} - -// assertChainIsHalted verifies that the chain isn't producing blocks -// by checking that the block height is still the same after 20 seconds -func (tc TestConfig) assertChainIsHalted( - action assertChainIsHaltedAction, - verbose bool, -) { - blockHeight := tc.getBlockHeight(action.chain) - time.Sleep(20 * time.Second) - if blockHeight != tc.getBlockHeight(action.chain) { - panic(fmt.Sprintf("chain %v isn't expected to produce blocks", action.chain)) - } - if verbose { - log.Printf("assertChainIsHalted - chain %v was successfully halted\n", action.chain) - } -} diff --git a/tests/e2e/main.go b/tests/e2e/main.go index 17b33545b5..fb73b6ee8f 100644 --- a/tests/e2e/main.go +++ b/tests/e2e/main.go @@ -393,8 +393,6 @@ func (tr *TestConfig) runStep(step Step, verbose bool) { tr.forkConsumerChain(action, verbose) case updateLightClientAction: tr.updateLightClient(action, verbose) - case assertChainIsHaltedAction: - tr.assertChainIsHalted(action, verbose) case startConsumerEvidenceDetectorAction: tr.startConsumerEvidenceDetector(action, verbose) case submitChangeRewardDenomsProposalAction: diff --git a/tests/e2e/state.go b/tests/e2e/state.go index 11cc8cc03e..a2a4bdbeac 100644 --- a/tests/e2e/state.go +++ b/tests/e2e/state.go @@ -26,7 +26,7 @@ type ChainState struct { Rewards *Rewards ConsumerChains *map[ChainID]bool AssignedKeys *map[ValidatorID]string - ProviderKeys *map[ValidatorID]string // ValidatorID: validator provider key + ProviderKeys *map[ValidatorID]string // validatorID: validator provider key ConsumerChainQueueSizes *map[ChainID]uint GlobalSlashQueueSize *uint RegisteredConsumerRewardDenoms *[]string @@ -324,13 +324,6 @@ func (tr TestConfig) getReward(chain ChainID, validator ValidatorID, blockHeight delAddresss = tr.validatorConfigs[validator].ConsumerDelAddress } - fmt.Println("getReward", tr.containerConfig.InstanceName, tr.chainConfigs[chain].BinaryName, - "query", "distribution", "rewards", - delAddresss, - - `--height`, fmt.Sprint(blockHeight), - `--node`, tr.getQueryNode(chain), - `-o`, `json`) //#nosec G204 -- Bypass linter warning for spawning subprocess with cmd arguments. bz, err := exec.Command("docker", "exec", tr.containerConfig.InstanceName, tr.chainConfigs[chain].BinaryName, diff --git a/tests/e2e/testnet-scripts/sovereign-genesis.json b/tests/e2e/testnet-scripts/sovereign-genesis.json index 2ab953b162..c3ae9da36c 100644 --- a/tests/e2e/testnet-scripts/sovereign-genesis.json +++ b/tests/e2e/testnet-scripts/sovereign-genesis.json @@ -292,4 +292,4 @@ "upgrade": {}, "vesting": {} } -} +} \ No newline at end of file diff --git a/tests/e2e/testnet-scripts/start-chain.sh b/tests/e2e/testnet-scripts/start-chain.sh index e1e7a5c95a..e3e6b6054f 100644 --- a/tests/e2e/testnet-scripts/start-chain.sh +++ b/tests/e2e/testnet-scripts/start-chain.sh @@ -198,7 +198,6 @@ do #'s/foo/bar/;s/abc/def/' sed -i "$TENDERMINT_CONFIG_TRANSFORM" $CHAIN_ID/validator$VAL_ID/config/config.toml fi - done diff --git a/third_party/proto/tendermint/types/evidence.proto b/third_party/proto/tendermint/types/evidence.proto deleted file mode 100644 index 451b8dca3c..0000000000 --- a/third_party/proto/tendermint/types/evidence.proto +++ /dev/null @@ -1,38 +0,0 @@ -syntax = "proto3"; -package tendermint.types; - -option go_package = "github.com/tendermint/tendermint/proto/tendermint/types"; - -import "gogoproto/gogo.proto"; -import "google/protobuf/timestamp.proto"; -import "tendermint/types/types.proto"; -import "tendermint/types/validator.proto"; - -message Evidence { - oneof sum { - DuplicateVoteEvidence duplicate_vote_evidence = 1; - LightClientAttackEvidence light_client_attack_evidence = 2; - } -} - -// DuplicateVoteEvidence contains evidence of a validator signed two conflicting votes. -message DuplicateVoteEvidence { - tendermint.types.Vote vote_a = 1; - tendermint.types.Vote vote_b = 2; - int64 total_voting_power = 3; - int64 validator_power = 4; - google.protobuf.Timestamp timestamp = 5 [(gogoproto.nullable) = false, (gogoproto.stdtime) = true]; -} - -// LightClientAttackEvidence contains evidence of a set of validators attempting to mislead a light client. -message LightClientAttackEvidence { - tendermint.types.LightBlock conflicting_block = 1; - int64 common_height = 2; - repeated tendermint.types.Validator byzantine_validators = 3; - int64 total_voting_power = 4; - google.protobuf.Timestamp timestamp = 5 [(gogoproto.nullable) = false, (gogoproto.stdtime) = true]; -} - -message EvidenceList { - repeated Evidence evidence = 1 [(gogoproto.nullable) = false]; -}