Skip to content
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.

Sign up for GitHub

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

fix(x/distribution): vulnerable incrementReferenceCount in distribution #19301

Merged
merged 9 commits into from
Feb 7, 2024
Merged

fix(x/distribution): vulnerable incrementReferenceCount in distribution #19301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0580ff9
fix ref count increment
knkcse Jan 31, 2024
073cae1
Updated go.mod
knkcse Feb 1, 2024
7086c09
Updated go.mod
knkcse Feb 2, 2024
1147f97
Merge branch 'main' into naveen/ref_count
alexanderbez Feb 2, 2024
1c2ad0d
Merge branch 'main' into naveen/ref_count
likhita-809 Feb 5, 2024
74f8068
Updated changelog in x/distribution
knkcse Feb 6, 2024
6194378
Merge branch 'naveen/ref_count' of https://github.com/naveenkumarcse2…
knkcse Feb 6, 2024
c319021
Merge branch 'main' into naveen/ref_count
knkcse Feb 6, 2024
fb0b0d6
Merge branch 'main' into naveen/ref_count
likhita-809 Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions x/distribution/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
### Client Breaking Changes

* [#17657](https://github.com/cosmos/cosmos-sdk/pull/17657) Deprecate `CommunityPool` and `FundCommunityPool` rpc methods. Use `x/protocolpool` module's rpc methods instead.

### Bug Fixes

* [#19301](https://github.com/cosmos/cosmos-sdk/pull/19301) Fix vulnerability in `incrementReferenceCount` in distribution.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changelog entry for the bug fix is correctly formatted, follows the guiding principles, and includes the appropriate PR link. However, it lacks detail about the nature of the vulnerability and how it was fixed. Consider adding a brief description of the vulnerability and the fix applied for better clarity.

3 changes: 2 additions & 1 deletion x/distribution/keeper/validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,10 +124,11 @@ func (k Keeper) incrementReferenceCount(ctx context.Context, valAddr sdk.ValAddr
if err != nil {
return err
}

historical.ReferenceCount++
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation of the panic condition when the reference count exceeds 2 in the incrementReferenceCount function aligns with the PR's objective to mitigate the identified vulnerability. However, this approach introduces a hard stop in the execution flow, which could lead to unhandled panics in production. Consider implementing a more graceful error handling mechanism that allows the calling function to manage the error, enhancing the robustness of the system.

if historical.ReferenceCount > 2 {
panic("reference count should never exceed 2")
}
historical.ReferenceCount++
return k.ValidatorHistoricalRewards.Set(ctx, collections.Join(valAddr, period), historical)
}

Comment on lines 124 to 134
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [198-204]

The call to incrementReferenceCount within updateValidatorSlashFraction does not account for the potential panic introduced by the new logic in incrementReferenceCount. This oversight could lead to unhandled exceptions during slashing events. It's crucial to wrap this call in a try-catch or equivalent error handling structure to prevent crashes and ensure the system's resilience.

Expand Down
Loading