fix!: prevent denom DOS #931

shaspitz · 2023-05-08T21:36:11Z

Description

Credit to @jtremback for most of this code

This PR ports the denom fixes in https://github.com/cosmos/interchain-security/releases/tag/v1.2.0-multiden and https://github.com/cosmos/interchain-security/releases/tag/v1.1.0-multiden on top of main. Note this includes the fixes for both consumer and provider.

ADR: #934

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage

* squash commit of multisig fix + everything involving denom fix * rebuild proto * fix todos --------- Co-authored-by: Jehan Tremback <[email protected]>

faddat · 2023-05-09T18:18:18Z

does 1.2.0-multiden need to be updated?

should we perhaps release a v1.3.0?

Make a v1.3.0 release of ics #933

faddat · 2023-05-09T18:26:17Z

@smarshall-spitzbart it's at minimum something with package names, I have this checked out too. Look at steps_reward_denom.go

should that file be in that folder?

Is this code deployed anywhere?

shaspitz · 2023-05-09T18:33:32Z

@faddat this PR is a WIP at this point, I'll make it non draft when it's ready to review. 1.2.0-multiden does not need updating from my perspective, this PR will eventually port the fixes in 1.2.0-multiden to main

faddat · 2023-05-09T19:25:31Z

@smarshall-spitzbart fyi, the issue was my git checkout have a nice day <3

tests/e2e/steps_democracy.go

MSalopek

Approval.

I'm doing some additional checks around GetEstimatedNextFeeDistribution function on the consumer side. New tracking issues will be opened if the function behaviour has changed.

mpoke

Nice work @smarshall-spitzbart.

x/ccv/provider/keeper/distribution.go

shaspitz · 2023-05-15T21:22:32Z

x/ccv/provider/keeper/proposal_test.go

