This repository was archived by the owner on May 22, 2023. It is now read-only.
Require beneficiary to be set for withdraw #478
Merged
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
We were handling a special case where beneficiary is not set on TokenStakingStub contract which was inconsistent with what the TokenStaking contract actually does. If the beneficiary is not set the contract will simply return zero address and here we do the same in the stub.
It is possible that unbonded value is deposited for the operator before beneficiary is set for that operator (via stake delegation). In such case beneficiaryOf will return zero address and the funds will be transfered to this address. Here we check if the beneficiary is set, if not the transaction will revert.
Before sending the valuse to beneficiary we need to check if it's set for the operator. It could happen that the value was sent to the zero address in case of beneficiary not defined.
Since the value is not sent directly to operator address we added also beneficiary to the event to help tracking transfers.
This was referenced Jun 16, 2020
pdyraga
reviewed
Jun 17, 2020
pdyraga
reviewed
Jun 23, 2020
There was a problem hiding this comment.
Everything looks good to me except the piece in BondedECDSAKeep (comment from the last week). This situation is not possible to happen and I don't think we should guard against it in the code. It's additional instruction that could be confusing and even if we had it and reverted the transaction, there is no way to recover - delegation parameters can not be updated.
This reverts commit c3de57f. To become a keep member, stake delegation has to exist and the beneficiary has to be defined.
pdyraga
reviewed
Jun 24, 2020
We removed the part of code that the test were validating, so we need to remove the test too.
|
@pdyraga it's ready |
pdyraga
approved these changes
Jun 30, 2020
Do we want to fix this by preventing withdrawal for an unset beneficiary, or by preventing bonding? |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
In this PR we update ETH withdraws with checks that a beneficiary is set for the operator.
Previously it was possible that bonding value was deposited for the operator before stake delegation happened, so the beneficiary was not set yet. In such a case, the operator was able to withdraw the unbonded value, but the
tokenStaking.beneficiaryOfreturned zero address, to which funds could be transferred.Here we added verification if the beneficiary is defined. If not the transaction will revert.