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

Jump to bottom

Stake return can cause node to crash if the account is deleted #2687

Closed
bowenwang1996 opened this issue May 20, 2020 · 0 comments · Fixed by #2688
Closed

Stake return can cause node to crash if the account is deleted #2687

bowenwang1996 opened this issue May 20, 2020 · 0 comments · Fixed by #2688
Assignees
@bowenwang1996
Labels
A-chain Area: Chain, client & related C-bug Category: This is a bug P-critical Priority: critical

Comments

@bowenwang1996
Copy link
Collaborator

bowenwang1996 commented May 20, 2020 •
edited
Loading

When we compute the stake return at the beginning of the epoch, we assume that the accounts that need stake return exist, which, under normal circumstances make sense, but is not necessarily true when the return stake is 0. This creates a vector of attack as follows:

  1. become a validator
  2. stake 0
  3. wait one epoch boundary
  4. stake 1
  5. wait 2 epoch boundaries until the account locked balance is 0
  6. delete account
  7. nodes will crash on next epoch boundary

Note: this critical vulnerability is reported by a third party partner (Thanks @kirk-baird).
@bowenwang1996 bowenwang1996 self-assigned this May 20, 2020
@bowenwang1996 bowenwang1996 added A-chain Area: Chain, client & related P-critical Priority: critical C-bug Category: This is a bug labels May 20, 2020
This was referenced May 20, 2020
Merged
Open
bowenwang1996 added a commit that referenced this issue May 20, 2020
@bowenwang1996
fix: delete account after unstake should not cause node to crash (#2688)
f46cfc0 
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_change` if 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bowenwang1996 bowenwang1996

Labels
A-chain Area: Chain, client & related C-bug Category: This is a bug P-critical Priority: critical
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: delete account after unstake should not cause node to crash near/nearcore
1 participant
@bowenwang1996