fix: delete account after unstake should not cause node to crash #2688
Conversation
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
bowenwang1996
requested review from
evgenykuzyakov,
frol,
MaksymZavershynskyi and
SkidanovAlex
as code owners
|
|
evgenykuzyakov
approved these changes
May 20, 2020
There was a problem hiding this comment.
@bowenwang1996 Please add a follow up issue, to fix it in the epoch manager.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
As described in #2687, there is a critical vulnerability caused by a situation where accounts are deleted after they unstake but we still try to return 0 stake to the account. This PR submits an easy fix to the issue by not requiring account to exist when the returned stake is no more than 0. Please note that this is not the optimal way to fix it. The better way would be to change epoch manager behavior so that it will only record accounts in
stake_changeif it is needed. In the case of unstaking, it means that a stake change should only be recorded if it is a change from nonzero (validator or fishermen) to zero. However, this PR is enough to quickly patch the issue so that we can spend more time ironing out the proper and better fix. Resolves #2687.