refactor(StakeVault): Remove obsolete trust stake manager logic #237
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR removes the obsolete trust stake manager logic from the StakeVault contract. The logic previously required users to explicitly trust the stake manager on every upgrade, but this has been determined to be unnecessary since the leave function provides an adequate escape hatch regardless of stake manager upgrade status.
Key changes:
- Removed trust-based access control for stake manager operations
- Simplified the
leavefunction to work without trust checks - Cleaned up related test cases and error definitions
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/StakeVault.sol | Removed trust stake manager logic, error definitions, and access control modifiers |
| test/StakeVault.test.sol | Removed tests for trust-based restrictions and updated leave function test |
| test/RewardsStreamerMP.t.sol | Removed comprehensive trust manager upgrade test scenarios |
| .gas-snapshot | Updated gas consumption measurements reflecting the simplified code |
| .gas-report | Updated detailed gas usage reports for modified functions |
| function updateLockUntil(uint256 _lockUntil) external { | ||
| if (msg.sender != address(stakeManager)) { | ||
| revert StakeVault__NotAuthorized(); | ||
| } |
There was a problem hiding this comment.
The updateLockUntil function removes the onlyTrustedStakeManager modifier but still has access control logic inside. This creates an inconsistency where the function is publicly callable but has internal authorization checks. Consider adding a proper access control modifier or make the access control more explicit in the function signature.
| function updateLockUntil(uint256 _lockUntil) external { | |
| if (msg.sender != address(stakeManager)) { | |
| revert StakeVault__NotAuthorized(); | |
| } | |
| function updateLockUntil(uint256 _lockUntil) external onlyTrustedStakeManager { |
Copilot uses AI. Check for mistakes.
| // We have to `try/catch` here in case the upgrade was bad and `leave()` | ||
| // either doesn't exist on the new stake manager or reverts for some reason. | ||
| // If it was a benign upgrade, it will cause the stake manager to properly update | ||
| // If it was a good upgrade, it will cause the stake manager to properly update | ||
| // its internal accounting before we move the funds out. |
There was a problem hiding this comment.
[nitpick] The comment has been updated from 'malicious' to 'bad' but the explanation is incomplete. The comment should clarify what constitutes a 'bad' upgrade and why the try/catch is necessary for proper error handling.
Copilot uses AI. Check for mistakes.
github-project-automation
bot
moved this from Needs Review
to Ready to Merge
in Tasks - Smart Contracts
|
This doesn't pass certora atm. Pls fix that first - otherwise, good changes! |
Description
Fixes #234
Replaces #235
This PR simply removes the logic of requring to trust the stake manager on every upgrade.
The reason for this is because there is no advantage or risk for funds if the stake manager upgraded. The leave function operates regardless of the stake manager being upgraded, and is a escape hatch for the case something bad happens during the upgrade.
There is no need for checking if the upgrade happened or not because the user still needs to wait the stake lock period, so leave also works as a unstake function.
Checklist
Ensure you completed all of the steps below before submitting your pull request:
pnpm adorno?pnpm verify?