From 48457aa8bf900613085fcbd728488013bf194a05 Mon Sep 17 00:00:00 2001 From: Jehan Date: Tue, 9 May 2023 16:04:20 -0700 Subject: [PATCH 01/10] Create adr-006-denom-dos-fixes --- docs/docs/adrs/adr-006-denom-dos-fixes | 53 ++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 docs/docs/adrs/adr-006-denom-dos-fixes diff --git a/docs/docs/adrs/adr-006-denom-dos-fixes b/docs/docs/adrs/adr-006-denom-dos-fixes new file mode 100644 index 0000000000..5fbe0cedd0 --- /dev/null +++ b/docs/docs/adrs/adr-006-denom-dos-fixes @@ -0,0 +1,53 @@ +--- +sidebar_position: 2 +title: ADR Template +--- +# ADR 006: Denom DOS fixes + +## Changelog +* 5/9/2023: ADR created + +## Status + +Accepted + +## Context + +The provider and consumer modules are vulnerable to similar issues involving an attacker sending millions of denoms to certain addresses and causing the chain to halt. This ADR outlines both fixes since they are similar. Both fixes involve processing only denoms that are on a whitelist to avoid iterating over millions of junk denoms but have different requirements and are implemented in different ways. + +## Decision + +### Provider + +- Put the distribution module's FeePoolAddress back on the blocklist so that it cannot receive funds from users. +- Create a new address called ConsumerRewardPool and unblock it, allowing funds to be sent to it. +- Create a set of strings in the database for ConsumerRewardDenoms. +- Create an endpoint called RegisterConsumerRewardDenom which deducts a fee from the sender's account, sends it to the community pool and adds a string to the ConsumerRewardDenoms set. +- Create a parameter called ConsumerRewardDenomRegistrationFee which determines the fee that is +- Create a function called TransferRewardsToFeeCollector which gets the entire ConsumerRewardDenoms set from the database, iterates over it, and for each entry: + - Gets the balance of this denom for the ConsumerRewardPool account + - Sends the entire balance out to the FeePoolAddress using SendCoinsFromModuleToModule which is not affected by the blocklist. +- Run TransferRewardsToFeeCollector in the endblock + +Now, nobody can send millions of junk denoms to the FeePoolAddress because it is on the block list. If they send millions of junk denoms to the ConsumerRewardPool, this does not matter because all balances are not iterated over, only those which are in the ConsumerRewardDenoms set. + +We also add a new tx: register-consumer-reward-denom, and a new query: registered-consumer-reward-denoms + +### Consumer + +- Create a new param RewardDenoms with a list of strings +- Create a new param ProviderRewardDenoms with a list of strings +- Create a function AllowedRewardDenoms which maps over ProviderRewardDenoms and converts each denom to its ibc-prefixed denom using the provider chain's ibc channel information, then concatenates the RewardDenoms list and returns the combined list of allowed denoms. +- In SendRewardsToProvider, instead of iterating over the balances of all denoms in the ToSendToProvider address, iterate over AllowedRewardDenoms + +Now, if somebody sends millions of junk denoms to ToSendToProvider, they will not be iterated over. Only the RewardDenoms and ProviderRewardDenoms will be iterated over. Since we do not require this feature to be permissionless on the consumer, we did not have to bother with the registration fee process. + +## Consequences + +### Positive + +- Denom DOS is no longer possible on either provider or consumer. + +### Negative + +- Consumer chain teams must pay a fee to register a denom for distribution on the provider, and add some extra parameters in their genesis file. From ce986508630ab1fe2102409fa09567b3ca98a5cb Mon Sep 17 00:00:00 2001 From: Jehan Date: Tue, 16 May 2023 16:26:54 -0700 Subject: [PATCH 02/10] Update docs/docs/adrs/adr-006-denom-dos-fixes Co-authored-by: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> --- docs/docs/adrs/adr-006-denom-dos-fixes | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/docs/adrs/adr-006-denom-dos-fixes b/docs/docs/adrs/adr-006-denom-dos-fixes index 5fbe0cedd0..c736659e79 100644 --- a/docs/docs/adrs/adr-006-denom-dos-fixes +++ b/docs/docs/adrs/adr-006-denom-dos-fixes @@ -21,7 +21,7 @@ The provider and consumer modules are vulnerable to similar issues involving an - Put the distribution module's FeePoolAddress back on the blocklist so that it cannot receive funds from users. - Create a new address called ConsumerRewardPool and unblock it, allowing funds to be sent to it. -- Create a set of strings in the database for ConsumerRewardDenoms. +- Create a set of strings in the store for allowed ConsumerRewardDenoms. - Create an endpoint called RegisterConsumerRewardDenom which deducts a fee from the sender's account, sends it to the community pool and adds a string to the ConsumerRewardDenoms set. - Create a parameter called ConsumerRewardDenomRegistrationFee which determines the fee that is - Create a function called TransferRewardsToFeeCollector which gets the entire ConsumerRewardDenoms set from the database, iterates over it, and for each entry: From 3fc1f6d1f9b35b6e9b0304badf147f3451ea857e Mon Sep 17 00:00:00 2001 From: Jehan Date: Tue, 16 May 2023 16:27:02 -0700 Subject: [PATCH 03/10] Update docs/docs/adrs/adr-006-denom-dos-fixes Co-authored-by: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> --- docs/docs/adrs/adr-006-denom-dos-fixes | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/docs/adrs/adr-006-denom-dos-fixes b/docs/docs/adrs/adr-006-denom-dos-fixes index c736659e79..fb6e453eb5 100644 --- a/docs/docs/adrs/adr-006-denom-dos-fixes +++ b/docs/docs/adrs/adr-006-denom-dos-fixes @@ -24,7 +24,7 @@ The provider and consumer modules are vulnerable to similar issues involving an - Create a set of strings in the store for allowed ConsumerRewardDenoms. - Create an endpoint called RegisterConsumerRewardDenom which deducts a fee from the sender's account, sends it to the community pool and adds a string to the ConsumerRewardDenoms set. - Create a parameter called ConsumerRewardDenomRegistrationFee which determines the fee that is -- Create a function called TransferRewardsToFeeCollector which gets the entire ConsumerRewardDenoms set from the database, iterates over it, and for each entry: +- Create a function called TransferRewardsToFeeCollector which gets the entire ConsumerRewardDenoms set from the store, iterates over it, and for each entry: - Gets the balance of this denom for the ConsumerRewardPool account - Sends the entire balance out to the FeePoolAddress using SendCoinsFromModuleToModule which is not affected by the blocklist. - Run TransferRewardsToFeeCollector in the endblock From 21d3d44f9f67bc821027e61469be6c3948f25212 Mon Sep 17 00:00:00 2001 From: Jehan Date: Tue, 16 May 2023 16:28:25 -0700 Subject: [PATCH 04/10] Update docs/docs/adrs/adr-006-denom-dos-fixes Co-authored-by: Marius Poke --- docs/docs/adrs/adr-006-denom-dos-fixes | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/docs/adrs/adr-006-denom-dos-fixes b/docs/docs/adrs/adr-006-denom-dos-fixes index fb6e453eb5..012d405359 100644 --- a/docs/docs/adrs/adr-006-denom-dos-fixes +++ b/docs/docs/adrs/adr-006-denom-dos-fixes @@ -40,7 +40,7 @@ We also add a new tx: register-consumer-reward-denom, and a new query: registere - Create a function AllowedRewardDenoms which maps over ProviderRewardDenoms and converts each denom to its ibc-prefixed denom using the provider chain's ibc channel information, then concatenates the RewardDenoms list and returns the combined list of allowed denoms. - In SendRewardsToProvider, instead of iterating over the balances of all denoms in the ToSendToProvider address, iterate over AllowedRewardDenoms -Now, if somebody sends millions of junk denoms to ToSendToProvider, they will not be iterated over. Only the RewardDenoms and ProviderRewardDenoms will be iterated over. Since we do not require this feature to be permissionless on the consumer, we did not have to bother with the registration fee process. +Now, if somebody sends millions of junk denoms to ToSendToProvider, they will not be iterated over. Only the RewardDenoms and ProviderRewardDenoms will be iterated over. Since we do not require this feature to be permissionless on the consumer, the registration fee process is not needed. ## Consequences From 0c6272430a116a609d62b77c2d1a5363a664d53f Mon Sep 17 00:00:00 2001 From: Jehan Date: Tue, 16 May 2023 16:28:56 -0700 Subject: [PATCH 05/10] Update docs/docs/adrs/adr-006-denom-dos-fixes --- docs/docs/adrs/adr-006-denom-dos-fixes | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/docs/adrs/adr-006-denom-dos-fixes b/docs/docs/adrs/adr-006-denom-dos-fixes index 012d405359..f6ec7f1236 100644 --- a/docs/docs/adrs/adr-006-denom-dos-fixes +++ b/docs/docs/adrs/adr-006-denom-dos-fixes @@ -37,7 +37,7 @@ We also add a new tx: register-consumer-reward-denom, and a new query: registere - Create a new param RewardDenoms with a list of strings - Create a new param ProviderRewardDenoms with a list of strings -- Create a function AllowedRewardDenoms which maps over ProviderRewardDenoms and converts each denom to its ibc-prefixed denom using the provider chain's ibc channel information, then concatenates the RewardDenoms list and returns the combined list of allowed denoms. +- Create a function AllowedRewardDenoms which iterates over ProviderRewardDenoms and converts each denom to its ibc-prefixed denom using the provider chain's ibc channel information, then concatenates the RewardDenoms list and returns the combined list of allowed denoms. - In SendRewardsToProvider, instead of iterating over the balances of all denoms in the ToSendToProvider address, iterate over AllowedRewardDenoms Now, if somebody sends millions of junk denoms to ToSendToProvider, they will not be iterated over. Only the RewardDenoms and ProviderRewardDenoms will be iterated over. Since we do not require this feature to be permissionless on the consumer, the registration fee process is not needed. From 4670fcfa2db12fc690135c1b78e814c8523b1002 Mon Sep 17 00:00:00 2001 From: Jehan Date: Tue, 13 Jun 2023 09:25:07 -0700 Subject: [PATCH 06/10] Update docs/docs/adrs/adr-006-denom-dos-fixes --- docs/docs/adrs/adr-006-denom-dos-fixes | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/docs/adrs/adr-006-denom-dos-fixes b/docs/docs/adrs/adr-006-denom-dos-fixes index f6ec7f1236..cc60f2b7f8 100644 --- a/docs/docs/adrs/adr-006-denom-dos-fixes +++ b/docs/docs/adrs/adr-006-denom-dos-fixes @@ -23,7 +23,7 @@ The provider and consumer modules are vulnerable to similar issues involving an - Create a new address called ConsumerRewardPool and unblock it, allowing funds to be sent to it. - Create a set of strings in the store for allowed ConsumerRewardDenoms. - Create an endpoint called RegisterConsumerRewardDenom which deducts a fee from the sender's account, sends it to the community pool and adds a string to the ConsumerRewardDenoms set. -- Create a parameter called ConsumerRewardDenomRegistrationFee which determines the fee that is +- Create a parameter called ConsumerRewardDenomRegistrationFee which determines the fee which is charged to register a consumer reward denom in the step above. - Create a function called TransferRewardsToFeeCollector which gets the entire ConsumerRewardDenoms set from the store, iterates over it, and for each entry: - Gets the balance of this denom for the ConsumerRewardPool account - Sends the entire balance out to the FeePoolAddress using SendCoinsFromModuleToModule which is not affected by the blocklist. From d068c407dd401df3323c3aaf58414d8af43db0dd Mon Sep 17 00:00:00 2001 From: Jehan Tremback Date: Tue, 13 Jun 2023 09:33:36 -0700 Subject: [PATCH 07/10] rename to adr 004 --- docs/docs/adrs/adr-004-denom-dos-fixes | 53 ++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 docs/docs/adrs/adr-004-denom-dos-fixes diff --git a/docs/docs/adrs/adr-004-denom-dos-fixes b/docs/docs/adrs/adr-004-denom-dos-fixes new file mode 100644 index 0000000000..7ca260f4f6 --- /dev/null +++ b/docs/docs/adrs/adr-004-denom-dos-fixes @@ -0,0 +1,53 @@ +--- +sidebar_position: 2 +title: ADR Template +--- +# ADR 004: Denom DOS fixes + +## Changelog +* 5/9/2023: ADR created + +## Status + +Accepted + +## Context + +The provider and consumer modules are vulnerable to similar issues involving an attacker sending millions of denoms to certain addresses and causing the chain to halt. This ADR outlines both fixes since they are similar. Both fixes involve processing only denoms that are on a whitelist to avoid iterating over millions of junk denoms but have different requirements and are implemented in different ways. + +## Decision + +### Provider + +- Put the distribution module's FeePoolAddress back on the blocklist so that it cannot receive funds from users. +- Create a new address called ConsumerRewardPool and unblock it, allowing funds to be sent to it. +- Create a set of strings in the store for allowed ConsumerRewardDenoms. +- Create an endpoint called RegisterConsumerRewardDenom which deducts a fee from the sender's account, sends it to the community pool and adds a string to the ConsumerRewardDenoms set. +- Create a parameter called ConsumerRewardDenomRegistrationFee which determines the fee which is charged to register a consumer reward denom in the step above. +- Create a function called TransferRewardsToFeeCollector which gets the entire ConsumerRewardDenoms set from the store, iterates over it, and for each entry: + - Gets the balance of this denom for the ConsumerRewardPool account + - Sends the entire balance out to the FeePoolAddress using SendCoinsFromModuleToModule which is not affected by the blocklist. +- Run TransferRewardsToFeeCollector in the endblock + +Now, nobody can send millions of junk denoms to the FeePoolAddress because it is on the block list. If they send millions of junk denoms to the ConsumerRewardPool, this does not matter because all balances are not iterated over, only those which are in the ConsumerRewardDenoms set. + +We also add a new tx: register-consumer-reward-denom, and a new query: registered-consumer-reward-denoms + +### Consumer + +- Create a new param RewardDenoms with a list of strings +- Create a new param ProviderRewardDenoms with a list of strings +- Create a function AllowedRewardDenoms which iterates over ProviderRewardDenoms and converts each denom to its ibc-prefixed denom using the provider chain's ibc channel information, then concatenates the RewardDenoms list and returns the combined list of allowed denoms. +- In SendRewardsToProvider, instead of iterating over the balances of all denoms in the ToSendToProvider address, iterate over AllowedRewardDenoms + +Now, if somebody sends millions of junk denoms to ToSendToProvider, they will not be iterated over. Only the RewardDenoms and ProviderRewardDenoms will be iterated over. Since we do not require this feature to be permissionless on the consumer, the registration fee process is not needed. + +## Consequences + +### Positive + +- Denom DOS is no longer possible on either provider or consumer. + +### Negative + +- Consumer chain teams must pay a fee to register a denom for distribution on the provider, and add some extra parameters in their genesis file. From db917002bf5c159ab4fe36055f762a73af227cf3 Mon Sep 17 00:00:00 2001 From: mpoke Date: Thu, 15 Jun 2023 10:20:48 +0200 Subject: [PATCH 08/10] remove extra file --- docs/docs/adrs/adr-006-denom-dos-fixes | 53 -------------------------- 1 file changed, 53 deletions(-) delete mode 100644 docs/docs/adrs/adr-006-denom-dos-fixes diff --git a/docs/docs/adrs/adr-006-denom-dos-fixes b/docs/docs/adrs/adr-006-denom-dos-fixes deleted file mode 100644 index cc60f2b7f8..0000000000 --- a/docs/docs/adrs/adr-006-denom-dos-fixes +++ /dev/null @@ -1,53 +0,0 @@ ---- -sidebar_position: 2 -title: ADR Template ---- -# ADR 006: Denom DOS fixes - -## Changelog -* 5/9/2023: ADR created - -## Status - -Accepted - -## Context - -The provider and consumer modules are vulnerable to similar issues involving an attacker sending millions of denoms to certain addresses and causing the chain to halt. This ADR outlines both fixes since they are similar. Both fixes involve processing only denoms that are on a whitelist to avoid iterating over millions of junk denoms but have different requirements and are implemented in different ways. - -## Decision - -### Provider - -- Put the distribution module's FeePoolAddress back on the blocklist so that it cannot receive funds from users. -- Create a new address called ConsumerRewardPool and unblock it, allowing funds to be sent to it. -- Create a set of strings in the store for allowed ConsumerRewardDenoms. -- Create an endpoint called RegisterConsumerRewardDenom which deducts a fee from the sender's account, sends it to the community pool and adds a string to the ConsumerRewardDenoms set. -- Create a parameter called ConsumerRewardDenomRegistrationFee which determines the fee which is charged to register a consumer reward denom in the step above. -- Create a function called TransferRewardsToFeeCollector which gets the entire ConsumerRewardDenoms set from the store, iterates over it, and for each entry: - - Gets the balance of this denom for the ConsumerRewardPool account - - Sends the entire balance out to the FeePoolAddress using SendCoinsFromModuleToModule which is not affected by the blocklist. -- Run TransferRewardsToFeeCollector in the endblock - -Now, nobody can send millions of junk denoms to the FeePoolAddress because it is on the block list. If they send millions of junk denoms to the ConsumerRewardPool, this does not matter because all balances are not iterated over, only those which are in the ConsumerRewardDenoms set. - -We also add a new tx: register-consumer-reward-denom, and a new query: registered-consumer-reward-denoms - -### Consumer - -- Create a new param RewardDenoms with a list of strings -- Create a new param ProviderRewardDenoms with a list of strings -- Create a function AllowedRewardDenoms which iterates over ProviderRewardDenoms and converts each denom to its ibc-prefixed denom using the provider chain's ibc channel information, then concatenates the RewardDenoms list and returns the combined list of allowed denoms. -- In SendRewardsToProvider, instead of iterating over the balances of all denoms in the ToSendToProvider address, iterate over AllowedRewardDenoms - -Now, if somebody sends millions of junk denoms to ToSendToProvider, they will not be iterated over. Only the RewardDenoms and ProviderRewardDenoms will be iterated over. Since we do not require this feature to be permissionless on the consumer, the registration fee process is not needed. - -## Consequences - -### Positive - -- Denom DOS is no longer possible on either provider or consumer. - -### Negative - -- Consumer chain teams must pay a fee to register a denom for distribution on the provider, and add some extra parameters in their genesis file. From 9f36b3c3fa7daf978846eee1342bfb77768919c4 Mon Sep 17 00:00:00 2001 From: mpoke Date: Thu, 15 Jun 2023 10:21:14 +0200 Subject: [PATCH 09/10] add entry to Table of Contents --- docs/docs/adrs/intro.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/docs/adrs/intro.md b/docs/docs/adrs/intro.md index aa6b7781c9..47297b6f75 100644 --- a/docs/docs/adrs/intro.md +++ b/docs/docs/adrs/intro.md @@ -35,3 +35,4 @@ To suggest an ADR, please make use of the [ADR template](./adr-template.md) prov | [001](./adr-001-key-assignment.md) | Consumer chain key assignment | Accepted, Implemented | | [002](./adr-002-throttle.md) | Jail Throttling | Accepted, Implemented | | [003](./adr-003-equivocation-gov-proposal.md) | Equivocation governance proposal | Accepted, Implemented +| [004](./adr-004-denom-dos-fixes) | Denom DOS fixes | Accepted, Implemented \ No newline at end of file From 3ad6440e2c4c20830a3dc422b367689bd2d4d84d Mon Sep 17 00:00:00 2001 From: mpoke Date: Thu, 15 Jun 2023 10:22:39 +0200 Subject: [PATCH 10/10] add ADR 7 to ToC --- docs/docs/adrs/intro.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/docs/adrs/intro.md b/docs/docs/adrs/intro.md index 47297b6f75..528b4799ae 100644 --- a/docs/docs/adrs/intro.md +++ b/docs/docs/adrs/intro.md @@ -34,5 +34,6 @@ To suggest an ADR, please make use of the [ADR template](./adr-template.md) prov | ------ | ----------- | ------ | | [001](./adr-001-key-assignment.md) | Consumer chain key assignment | Accepted, Implemented | | [002](./adr-002-throttle.md) | Jail Throttling | Accepted, Implemented | -| [003](./adr-003-equivocation-gov-proposal.md) | Equivocation governance proposal | Accepted, Implemented -| [004](./adr-004-denom-dos-fixes) | Denom DOS fixes | Accepted, Implemented \ No newline at end of file +| [003](./adr-003-equivocation-gov-proposal.md) | Equivocation governance proposal | Accepted, Implemented | +| [004](./adr-004-denom-dos-fixes) | Denom DOS fixes | Accepted, Implemented | +| [007](./adr-007-pause-unbonding-on-eqv-prop.md) | Pause validator unbonding during equivocation proposal | Proposed | \ No newline at end of file