fix(x/distribution): vulnerable incrementReferenceCount in distribution #19301
Conversation
WalkthroughThe modification to the Changes
Assessment against linked issues
Related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -124,10 +124,11 @@ func (k Keeper) incrementReferenceCount(ctx context.Context, valAddr sdk.ValAddr | |||
| if err != nil { | |||
| return err | |||
| } | |||
|
|
|||
| historical.ReferenceCount++ | |||
There was a problem hiding this comment.
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 err != nil { | ||
| return err | ||
| } | ||
|
|
||
| historical.ReferenceCount++ | ||
| if historical.ReferenceCount > 2 { | ||
| panic("reference count should never exceed 2") | ||
| } | ||
| historical.ReferenceCount++ | ||
| return k.ValidatorHistoricalRewards.Set(ctx, collections.Join(valAddr, period), historical) | ||
| } | ||
|
|
There was a problem hiding this comment.
📝 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.
julienrbrt
changed the title
In the original code, the reference count is checked first. If it exceeds the limit of 2, a panic is triggered. However, if the count is exactly 2, it will still be incremented before the panic occurs. This means that the count could potentially become 3, violating the intended constraint. By swapping the order of operations, the fixed code ensures that the reference count is properly checked and incremented without compromising intended logic and constraints |
|
Makes sense. @naveenkumarcse2018 we need a changelog entry for the bug fix. Then we can merge. Thank you!! |
|
ping @naveenkumarcse2018 about the changelog |
|
|
||
| ### Bug Fixes | ||
|
|
||
| * [#19301](https://github.com/cosmos/cosmos-sdk/pull/19301) Fix vulnerability in `incrementReferenceCount` in distribution. |
There was a problem hiding this comment.
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.
|
updated the changelog @julienrbrt @alexanderbez |
Description
Closes: #7609
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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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...
Summary by CodeRabbit
incrementReferenceCountfunction to correctly manage reference counts.