Skip to content

Conversation

pdyraga
Copy link
Member

@pdyraga pdyraga commented Apr 28, 2022

The application may revert authorization decrease request, if there is
already one pending for the given application.

This is entirely up to the application if overwriting pending request at
the given moment is fine or not.

This is important for TBTC and Random Beacon because allowing to
"decrease authorization decrease request" at any moment may be used for
pool manipulation and becoming a free-rider.

See threshold-network/keep-core#2960
See threshold-network/keep-core#2961

The application may revert authorization decrease request, if there is
already one pending for the given application.

This is entirely up to the application if overwriting pending request at
the given moment is fine or not.

This is important for TBTC and Random Beacon because allowing to
"decrease authorization decrease request" at any moment may be used for
pool manipulation and becoming a free-rider.
@vzotova
Copy link
Contributor

vzotova commented Apr 29, 2022

I thought when second request is send Application will try to start over decreasing process, so it's not overwriting but resetting. And if we always reset why it could lead to manipulation? (I'm fine if app can fail on such tx)

nkuba added a commit to threshold-network/keep-core that referenced this pull request Apr 29, 2022
Introduced authorization decrease change period parameter to the beacon

See threshold-network/solidity-contracts#99

Introduced authorization decrease change period parameter to the beacon

This value protect against malicious operators who manipulate
their weight by overwriting authorization decrease request, and
lowering or increasing their eligible stake this way.

Authorization decrease change period is the time period before the authorization
decrease delay end, during which the authorization decrease request can be
overwritten.

When the request is overwritten, the authorization decrease delay is
reset.

For example, if `authorizationDecraseChangePeriod` is set to 4
days, `authorizationDecreaseDelay` is set to 14 days, and someone
requested authorization decrease, it means they can not
request another decrease for the first 10 days. After 10 days pass,
they can request again and overwrite the previous authorization
decrease request. The delay time will reset for them and they
will have to wait another 10 days to alter it and 14 days to
approve it.

If set to a value equal to `authorizationDecreaseDelay, it means
that authorization decrease request can be always overwritten.
If set to zero, it means authorization decrease request can not
be overwritten until the delay end, and one needs to wait for the entire
authorization decrease delay to approve their decrease or to alter it.

```
(1) authorization decrease requested timestamp
(2) from this moment authorization decrease request can be
    overwritten
(3) from this moment authorization decrease request can be
    approved, assuming it was NOT overwritten in (2)

  (1)                            (2)                        (3)
 --x------------------------------x--------------------------x---->
   |                               \________________________/
   |                             authorizationDecreaseChangePeriod
    \______________________________________________________/
                   authorizationDecreaseDelay
```
nkuba added a commit to threshold-network/keep-core that referenced this pull request Apr 29, 2022
Introduced authorization decrease change period parameter to ECDSA

See threshold-network/solidity-contracts#99

This value protect against malicious operators who manipulate
their weight by overwriting authorization decrease request, and
lowering or increasing their eligible stake this way.

Authorization decrease change period is the time period before the authorization
decrease delay end, during which the authorization decrease request can be
overwritten.

When the request is overwritten, the authorization decrease delay is
reset.

For example, if `authorizationDecraseChangePeriod` is set to 4
days, `authorizationDecreaseDelay` is set to 14 days, and someone
requested authorization decrease, it means they can not
request another decrease for the first 10 days. After 10 days pass,
they can request again and overwrite the previous authorization
decrease request. The delay time will reset for them and they
will have to wait another 10 days to alter it and 14 days to
approve it.

If set to a value equal to `authorizationDecreaseDelay, it means
that authorization decrease request can be always overwritten.
If set to zero, it means authorization decrease request can not
be overwritten until the delay end, and one needs to wait for the entire
authorization decrease delay to approve their decrease or to alter it.

```
(1) authorization decrease requested timestamp
(2) from this moment authorization decrease request can be
    overwritten
(3) from this moment authorization decrease request can be
    approved, assuming it was NOT overwritten in (2)

  (1)                            (2)                        (3)
 --x------------------------------x--------------------------x---->
   |                               \________________________/
   |                             authorizationDecreaseChangePeriod
    \______________________________________________________/
                   authorizationDecreaseDelay

```
Copy link
Member

@manumonti manumonti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this contribution, but I agree with Vicky's question

@pdyraga
Copy link
Member Author

pdyraga commented Aug 19, 2022

I thought when second request is send Application will try to start over decreasing process, so it's not overwriting but resetting. And if we always reset why it could lead to manipulation? (I'm fine if app can fail on such tx)

When the staker requests authorization decrease, their weight in the sortition pool is decreased right away. We do it to ensure the staker is not going to be selected to new signing groups based on their previous weight since after the delay period we'll not be able to slash their entire previous stake.

For example, if I have 100 tokens staked now and I request authorization decrease to 20 tokens, I should only be selected to new groups based on 20 tokens weight with an assumption that the protocol can slash me for no more than 20 tokens if I do something wrong in the new group.

If we allow to discard existing authorization decrease request and start a new one, the following manipulation is possible:

  1. I have 100 tokens staked and I earn rewards for 100 tokens staked.
  2. The new signing group selection is about to happen, I request authorization decrease to 1 token.
  3. The new signing group selection selects me to the group based on 1 token weight (minimal responsibility).
  4. I send a new authorization decrease request to 99 tokens. I now earn rewards for 99 tokens weight.

To prevent this manipulation, beacon and ECDSA can reject the authorization decrease request if the amount we are decreasing to is higher than the previous one. They can also accept the new request - with the delay being reset - if the amount to which we are decreasing to is lower than the previous one.

@pdyraga
Copy link
Member Author

pdyraga commented Aug 19, 2022

The red build of system tests has been fixed in #103 at commit 7c05a34. The tests are passing, it's just some unrelated error returned by the runner after the tests are done executing. We can merge #103 first, then merge main to this PR and the build should turn green.

Copy link
Contributor

@vzotova vzotova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I see, looks good!

@cygnusv cygnusv merged commit 051a72b into main Aug 24, 2022
@cygnusv cygnusv deleted the ackchyually branch August 24, 2022 10:55
@pdyraga pdyraga added this to the v1.2.0 milestone Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants