feat(RewardsStreamerMP): introduce emergency mode and ability to leave #72
Conversation
|
The ideal logic would be: Stake Manager contains a flag which is read by Stake Vault. When this flag is true, Stake Vault just allows withdraw and Stake Manager wont accept any iteractions anymore. |
0x-r4bbit
force-pushed
the
feat/emergency-exit
branch
3 times, most recently
from
2f4b5a0 to
9e1dcaf
Compare
|
@3esmit So, the reason I've done it this way (StakeManager sets flag, and reverts accordingly in functions where necessary), is because the logic for preventing interaction with the manager should stay the same no matter what stakevault you interact with. For example, if we upgrade StakeVault to a different version, we'd otherwise have to ensure that it reverts in functions where it should revert. If we keep the revert logic in the StakeManager, that's one thing less to keep in mind. -- Are there any other good reasons to have StakeVault explicitly check emergency mode first, before interacting? Happy to make the necessary changes in that case, but right now I don't see how that would improve the state. |
|
I've updated this PR with a certora rule that ensures only view functions, ownable functions, trusted codehash functions and |
src/RewardsStreamerMP.sol
Outdated
| @@ -132,6 +135,27 @@ contract RewardsStreamerMP is IStakeManager, TrustedCodehashAccess, ReentrancyGu | |||
| account.accountRewardIndex = rewardIndex; | |||
| } | |||
|
|
|||
| function leave() external onlyTrustedCodehash nonReentrant { | |||
There was a problem hiding this comment.
emergency mode should not have any complex logic. what happens if there is a problem, such as underflow, inside of _updateGlobalState() or _updateAccountMP()?
Emergency exit suppousedly should not process any storage or anything, just allow withdraws.
| uint256 aliceInitialRewardBalance = rewardToken.balanceOf(vaults[alice]); | ||
|
|
||
| streamer.enableEmergencyMode(); | ||
|
|
There was a problem hiding this comment.
To properly test emergency mode, cause the code to make a division by zero in the logic, or whatever unexpected fail.
There would be no reason in having an emergency mode for expected cases, that wouldnt be called emergency mode.
There was a problem hiding this comment.
So this is now testing the "benign" exit mode case, where the owner enables it, allowing users to exit.
src/RewardsStreamerMP.sol
Outdated
|
|
||
| uint256 accountRewards = calculateAccountRewards(msg.sender); | ||
| if (accountRewards > 0) { | ||
| distributeRewards(msg.sender, accountRewards); |
There was a problem hiding this comment.
In contrast with emergency exits on real world, they are usually used when there is a fire in the building. In that case, you don't go grab all your stuff before leave, you just run for your life directly to the exit.
src/RewardsStreamerMP.sol
Outdated
| Account storage account = accounts[msg.sender]; | ||
|
|
||
| if (account.stakedBalance > 0) { | ||
| _updateGlobalState(); |
There was a problem hiding this comment.
Why need to update global state if its not going being used anymore?
0x-r4bbit
force-pushed
the
feat/emergency-exit
branch
from
9e1dcaf to
d883a7a
Compare
0x-r4bbit
force-pushed
the
feat/emergency-exit
branch
2 times, most recently
from
cfb50de to
f5109a9
Compare
|
@3esmit @gravityblast I've updated this PR according to the discussion with had with @3esmit In summary:
Please give this another review |
| @@ -65,6 +66,7 @@ contract RewardsStreamerMPTest is Test { | |||
| address account; | |||
| uint256 rewardBalance; | |||
| uint256 stakedBalance; | |||
| uint256 vaultBalance; | |||
There was a problem hiding this comment.
I had to introduce vaultBalance here so we can differentiate between accountInfo.stakedBalance and balanceOf(vault)
This adds a new emergency mode that can be enabled by the owner of the system. When in emergency mode, stakers or `StakeVault`s can leave the system immediately. This also applies when there was a malicious upgrade and a call to `emergencyModeEnabled()` panics. To have this in a fully secure manner, we still have to add the counter part of "leaving" the system. This will allow users that don't agree with a (malicious) upgrade to get their funds out of the vaults regardless. Closes #66
0x-r4bbit
force-pushed
the
feat/emergency-exit
branch
from
f5109a9 to
ff84621
Compare
This adds a new emergency mode that can be enabled by the owner of the system.
When in emergency mode, stakers or
StakeVaults can leave the system immediately.This also applies when there was a malicious upgrade and a call to
emergencyModeEnabled()panics.To have this in a fully secure manner, we still have to add the counter
part of "leaving" the system. This will allow users that don't agree
with a (malicious) upgrade to get their funds out of the vaults
regardless.
Closes #66
Checklist
Ensure you completed all of the steps below before submitting your pull request:
pnpm adorno?pnpm verify?