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.

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

Interaction of mandatory reward-withdraw and reward truncation #2914

Closed
cwgoes opened this issue Nov 27, 2018 · 12 comments · Fixed by #2984
Closed

Interaction of mandatory reward-withdraw and reward truncation #2914

cwgoes opened this issue Nov 27, 2018 · 12 comments · Fixed by #2984
Assignees
Labels
C:x/distribution distribution module related T:Bug

Comments

@cwgoes
Copy link
Contributor

cwgoes commented Nov 27, 2018

Ref #2764
Ref https://forum.cosmos.network/t/3-important-topics-to-discuss-on-fee-reward-distribution-logic/

Any delegator can force decimal truncation of a validators' or the validators' self-delegation rewards by delegating/undelegating a tiny amount (to cause a power change). As delegator rewards are truncated to integers and rewards are minted each block, this seems like a substantial attack vector, with which the reward of a validators' self-bond could be almost totally negated.

@cwgoes cwgoes added T:Bug C:x/distribution distribution module related game-of-stakes labels Nov 27, 2018
@cwgoes
Copy link
Contributor Author

cwgoes commented Nov 27, 2018

cc @rigelrozanski @alexanderbez

I wonder if all of these hooks are quite necessary for the lazy calculation, I think a few of the ones we have might not be.

@rigelrozanski
Copy link
Contributor

We do not require withdrawing a validator self-delegation when a 3rd party delegates-to/undelegates-from that validator. We do however require that that validator withdraw all of it's unclaimed rewards from the global pool to it's local pools when a 3rd party delegates-to/undelegates-from it (affecting its power)

The reason the code currently does this is out of convenience when this was being added if I'm not mistaken

@alexanderbez
Copy link
Contributor

Awesome. Mind if I tackle this?

@rigelrozanski
Copy link
Contributor

no-t-at-tall :)

@alexanderbez
Copy link
Contributor

Discussed with @cwgoes and we agreed that the staking spec should first be reconciled prior to this. This is mainly motivated by some inconsistencies/inefficiencies in the staking hooks.

@cwgoes
Copy link
Contributor Author

cwgoes commented Nov 29, 2018

As in, we should correct anything incorrect in the spec, and then update the code to match (which it presently does not).

@rigelrozanski
Copy link
Contributor

I'm not entirely sure that the spec is the best way for us to spend our documentation efforts at the moment - For instance, I think diagram creation will do better to both explain the logic flow, as well as clarify hook calls. Diagrams fall more under description documentation than specs - we have very little description documentation in general

@rigelrozanski
Copy link
Contributor

sry I meant - Explanation documentation https://www.divio.com/blog/documentation/

@cwgoes
Copy link
Contributor Author

cwgoes commented Dec 10, 2018

Reopening this as we have reintroduced the attack.

@alexanderbez
Copy link
Contributor

I think we can close this due to F1, correct me if I'm wrong @cwgoes.

@cwgoes
Copy link
Contributor Author

cwgoes commented Dec 22, 2018

Let's leave it open for now; F1 should prevent forced-withdraws, but we haven't merged it yet.

@cwgoes cwgoes assigned cwgoes and unassigned alexanderbez Jan 16, 2019
@cwgoes
Copy link
Contributor Author

cwgoes commented Jan 16, 2019

Closed by #3099, which is about to be merged.

@cwgoes cwgoes closed this as completed Jan 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/distribution distribution module related T:Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants