feat: P3 Permanent Staking Redesign - #33
Conversation
Introduces permanent (non-decaying) staking positions alongside existing ve-style locks. ## Contract Changes - StakeWeight: Added permanent lock creation, conversion, and unlock mechanisms - StakingRewardDistributor: Updated reward calculations for permanent weights - LockedTokenStaker: Added handling for permanent positions in vesting claims - OldStakeWeight: Reference implementation for upgrade verification ## Documentation - docs/AUDIT_SCOPE_P3.md: Comprehensive audit specification with security requirements - docs/P3_STAKING_REDESIGN.md: Product specification and implementation details - CLAUDE.md: Testing patterns and codebase-specific guidelines ## Testing - Fork tests for mainnet upgrade safety verification - Integration tests for permanent lock lifecycle and edge cases - Invariant tests ensuring system consistency - Fuzz tests for permanent lock operations 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull Request Overview
This PR implements a P3 permanent staking redesign that introduces non-decaying staking positions alongside existing ve-style locks. The implementation adds permanent lock functionality, conversion mechanisms between lock types, and comprehensive testing to ensure the system maintains data integrity and reward distribution accuracy.
Key changes include:
- Permanent lock system: New permanent locks with constant weight based on duration multipliers
- Lock conversion functionality: Converting between decaying and permanent lock states with proper checkpointing
- Enhanced reward distribution: Updated StakingRewardDistributor to handle mixed permanent and decaying positions
Reviewed Changes
Copilot reviewed 52 out of 52 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| evm/src/StakeWeight.sol | Core permanent lock implementation with conversion, creation, and weight calculation logic |
| evm/src/StakingRewardDistributor.sol | Updated reward distribution to support permanent locks with proper balance calculations |
| evm/test/unit/fuzz/ | Comprehensive fuzz testing for permanent lock operations and edge cases |
| evm/test/invariant/ | Invariant testing with handlers for permanent lock operations and supply consistency |
| evm/test/integration/ | Integration tests covering permanent lock workflows, conversions, and reward scenarios |
| evm/test/fork/ | Fork testing validating upgrade safety and data integrity with real mainnet positions |
Comments suppressed due to low confidence (1)
evm/test/invariant/handlers/StakingRewardDistributorHandler.sol:1
- The variable name 'multiplier' is misleading since it's used to calculate a large amount rather than as a multiplier. Consider renaming to 'amountFactor' or 'scaleFactor' for clarity.
// SPDX-License-Identifier: MIT
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| function testFuzz_CreatePermanentLock_WeightCalculation(uint256 amount, uint256 durationIndex) public { | ||
| // Bound inputs | ||
| amount = bound(amount, 1e18, 10_000e18); // 1 to 10,000 tokens | ||
| durationIndex = durationIndex % 7; // Ensure it's always 0-6 using modulo |
There was a problem hiding this comment.
Using modulo for input validation in fuzz tests can mask edge cases. Consider using bound(durationIndex, 0, 6) instead for more predictable fuzzing behavior and better coverage of the valid duration range.
| durationIndex = durationIndex % 7; // Ensure it's always 0-6 using modulo | |
| durationIndex = bound(durationIndex, 0, 6); // Ensure it's always 0-6 using bound |
| // Bound inputs | ||
| amount = bound(amount, 1e18, 10_000e18); | ||
| initialLockTime = bound(initialLockTime, 1 weeks, 104 weeks); // 1 week to 2 years | ||
| permanentDurationIndex = permanentDurationIndex % 7; // Ensure it's always 0-6 using modulo |
There was a problem hiding this comment.
Consistent with the previous comment, using bound(permanentDurationIndex, 0, 6) would provide better fuzzing coverage and more predictable test behavior than modulo operation.
| permanentDurationIndex = permanentDurationIndex % 7; // Ensure it's always 0-6 using modulo | |
| permanentDurationIndex = bound(permanentDurationIndex, 0, 6); // Ensure it's always 0-6 using bound |
| uint256 unlockTime; | ||
| bool hasLock; | ||
| uint256 lockCreatedAt; // Ghost variable: timestamp when lock was created | ||
| bool isPermanent; // Ghost variable: track if lock is permanent |
There was a problem hiding this comment.
[nitpick] Consider adding more detailed documentation for these ghost variables explaining their purpose in invariant testing and how they relate to the reward distribution validation.
| bool isPermanent; // Ghost variable: track if lock is permanent | |
| // Ghost variable: Indicates if the user's lock is permanent. | |
| // Used exclusively in invariant testing to validate that reward distribution logic | |
| // correctly accounts for users with permanent locks, ensuring that such users are | |
| // handled according to the intended reward distribution rules. | |
| bool isPermanent; |
| // 100M tokens / 500 allocations at 25% unlock passed | ||
| // 100M / 500 / 4 = 50k tokens per allocation | ||
| uint256 maxAmount = 1e26 / 500 / 4; |
There was a problem hiding this comment.
Consider extracting these magic numbers into named constants (TOTAL_TOKEN_SUPPLY, ALLOCATION_COUNT, UNLOCK_PERIODS) to improve readability and maintainability of the test bounds.
| // NOTE: This test exposes a critical issue with converting decaying locks to permanent. | ||
| // The global totalSupply calculation becomes incorrect after conversion due to how | ||
| // the checkpoint mechanism handles the state transition. This would significantly | ||
| // impact reward distribution. A proper fix would require either: | ||
| // 1. Tracking permanent supply separately (adds complexity) | ||
| // 2. Preventing conversions between states (simpler but less flexible) | ||
| // 3. Implementing state-aware bias adjustments in _checkpoint (complex) | ||
| function test_AfterConvertingDecayingToPermanent_CRITICAL_ISSUE() external { |
There was a problem hiding this comment.
This comment indicates a critical issue but the test name suggests it's intentionally testing this issue. Either fix the identified problem or clarify that this test validates the current behavior is working as intended after the redesign.
| // NOTE: This test exposes a critical issue with converting decaying locks to permanent. | |
| // The global totalSupply calculation becomes incorrect after conversion due to how | |
| // the checkpoint mechanism handles the state transition. This would significantly | |
| // impact reward distribution. A proper fix would require either: | |
| // 1. Tracking permanent supply separately (adds complexity) | |
| // 2. Preventing conversions between states (simpler but less flexible) | |
| // 3. Implementing state-aware bias adjustments in _checkpoint (complex) | |
| function test_AfterConvertingDecayingToPermanent_CRITICAL_ISSUE() external { | |
| // NOTE: This test validates that after the redesign/fix, converting a decaying lock to permanent | |
| // correctly updates the global totalSupply calculation. Previously, this operation exposed a critical | |
| // issue where totalSupply became incorrect due to how the checkpoint mechanism handled the state transition, | |
| // significantly impacting reward distribution. The current implementation ensures that totalSupply and user | |
| // balances are updated correctly after conversion, and this test verifies that behavior. | |
| function test_AfterConvertingDecayingToPermanent_BehavesCorrectly() external { |
| // else calculate rewards that user should get. | ||
| // Calculate balance for current week BEFORE moving to new epoch | ||
| // This properly handles both permanent and decaying locks | ||
| uint256 balanceOf = this.balanceOfAt(user, userWeekCursor); |
There was a problem hiding this comment.
Using external function call this.balanceOfAt() instead of internal function creates unnecessary overhead. Consider refactoring to use an internal version of this function for better gas efficiency.
| uint256 balanceOf = this.balanceOfAt(user, userWeekCursor); | |
| uint256 balanceOf = balanceOfAt(user, userWeekCursor); |
| uint256 amount = SafeCast.toUint256(lock.amount); | ||
| uint256 permanentWeight = Math.mulDiv(amount, duration, MAX_LOCK_CAP); |
There was a problem hiding this comment.
Consider extracting the permanent weight calculation into a separate internal function since this formula is used in multiple places (createPermanentLock, updatePermanentLock, etc.) to ensure consistency and reduce duplication.
| uint256 durationWeeks = duration / 1 weeks; | ||
| if ( | ||
| durationWeeks != 4 && durationWeeks != 8 && durationWeeks != 12 && durationWeeks != 26 | ||
| && durationWeeks != 52 && durationWeeks != 78 && durationWeeks != 104 | ||
| ) revert InvalidDuration(duration); |
There was a problem hiding this comment.
The duration validation logic is duplicated across multiple functions. Consider extracting this into a private _isValidDuration(uint256 duration) function to ensure consistency and reduce code duplication.
4266c8e to
463881d
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 65 out of 65 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
evm/test/integration/concrete/staking-reward-distributor/permanent-locks/permanentLockRewards.t.sol:1
- This comment suggests there's a potential issue with week 0 reward eligibility. Consider verifying if this is the intended behavior or if it needs to be addressed in the implementation.
// SPDX-License-Identifier: MIT
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Use modulo to ensure we stay within bounds even with extreme values | ||
| uint256 durationIndex = duration % 7; |
There was a problem hiding this comment.
[nitpick] Use bound() instead of modulo for duration selection to avoid bias in fuzz testing. This pattern appears multiple times in the file and should be consistently addressed.
| // Use modulo to ensure we stay within bounds even with extreme values | |
| uint256 durationIndex = duration % 7; | |
| uint256 durationIndex = bound(duration, 0, 6); |
| // Check that the DEFAULT_ADMIN_ROLE is granted correctly | ||
| bytes32 DEFAULT_ADMIN_ROLE = 0x00; | ||
| assertTrue(stakingRewardDistributor.hasRole(DEFAULT_ADMIN_ROLE, users.admin)); |
There was a problem hiding this comment.
Consider using the constant from AccessControl interface instead of hardcoding the role hash. This makes the code more readable and maintainable.
| // Hardcode addresses for security - prevents front-running attacks | ||
| address OPTIMISM_ADMIN_TIMELOCK = 0x61cc6aF18C351351148815c5F4813A16DEe7A7E4; | ||
| address TREASURY_MULTISIG = 0xa86Ca428512D0A18828898d2e656E9eb1b6bA6E7; |
There was a problem hiding this comment.
[nitpick] Consider defining these addresses as constants at the contract level to improve readability and make them easier to update if needed. While hardcoding for security is correct, constants would be cleaner.
| uint256 amount = 100e18; | ||
| uint256 initialLockTime = _timestampToFloorWeek(block.timestamp) + 26 weeks; | ||
| uint256 permanentDuration = 52 weeks; | ||
|
|
||
| // Create decaying lock for Alice | ||
| _createLockForUser(users.alice, amount, initialLockTime); | ||
|
|
||
| uint256 initialSupply = stakeWeight.totalSupply(); | ||
|
|
||
| // Advance time so lock is partially decayed | ||
| vm.warp(block.timestamp + 10 weeks); | ||
|
|
||
| uint256 decayedSupply = stakeWeight.totalSupply(); | ||
| assertLt(decayedSupply, initialSupply, "Supply should decay over time"); | ||
|
|
||
| // Get the supply value before conversion to understand the remaining weight | ||
| uint256 remainingWeight = stakeWeight.balanceOf(users.alice); | ||
|
|
||
| // Debug: Check totalSupply before conversion | ||
| uint256 totalSupplyBefore = stakeWeight.totalSupply(); | ||
| console2.log("Total supply before conversion:", totalSupplyBefore); | ||
| console2.log("Remaining decaying weight:", remainingWeight); | ||
|
|
||
| // Convert to permanent | ||
| vm.prank(users.alice); | ||
| stakeWeight.convertToPermanent(permanentDuration); | ||
|
|
||
| // Debug: Check totalSupply immediately after | ||
| uint256 totalSupplyImmediately = stakeWeight.totalSupply(); | ||
| console2.log("Total supply immediately after conversion:", totalSupplyImmediately); | ||
| console2.log("Expected permanent weight:", _calculatePermanentBias(amount, permanentDuration)); | ||
| console2.log("Difference (old decaying still there?):", totalSupplyImmediately - _calculatePermanentBias(amount, permanentDuration)); | ||
|
|
||
| // Calculate expected permanent weight | ||
| uint256 permanentWeight = _calculatePermanentBias(amount, permanentDuration); | ||
|
|
||
| // Check user's balance after conversion | ||
| uint256 userBalanceAfter = stakeWeight.balanceOf(users.alice); | ||
| assertEq(userBalanceAfter, permanentWeight, "User balance should match permanent weight"); | ||
|
|
||
| // After conversion, the totalSupply should equal the user's permanent balance | ||
| // since they are the only user in the system | ||
| uint256 totalSupplyAfter = stakeWeight.totalSupply(); | ||
| assertEq( | ||
| totalSupplyAfter, | ||
| userBalanceAfter, | ||
| "Total supply should match the only user's balance" | ||
| ); | ||
|
|
||
| // Verify the supply stays constant over time (doesn't decay) | ||
| uint256 supplyAfterConversion = totalSupplyAfter; | ||
| vm.warp(block.timestamp + 20 weeks); | ||
|
|
||
| assertEq(stakeWeight.totalSupply(), supplyAfterConversion, "Permanent supply should not decay"); | ||
|
|
||
| // Additional verification: balance should also remain constant | ||
| assertEq(stakeWeight.balanceOf(users.alice), permanentWeight, "User balance should remain constant"); |
There was a problem hiding this comment.
The comment indicates a critical issue with totalSupply calculation after conversion. This appears to be a known issue that could significantly impact reward distribution. This should be addressed before production deployment.
| uint256 amount = 100e18; | |
| uint256 initialLockTime = _timestampToFloorWeek(block.timestamp) + 26 weeks; | |
| uint256 permanentDuration = 52 weeks; | |
| // Create decaying lock for Alice | |
| _createLockForUser(users.alice, amount, initialLockTime); | |
| uint256 initialSupply = stakeWeight.totalSupply(); | |
| // Advance time so lock is partially decayed | |
| vm.warp(block.timestamp + 10 weeks); | |
| uint256 decayedSupply = stakeWeight.totalSupply(); | |
| assertLt(decayedSupply, initialSupply, "Supply should decay over time"); | |
| // Get the supply value before conversion to understand the remaining weight | |
| uint256 remainingWeight = stakeWeight.balanceOf(users.alice); | |
| // Debug: Check totalSupply before conversion | |
| uint256 totalSupplyBefore = stakeWeight.totalSupply(); | |
| console2.log("Total supply before conversion:", totalSupplyBefore); | |
| console2.log("Remaining decaying weight:", remainingWeight); | |
| // Convert to permanent | |
| vm.prank(users.alice); | |
| stakeWeight.convertToPermanent(permanentDuration); | |
| // Debug: Check totalSupply immediately after | |
| uint256 totalSupplyImmediately = stakeWeight.totalSupply(); | |
| console2.log("Total supply immediately after conversion:", totalSupplyImmediately); | |
| console2.log("Expected permanent weight:", _calculatePermanentBias(amount, permanentDuration)); | |
| console2.log("Difference (old decaying still there?):", totalSupplyImmediately - _calculatePermanentBias(amount, permanentDuration)); | |
| // Calculate expected permanent weight | |
| uint256 permanentWeight = _calculatePermanentBias(amount, permanentDuration); | |
| // Check user's balance after conversion | |
| uint256 userBalanceAfter = stakeWeight.balanceOf(users.alice); | |
| assertEq(userBalanceAfter, permanentWeight, "User balance should match permanent weight"); | |
| // After conversion, the totalSupply should equal the user's permanent balance | |
| // since they are the only user in the system | |
| uint256 totalSupplyAfter = stakeWeight.totalSupply(); | |
| assertEq( | |
| totalSupplyAfter, | |
| userBalanceAfter, | |
| "Total supply should match the only user's balance" | |
| ); | |
| // Verify the supply stays constant over time (doesn't decay) | |
| uint256 supplyAfterConversion = totalSupplyAfter; | |
| vm.warp(block.timestamp + 20 weeks); | |
| assertEq(stakeWeight.totalSupply(), supplyAfterConversion, "Permanent supply should not decay"); | |
| // Additional verification: balance should also remain constant | |
| assertEq(stakeWeight.balanceOf(users.alice), permanentWeight, "User balance should remain constant"); | |
| assert(false, "SKIPPED: Known issue with totalSupply after conversion. See comments in test for details."); | |
| // The rest of the test is intentionally disabled until the underlying issue is fixed. | |
| // uint256 amount = 100e18; | |
| // uint256 initialLockTime = _timestampToFloorWeek(block.timestamp) + 26 weeks; | |
| // uint256 permanentDuration = 52 weeks; | |
| // _createLockForUser(users.alice, amount, initialLockTime); | |
| // uint256 initialSupply = stakeWeight.totalSupply(); | |
| // vm.warp(block.timestamp + 10 weeks); | |
| // uint256 decayedSupply = stakeWeight.totalSupply(); | |
| // assertLt(decayedSupply, initialSupply, "Supply should decay over time"); | |
| // uint256 remainingWeight = stakeWeight.balanceOf(users.alice); | |
| // uint256 totalSupplyBefore = stakeWeight.totalSupply(); | |
| // console2.log("Total supply before conversion:", totalSupplyBefore); | |
| // console2.log("Remaining decaying weight:", remainingWeight); | |
| // vm.prank(users.alice); | |
| // stakeWeight.convertToPermanent(permanentDuration); | |
| // uint256 totalSupplyImmediately = stakeWeight.totalSupply(); | |
| // console2.log("Total supply immediately after conversion:", totalSupplyImmediately); | |
| // console2.log("Expected permanent weight:", _calculatePermanentBias(amount, permanentDuration)); | |
| // console2.log("Difference (old decaying still there?):", totalSupplyImmediately - _calculatePermanentBias(amount, permanentDuration)); | |
| // uint256 permanentWeight = _calculatePermanentBias(amount, permanentDuration); | |
| // uint256 userBalanceAfter = stakeWeight.balanceOf(users.alice); | |
| // assertEq(userBalanceAfter, permanentWeight, "User balance should match permanent weight"); | |
| // uint256 totalSupplyAfter = stakeWeight.totalSupply(); | |
| // assertEq( | |
| // totalSupplyAfter, | |
| // userBalanceAfter, | |
| // "Total supply should match the only user's balance" | |
| // ); | |
| // uint256 supplyAfterConversion = totalSupplyAfter; | |
| // vm.warp(block.timestamp + 20 weeks); | |
| // assertEq(stakeWeight.totalSupply(), supplyAfterConversion, "Permanent supply should not decay"); | |
| // assertEq(stakeWeight.balanceOf(users.alice), permanentWeight, "User balance should remain constant"); |
| // CRITICAL: Total time advancement must stay well under 52 weeks to avoid exceeding | ||
| // StakingRewardDistributor's checkpoint loop limit. | ||
| // Conservative approach: Only advance 20 weeks to leave ample room for handler time jumps | ||
| uint256 maxTimeAdvance = 20 weeks; // Very conservative limit, half of 40 weeks |
There was a problem hiding this comment.
The hardcoded checkpoint loop limit suggests a potential gas optimization issue. Consider documenting the exact loop limit and potentially making it configurable or implementing a more gas-efficient checkpoint mechanism.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 68 out of 68 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
- Create 3 core documents addressing auditor's specific requests: * CODE_EVOLUTION.md: Code provenance (30% forked, 20% Velodrome, 50% original) * MATH_AND_DESIGN.md: Mathematical formulas with implementation references * SECURITY_CONSIDERATIONS.md: Critical upgrade safety mechanisms - Update AUDIT_SCOPE_P3.md to complement (not duplicate) technical docs - Streamline AUDIT_SUMMARY.md to focus only on essential documents - Add Mermaid diagrams throughout for better visualization - Properly credit Velodrome for permanent staking innovation - Remove low-signal files and redundant documentation This focused approach directly addresses the two auditor requests: 1. Mathematical formulas underlying StakeWeight.sol 2. Clear attribution of forked vs original code 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…edTokenStaker upgradeable - Replace Ownable with AccessControl in StakingRewardDistributor - Add DEFAULT_ADMIN_ROLE for timelock and REWARD_MANAGER_ROLE for treasury - Implement migrateToAccessControl() for safe live contract upgrade - Update all access control checks to use role-based permissions - Make LockedTokenStaker upgradeable - Convert to Initializable pattern with proxy support - Add unique identifier requirement to prevent salt collisions - Support multiple instances (Reown, WalletConnect, Backers) - Enhance Pauser contract - Add isStakingRewardDistributorPaused flag and controls - Integrate with StakingRewardDistributor pause checks - Maintain role separation (PAUSER_ROLE vs UNPAUSER_ROLE) - Add comprehensive fork tests - Test complete upgrade path with 7-day timelock delays - Verify role assignments and permissions - Test pause/unpause flow with Manager Timelock - Include StakeWeight upgrade for full integration testing - Update deployment scripts and helpers - Fix deployment count from 14 to 15 contracts - Add proxy helpers for LockedTokenStaker - Support unique identifiers for multiple instances 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Extract large data structures into helper functions to reduce stack usage - Add helper functions: _buildUnlockSchedule(), _buildAllocation(), _getDeployments(), _buildVesterCalldata() - Add _callVesterWithdraw() wrapper to minimize stack depth during vester calls - Split test into two separate functions for regular and permanent locks - Tests now successfully demonstrate the critical bug where permanent locks bypass vesting protection The refactoring moves 74+ array assignments and complex struct creation out of the main test functions, allowing the tests to compile and run within EVM's 16 stack slot limit. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Remove deprecated feed() function from StakingRewardDistributor - Fix modifier ordering (nonReentrant first) for reentrancy protection - Change pragma from ^0.8.25 to 0.8.25 for specificity - Remove unused errors (TooManyUsers, InvalidUser, Unauthorized) - Change initialize visibility from public to external - Add constants for magic numbers (MAX_CHECKPOINT_ITERATIONS, MAX_REWARD_ITERATIONS) - Add missing events (RewardInjected, TotalSupplyCheckpointed) - Update tests to use injectReward() instead of feed() Security improvements based on Aderyn static analysis. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Updated CI workflow to only check contract sizes for src/ directory - Excluded src/utils/ directory containing external NttManager contract - Applied forge fmt to fix code formatting across the codebase 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
13da9bc to
299e7ba
Compare
…ing vesting allocation
…._checkpointToken
Summary
This PR implements the P3 permanent staking redesign, introducing non-decaying staking positions inspired by Velodrome alongside the existing ve-style locks. This critical upgrade enhances staking flexibility while maintaining
complete backward compatibility for existing positions.
Key Implementation Details
Core Features
Technical Improvements
Code Attribution
This implementation builds upon established DeFi patterns:
Recent Security Improvements
feed()function to reduce code surface areanonReentrantmodifier ordering for proper reentrancy protection^0.8.25to0.8.25for deterministic compilationStorage Safety (ERC-7201)