-
Notifications
You must be signed in to change notification settings - Fork 129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs: Create adr-004-denom-dos-fixes.md #934
Merged
Merged
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
48457aa
Create adr-006-denom-dos-fixes
jtremback ce98650
Update docs/docs/adrs/adr-006-denom-dos-fixes
jtremback 3fc1f6d
Update docs/docs/adrs/adr-006-denom-dos-fixes
jtremback 21d3d44
Update docs/docs/adrs/adr-006-denom-dos-fixes
jtremback 0c62724
Update docs/docs/adrs/adr-006-denom-dos-fixes
jtremback 14f2411
Merge branch 'main' into jtremback-patch-1
jtremback 4670fcf
Update docs/docs/adrs/adr-006-denom-dos-fixes
jtremback d068c40
rename to adr 004
jtremback db91700
remove extra file
mpoke 9f36b3c
add entry to Table of Contents
mpoke 780fee5
Merge branch 'main' into jtremback-patch-1
mpoke 3ad6440
add ADR 7 to ToC
mpoke File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
--- | ||
sidebar_position: 2 | ||
title: ADR Template | ||
--- | ||
# ADR 006: Denom DOS fixes | ||
mpoke marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
## 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 | ||
jtremback marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- 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 | ||
jtremback marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- 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. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this file.