@@ -985,8 +989,9 @@ func TestBeginBlockCCR(t *testing.T) {
 	// Only first two consumer chains should be stopped
 	expectations = append(expectations, testkeeper.GetMocksForStopConsumerChain(ctx, &mocks)...)
 	expectations = append(expectations, testkeeper.GetMocksForStopConsumerChain(ctx, &mocks)...)
+	expectations = append(expectations, mocks.MockStakingKeeper.EXPECT().BondDenom(ctx).Return("stake").AnyTimes())


missed removing this

done df1fcbd

jtremback

LGTM

@MSalopek

* build(deps): bump gaurav-nelson/github-action-markdown-link-check from 1.0.13 to 1.0.15 (#928) build(deps): bump gaurav-nelson/github-action-markdown-link-check Bumps [gaurav-nelson/github-action-markdown-link-check](https://github.com/gaurav-nelson/github-action-markdown-link-check) from 1.0.13 to 1.0.15. - [Release notes](https://github.com/gaurav-nelson/github-action-markdown-link-check/releases) - [Commits](gaurav-nelson/github-action-markdown-link-check@1.0.13...1.0.15) --- updated-dependencies: - dependency-name: gaurav-nelson/github-action-markdown-link-check dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore: bump hermes (#921) * bump the version of hermes used in docs and images * use the multiplatform ghcr.io build of hermes * build(deps): bump github.com/spf13/cast from 1.5.0 to 1.5.1 (#961) Bumps [github.com/spf13/cast](https://github.com/spf13/cast) from 1.5.0 to 1.5.1. - [Release notes](https://github.com/spf13/cast/releases) - [Commits](spf13/cast@v1.5.0...v1.5.1) --- updated-dependencies: - dependency-name: github.com/spf13/cast dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * refactor: adopt the errors module to reduce the changeset for 47 (#920) adopt the errors module to reduce the changeset for 47 Co-authored-by: Shawn <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Marius Poke <[email protected]> * fix: all feature branches should have CI (#958) * Update automated-tests.yml * Update build.yml * all feature branches will now run all ci jobs relevant to them --------- Co-authored-by: Shawn <[email protected]> * fix!: consumer key prefix order to avoid complex migrations (#963) proper order matching v1.0.0 Co-authored-by: Marius Poke <[email protected]> * docs: update changelog to prep for v1.3.0 release (#953) * wip * Update CHANGELOG.md * small comment * comment * progress save * another progress save * progress save * done * Update CHANGELOG.md * add denom dos entry * remove extraneous changelog entries * restore a couple entries * Changes from PR review * add entry for 963 * fix: mitigate e2e tests relaying and non-determinism (#968) * fix: mitigate e2e tests relaying non-determinism * fix: bump signed blocks windows in e2e test configs * deps: bump cometbft to v0.34.28 (#906) this bumps only cometbft Co-authored-by: MSalopek <[email protected]> * fix!: Remove panics on failure to send IBC packets (#876) * provider: replace panic with StopConsumerChain * provider: replace panic with error message * Info logging on client expiration * add test for consumer * add test for provider * linter * Update CHANGELOG.md --------- Co-authored-by: Shawn <[email protected]> * build(deps): bump github.com/stretchr/testify from 1.8.2 to 1.8.3 (#969) Bumps [github.com/stretchr/testify](https://github.com/stretchr/testify) from 1.8.2 to 1.8.3. - [Release notes](https://github.com/stretchr/testify/releases) - [Commits](stretchr/testify@v1.8.2...v1.8.3) --- updated-dependencies: - dependency-name: github.com/stretchr/testify dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Shawn <[email protected]> * build(deps): bump slackapi/slack-github-action from 1.23.0 to 1.24.0 (#971) Bumps [slackapi/slack-github-action](https://github.com/slackapi/slack-github-action) from 1.23.0 to 1.24.0. - [Release notes](https://github.com/slackapi/slack-github-action/releases) - [Commits](slackapi/slack-github-action@v1.23.0...v1.24.0) --- updated-dependencies: - dependency-name: slackapi/slack-github-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * refactor!: upgrade ICS imports to v2 (#974) * v2 imports * Update CHANGELOG.md * docs: update PR template to consider migrations (#976) Update PULL_REQUEST_TEMPLATE.md * fix: v2 imports proto go_package option (#978) * add v2 to proto files, adjust protocgen scripts * regen proto * fix: partially revert key assignment type safety PR (#980) * use bytes in place where possible * fix tests * add v2 to proto files, adjust protocgen scripts * regen proto * change protos, define custom types, fix references * Update key_assignment_test.go * Update key_assignment.go * format * Update CHANGELOG.md * nit for better diff * docs: update top level readme for repo (#981) * Update base.css * Update README.md * smol --------- Co-authored-by: Marius Poke <[email protected]> * ci: makefile target for checking if protos are updated (#979) * proto-check makefile target * comment * add to GH actions workflow * put proto check before other tests * gotta regenerate protos --------- Co-authored-by: Marius Poke <[email protected]> * build(deps): bump github.com/cosmos/ibc-go/v4 from 4.4.0 to 4.4.2 (#982) * build(deps): bump github.com/cosmos/ibc-go/v4 from 4.4.0 to 4.4.2 Bumps [github.com/cosmos/ibc-go/v4](https://github.com/cosmos/ibc-go) from 4.4.0 to 4.4.2. - [Release notes](https://github.com/cosmos/ibc-go/releases) - [Changelog](https://github.com/cosmos/ibc-go/blob/main/CHANGELOG.md) - [Commits](cosmos/ibc-go@v4.4.0...v4.4.2) --- updated-dependencies: - dependency-name: github.com/cosmos/ibc-go/v4 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> * update changelog --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: mpoke <[email protected]> Co-authored-by: Shawn <[email protected]> * build(deps): bump JamesIves/github-pages-deploy-action from 4.4.1 to 4.4.2 (#983) build(deps): bump JamesIves/github-pages-deploy-action Bumps [JamesIves/github-pages-deploy-action](https://github.com/JamesIves/github-pages-deploy-action) from 4.4.1 to 4.4.2. - [Release notes](https://github.com/JamesIves/github-pages-deploy-action/releases) - [Commits](JamesIves/github-pages-deploy-action@v4.4.1...v4.4.2) --- updated-dependencies: - dependency-name: JamesIves/github-pages-deploy-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Shawn <[email protected]> * build(deps): bump github.com/stretchr/testify from 1.8.3 to 1.8.4 (#985) Bumps [github.com/stretchr/testify](https://github.com/stretchr/testify) from 1.8.3 to 1.8.4. - [Release notes](https://github.com/stretchr/testify/releases) - [Commits](stretchr/testify@v1.8.3...v1.8.4) --- updated-dependencies: - dependency-name: github.com/stretchr/testify dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Marius Poke <[email protected]> * feat: v2 migrations (#975) * v2 imports * Squashed commit of the following: commit a4c9224 Author: Shawn <[email protected]> Date: Wed May 24 10:13:10 2023 -0700 Revert "Merge branch 'shawn/v2-imports' into shawn/ccv-migrations" This reverts commit 53e3362, reversing changes made to 9c3f338. commit 6885ad1 Author: Shawn <[email protected]> Date: Wed May 24 10:12:49 2023 -0700 Revert "Merge branch 'shawn/v2-imports' into shawn/ccv-migrations" This reverts commit 45d74c7, reversing changes made to 53e3362. commit 9589144 Author: Shawn <[email protected]> Date: Tue May 23 14:48:06 2023 -0700 provider migration boilerplate commit 9521ecb Author: Shawn <[email protected]> Date: Tue May 23 12:25:14 2023 -0700 lint commit fc3f273 Author: Shawn <[email protected]> Date: Tue May 23 12:20:33 2023 -0700 old default params commit 80a490c Author: Shawn <[email protected]> Date: Tue May 23 12:15:30 2023 -0700 naming commit 45d74c7 Merge: 53e3362 8e6bdfb Author: Shawn <[email protected]> Date: Tue May 23 12:12:03 2023 -0700 Merge branch 'shawn/v2-imports' into shawn/ccv-migrations commit 8e6bdfb Author: Shawn <[email protected]> Date: Tue May 23 12:10:22 2023 -0700 proto name for gov prop registration commit 53e3362 Merge: 9c3f338 5ca68d1 Author: Shawn <[email protected]> Date: Tue May 23 12:05:39 2023 -0700 Merge branch 'shawn/v2-imports' into shawn/ccv-migrations commit 5ca68d1 Author: Shawn <[email protected]> Date: Tue May 23 11:53:12 2023 -0700 fix e2e tests commit aa6bd0c Author: Shawn <[email protected]> Date: Tue May 23 11:42:47 2023 -0700 rm bad files commit 6e3dc88 Author: Shawn <[email protected]> Date: Tue May 23 11:42:14 2023 -0700 correct generation commit 056ef7a Author: Shawn <[email protected]> Date: Tue May 23 11:29:45 2023 -0700 proto upgrade too commit 9c3f338 Author: Shawn <[email protected]> Date: Tue May 23 10:57:25 2023 -0700 remove hardcoded old code commit 1e73173 Merge: dbf9ded 8769fd5 Author: Shawn <[email protected]> Date: Tue May 23 10:07:31 2023 -0700 Merge branch 'shawn/v2-imports' into shawn/ccv-migrations commit 8769fd5 Author: Shawn <[email protected]> Date: Tue May 23 09:58:28 2023 -0700 v2 imports commit dbf9ded Author: Shawn <[email protected]> Date: Mon May 22 16:10:05 2023 -0700 provider migration commit 2d95e2e Author: Shawn <[email protected]> Date: Mon May 22 15:01:47 2023 -0700 improve consumer test commit 85f4cfd Author: Shawn <[email protected]> Date: Mon May 22 14:03:20 2023 -0700 consumer params * rm old code * go.mod restore * better naming of hardcodes * consumer boilerplate * comments * migrate consumer genesis states * test and cleans * lint * migration and partial test * cleans * finish test * comments and doc * Update migration_test.go * Update CHANGELOG.md * expand in changelog * increment consensus ver * set key table on construction * rm semver migration funcs * comment explaining consensus version * docs: cleanup changelog for v2.0.0 on main (#988) cleans * chore: Hardcode golangci-lint version (#990) * Hardcode golangci-lint version * Hardcode version in CI config * docs: Increase the validator set of cosmos hub to 180 from 175 (#999) Updated number of validators to 180 * fix: proper consumer key prefix ordering (#991) * Update keys.go * tests * fix another bug * fix comments * feat: Remove consumer genesis migration on provider (#997) * Update keys.go * tests * fix another bug * remove consumer genesis deletion, link to test * remove unused bond denom method * Revert "remove unused bond denom method" This reverts commit f930eca. * remove test too * update changelog * docs: Update reward-distribution.md (#994) * Update reward-distribution.md * docs: add instructions for registering denoms * Update docs/docs/features/reward-distribution.md Co-authored-by: Marius Poke <[email protected]> * Update reward-distribution.md * Update docs/docs/features/reward-distribution.md Co-authored-by: Shawn <[email protected]> --------- Co-authored-by: MSalopek <[email protected]> Co-authored-by: Marius Poke <[email protected]> * chore: update workflow re. issues and PRs (#1002) * update PR workflow * update issue workflow * rename other.md to others.md * fix typo --------- Co-authored-by: Shawn <[email protected]> * docs(adr): ADR-007 pause unbonding period during equivocation proposal (#964) * docs(adr): pause unbonding period during equivocation proposal Co-authored-by: Albert Le Batteux <[email protected]> Co-authored-by: Giuseppe Natale <[email protected]> * fix voting period duration * remove issue reference * docs: filter out unbonding operations before pause/unpause Co-authored-by: Albert Le Batteux <[email protected]> Co-authored-by: Giuseppe Natale <[email protected]> --------- Co-authored-by: Albert Le Batteux <[email protected]> Co-authored-by: Giuseppe Natale <[email protected]> * docs: Add type prefix link to CONTRIBUTING.md (#1007) Update CONTRIBUTING.md * chore: enable mergify (#1009) * add config for mergify * enable security dependecies for v2.0.x * Markdownlint (#907) markdownlint Co-authored-by: Jacob Gadikian <[email protected]> * fix: limit vsc matured packets handled per endblocker (#1004) * initial implementation, still need tests * UTs * integration test * linter * Update CHANGELOG.md * make vsc matured handled this block a var * comment * feat: integrate cometmock (#989) * Add gorelayer and CometMock to Dockerfile * Add option to start with cometmock in start-chain script * Start adding support for rly * Adjust relayer start action * Add entrypoint for short happy path steps * Add . nosec G204 and waiting for blocks * Adjust rly config: Gas is free * Remove optout steps from short happy path * Use separate redelegate step for short happy path * Wait for blocks after unbonding * Make naming more descriptive and add comments * Add comment to chain name sorting and improve comments * Update start-chain.sh Address comments form joint review session with @MSalopek * Fix typo * docs: Create adr-004-denom-dos-fixes.md (#934) * Create adr-006-denom-dos-fixes * Update docs/docs/adrs/adr-006-denom-dos-fixes Co-authored-by: Shawn <[email protected]> * Update docs/docs/adrs/adr-006-denom-dos-fixes Co-authored-by: Shawn <[email protected]> * Update docs/docs/adrs/adr-006-denom-dos-fixes Co-authored-by: Marius Poke <[email protected]> * Update docs/docs/adrs/adr-006-denom-dos-fixes * Update docs/docs/adrs/adr-006-denom-dos-fixes * rename to adr 004 * remove extra file * add entry to Table of Contents * add ADR 7 to ToC --------- Co-authored-by: Shawn <[email protected]> Co-authored-by: Marius Poke <[email protected]> * docs: Fix link to template (#1027) Fix link to template Fixes typo in contributing.md * feat!: Add DistributionTransmissionChannel to ConsumerAdditionProposal (#965) * update proto * remove transfer_channel_id from consumer genesis * ConsumerAdditionProposal: transfer_channel_id -> distribution_transmission_channel * send updated ConsumerAdditionProposal * validate consumer genesis param * remove StandaloneTransferChannelID from store * fix TestOnChanOpenAck * remove state breaking change * finalize merge and fix issues * chore: update docs and changelog * chore: regenerate protos * re-add integrationt tests around changeover * mv entry in changelog * test: add sovereign to consumer changeover e2e (#1025) * tests: add sovereign to consumer e2e test * rm unused bash scripts * partially address review comments * apply remaining review comments * chore: apply formatting rules --------- Co-authored-by: MSalopek <[email protected]> * docs: ADR for throttle with retries (#1005) * all of ADR is filled out except design portion * design * Update adr-008-throttle-retries.md * Update adr-008-throttle-retries.md * Update adr-008-throttle-retries.md * Apply suggestions from code review Co-authored-by: Marius Poke <[email protected]> * nit formatting * describe consumer changes first * add comment on rareness of throttling being triggered * split out paragraph * hopefully better explanation * Update adr-008-throttle-retries.md * accepted * TOC entry --------- Co-authored-by: Marius Poke <[email protected]> * Add time and block advancement integration for CometMock (#1017) * Add time and block advancement * Adhere to gocritic: use += * Remove extra debug output * Fix: use correct key when consumer key is not assigned * Correct private key address field * Clarify comment for WaitTime * Use bool instead of *bool type * Add review comments * refactor: first batch of post-merge changes * refactor: batch sovereign changes with v47 * refactor: another batch of post-merge changes * changes to go.mod * refactor: final batch of changes post-merge * refactor: rebuild protos for v47 * refactor: rebuild mocks for v47 * refactor: testing changes * refactor: update proto tooling and rebuild protos * lint: appease gosec * chore: rm unused string from Makefile * chore: rm unused in makefile .phony * temporarily disable proto-check to run automated tests --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Jacob Gadikian <[email protected]> Co-authored-by: Shawn <[email protected]> Co-authored-by: Jehan Tremback <[email protected]> Co-authored-by: Marius Poke <[email protected]> Co-authored-by: Philip Offtermatt <[email protected]> Co-authored-by: Milan Mulji <[email protected]> Co-authored-by: Thomas Bruyelle <[email protected]> Co-authored-by: Albert Le Batteux <[email protected]> Co-authored-by: Giuseppe Natale <[email protected]> Co-authored-by: Ruslan Akhtariev <[email protected]> Co-authored-by: Jehan <[email protected]>

shaspitz and others added 2 commits May 8, 2023 14:22

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

ccc6336

* squash commit of multisig fix + everything involving denom fix * rebuild proto * fix todos --------- Co-authored-by: Jehan Tremback <[email protected]>

regen proto

cf8f2fb

shaspitz added 3 commits May 9, 2023 11:56

fix cherrypick issues

dba2116

lint

86a0417

cleans

20c7776

gosec

6914ee7

shaspitz changed the title ~~fix: denoms~~ fix: prevent denom DOS May 10, 2023

restore param, remove tech debt from tests

a43c0d3

shaspitz changed the title ~~fix: prevent denom DOS~~ fix!: prevent denom DOS May 10, 2023

shaspitz marked this pull request as ready for review May 10, 2023 18:05

shaspitz requested a review from a team as a code owner May 10, 2023 18:05

Merge branch 'main' into shawn/multiden-to-main

ab5201f

shaspitz mentioned this pull request May 11, 2023

EPIC: v2.0.0 release #949

Closed

MSalopek reviewed May 12, 2023

View reviewed changes

tests/e2e/steps_democracy.go Outdated Show resolved Hide resolved

MSalopek approved these changes May 12, 2023

View reviewed changes

mpoke approved these changes May 12, 2023

View reviewed changes

x/ccv/provider/keeper/distribution.go Show resolved Hide resolved

x/ccv/provider/keeper/distribution.go Show resolved Hide resolved

shaspitz and others added 4 commits May 12, 2023 10:16

ibc denom as const

a4bc96e

add check for consumer reward denom already registered

f710f21

lint

1264a31

Merge branch 'main' into shawn/multiden-to-main

ae80ed8

shaspitz commented May 15, 2023

View reviewed changes

jtremback approved these changes May 15, 2023

View reviewed changes

shaspitz added 2 commits May 15, 2023 14:51

remove unneeded expect

df1fcbd

Merge branch 'main' into shawn/multiden-to-main

e3c474b

shaspitz merged commit da06d17 into main May 15, 2023

shaspitz deleted the shawn/multiden-to-main branch May 15, 2023 23:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix!: prevent denom DOS #931

fix!: prevent denom DOS #931

shaspitz commented May 8, 2023 •

edited

Loading

faddat commented May 9, 2023 •

edited

Loading

faddat commented May 9, 2023 •

edited

Loading

shaspitz commented May 9, 2023 •

edited

Loading

faddat commented May 9, 2023

MSalopek left a comment •

edited

Loading

mpoke left a comment

shaspitz May 15, 2023

shaspitz May 15, 2023

jtremback left a comment

fix!: prevent denom DOS #931

fix!: prevent denom DOS #931

Conversation

shaspitz commented May 8, 2023 • edited Loading

Description

Author Checklist

Reviewers Checklist

faddat commented May 9, 2023 • edited Loading

faddat commented May 9, 2023 • edited Loading

shaspitz commented May 9, 2023 • edited Loading

faddat commented May 9, 2023

MSalopek left a comment • edited Loading

Choose a reason for hiding this comment

mpoke left a comment

Choose a reason for hiding this comment

shaspitz May 15, 2023

Choose a reason for hiding this comment

shaspitz May 15, 2023

Choose a reason for hiding this comment

jtremback left a comment

Choose a reason for hiding this comment

shaspitz commented May 8, 2023 •

edited

Loading

faddat commented May 9, 2023 •

edited

Loading

faddat commented May 9, 2023 •

edited

Loading

shaspitz commented May 9, 2023 •

edited

Loading

MSalopek left a comment •

edited

Loading