Cosmos-sdk: Undelegate may lose the delegator鈥檚 tokens when exceed the MaxEntries of unbonding delegations

Created on 14 Jun 2019  路  2Comments  路  Source: cosmos/cosmos-sdk

Summary of Bug

Undelegate may lose the delegator鈥檚 tokens when exceed the MaxEntries of unbonding delegations

Version

$ git log -1
commit 95f3d322503cd79ddeebea8986261594b239aed9 (HEAD -> master, origin/master, origin/HEAD)
Author: colin axner colinaxner@berkeley.edu
Date: Thu Jun 13 06:54:17 2019 -0700

Merge PR #4536: Return account queries with height

Steps to Reproduce

Suppose the MaxEntries of unbonding delegations is 7. When delegator perform the eighth undelegate, the Undelegate will deduct the shares in the types.Delegation, but not put the redelegation in the UBDQueue, which will cause the reduce of the delegator shares, and the corresponding account balance will not receive the reduced token, Supplementing TestUnbondingDelegationsMaxEntries with delegation and account balance check can be exposed to this problem


For Admin Use

  • [ ] Not duplicate issue
  • [ ] Appropriate labels applied
  • [ ] Appropriate contributors tagged
  • [ ] Contributor assigned/self-assigned
code-hygiene staking

Most helpful comment

Thanks for opening up an issue @helldealer. However, _this isn't technically a bug_. If a validator/delegator pair has max entries, then the message will fail and no state will be written. i.e. the order of operations doesn't really matter -- no shares will be deducted.

Suppose the MaxEntries of unbonding delegations is 7. When delegator perform the eighth undelegate, the Undelegate will deduct the shares in the types.Delegation, but not put the redelegation in the UBDQueue, which will cause the reduce of the delegator shares, and the corresponding account balance will not receive the reduced token

Not true as the message will fail because HasMaxUnbondingDelegationEntries will return true on the 8th attempt.

However, I do agree that the code and logic reads better by having the max entries check _first_! But again, no bug.

All 2 comments

Thanks for opening up an issue @helldealer. However, _this isn't technically a bug_. If a validator/delegator pair has max entries, then the message will fail and no state will be written. i.e. the order of operations doesn't really matter -- no shares will be deducted.

Suppose the MaxEntries of unbonding delegations is 7. When delegator perform the eighth undelegate, the Undelegate will deduct the shares in the types.Delegation, but not put the redelegation in the UBDQueue, which will cause the reduce of the delegator shares, and the corresponding account balance will not receive the reduced token

Not true as the message will fail because HasMaxUnbondingDelegationEntries will return true on the 8th attempt.

However, I do agree that the code and logic reads better by having the max entries check _first_! But again, no bug.

Thanks @alexanderbez. It uses a CacheMultiStore when run msg, so failed msg will not be written.

Was this page helpful?
0 / 5 - 0 ratings