-
Notifications
You must be signed in to change notification settings - Fork 54
Add pausing to staking contract #606
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Ramarti
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address comments and also add the deploy script for new version.
That way we can ensure new admin is the timelock (get admin value from owner() using old implementation)
I don't see the schedule/cancel/execute scripts and txs in this repo. Do they go here or elsewhere? |
Ramarti
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
- We can do the upgrade transactions in a different PR
- Default min fee might be lower if this is merged after the staking changes
0xHansLee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you provide some more contexts on adding pausing to only stake function? I wonder if we don't need to add it to other functions, e.g. create validator, redelegate, and unstake.
| /// @param maxCommissionChangeRate The maximum commission change rate of the validator. | ||
| /// @param supportsUnlocked Whether the validator supports unlocked staking. | ||
| /// @param data Additional data for the validator. | ||
| function createValidator( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to add the whenNotPaused modifier here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createValidator and redelegate we can consider - what do you think @Ramarti ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide why you consider to add the pausing only to create validator and redelegate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point @0xHansLee .
Our initial thought was to start with whatever stops a possible loss of funds from the users.
Revisiting this I think we could go with 2 options
- Every non admin methos is pausable but
unstake - Every non admin method is pausable
Always allow users to withdraw during an attack vs the attack may be exploiting the unstake logic so nothing should come out.
I think we can start with everything pausable and post in the Forum, see how community feels
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted so that "Every non admin method is pausable": 354e206
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would depend on which kind of attack we intend to mitigate via a pause.
The first option, supporting immediate withdrawal, would likely be needed in a situation where there is a problem with the staking system itself. On the other hand, if we need to block withdrawals via unstake, it is more likely due to issues such as private key leakage or compromised permissions of a contract account involved in staking. In such cases, a pause would need to be triggered before the funds are transferred to another account with malicious or unintended authority.
In my humble opinion, it seems more reasonable to focus on the latter rather than the former. Since the staking system is based on the Cosmos SDK, I believe the likelihood of critical issues originating from the staking system itself is relatively low.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After talking with the security council, we are leaning to pause everything but admin methods as the safer method. We are waiting on legal input for confirmation.
Adds pausing capability to
stakeandstakeOnBehalffunctions.Adds access control roles which replaces the original
Ownable2StepUpgradeable