Skip to content

Comments

feat: duration vaults#1732

Open
eigenmikem wants to merge 8 commits intomainfrom
release-dev/duration-vaults
Open

feat: duration vaults#1732
eigenmikem wants to merge 8 commits intomainfrom
release-dev/duration-vaults

Conversation

@eigenmikem
Copy link
Contributor

Motivation:

Explain here the context, and why you're making that change. What is the problem you're trying to solve.

Modifications:

Describe the modifications you've done.

Result:

After your change, what will change.

eigenmikem and others added 7 commits February 13, 2026 12:37
**Motivation:**

Financial AVS's need stake constraints (duration, quantity) in order to
use EigenLayer.

**Modifications:**

A new type of strategy, `DurationVaultStrategy`, that enforces delegate
stake caps and duration constraints, and registers as an operator. Also
includes modifications to the `StrategyManager` and default strategies,
including hooks before shares are added/removed, to accommodate
alternative stake models.

**Result:**

Financial AVS's will have the tools to build on Eigenlayer more easily.
<!-- 
    🚨 ATTENTION! 🚨 
    
This PR template is REQUIRED. PRs not following this format will be
closed without review.
    
    Requirements:
- PR title must follow commit conventions:
https://www.conventionalcommits.org/en/v1.0.0/
- Label your PR with the correct type (e.g., 🐛 Bug, ✨ Enhancement, 🧪
Test, etc.)
    - Provide clear and specific details in each section
-->

**Motivation:**

use call instead of try/catch

**Modifications:**

*Describe the modifications you've done.*

**Result:**

*After your change, what will change.*
**Motivation:**

*DurationVaultStrategy*  documentation

**Modifications:**

- added durationVaultStrategy documantation

**Result:**
**Motivation:**

*Explain here the context, and why you're making that change. What is
the problem you're trying to solve.*

**Modifications:**

*Describe the modifications you've done.*

**Result:**

*After your change, what will change.*
<!-- 
    🚨 ATTENTION! 🚨 
    
This PR template is REQUIRED. PRs not following this format will be
closed without review.
    
    Requirements:
- PR title must follow commit conventions:
https://www.conventionalcommits.org/en/v1.0.0/
- Label your PR with the correct type (e.g., 🐛 Bug, ✨ Enhancement, 🧪
Test, etc.)
    - Provide clear and specific details in each section
-->

**Motivation:**

*Explain here the context, and why you're making that change. What is
the problem you're trying to solve.*

**Modifications:**

*Describe the modifications you've done.*

**Result:**

*After your change, what will change.*
**Motivation:**

Address findings from the Certora security audit of the Duration Vaults implementation.

**Modifications:**

### Medium Severity

- **M-01 Fix**: Added `setRewardsClaimer(address claimer)` to designate a claimer for operator-attributed rewards, ensuring if any rewards accrued during split activation delay  or via a new rewards type  can be claimed
- **M-02 Fix**: Implemented retry mechanism in `markMatured()` and `advanceToWithdrawals()` - both now check if vault is in WITHDRAWALS state and re-attempt operator cleanup (`_deallocateAll()` and `_deregisterFromOperatorSet()`) if needed
- **M-03 Fix**: Added `updateDelegationApprover(address newDelegationApprover)` calling `DelegationManager.modifyOperatorDetails()` for delegation approver updates
- **M-04 Fix**: Same retry mechanism as M-02 allows subsequent calls to complete cleanup if initial call was gas-griefed

### Informational Severity

- **I-02 Fix**: Added `DeallocateAttempted(bool success)` and `DeregisterAttempted(bool success)` events for off-chain monitoring
- **I-03 Fix**: changed to `DepositExceedsMaxPerDeposit()` error for clearer messaging in `beforeAddShares()`
- **I-04 Fix**: Added strategy support check in `lock()` via `AllocationManager.getStrategiesInOperatorSet()`, reverts with `StrategyNotSupportedByOperatorSet()` if unsupported
- **I-05 Fix**: Added `@param arbitrator` NatSpec to VaultConfig struct
- **I-06 Fix**: Changed `operatorSetRegistered()` to query `AllocationManager.isMemberOfOperatorSet()` directly


**Result:**

Improved protocol correctness and security for Duration Vaults.
<!-- 
    🚨 ATTENTION! 🚨 
    
This PR template is REQUIRED. PRs not following this format will be
closed without review.
    
    Requirements:
- PR title must follow commit conventions:
https://www.conventionalcommits.org/en/v1.0.0/
- Label your PR with the correct type (e.g., 🐛 Bug, ✨ Enhancement, 🧪
Test, etc.)
    - Provide clear and specific details in each section
-->

**Motivation:**

*Explain here the context, and why you're making that change. What is
the problem you're trying to solve.*

**Modifications:**

*Describe the modifications you've done.*

**Result:**

*After your change, what will change.*
@claude
Copy link

claude bot commented Feb 23, 2026

Claude encountered an error after 1m 27s —— View job


Code Review: Duration Vaults Feature

Reviewing the duration vaults implementation...

Tasks

  • Read PR description and documentation
  • Review DurationVaultStrategy core logic
  • Review storage layout and upgradability
  • Review StrategyManager hook integration
  • Review StrategyFactory changes
  • Check state transitions and edge cases
  • Review security considerations
  • Assess test coverage
  • Provide comprehensive feedback

@eigenmikem eigenmikem changed the title Release dev/duration vaults feat: duration vaults Feb 23, 2026
require(depositsOpen(), VaultAlreadyLocked());

// Verify this strategy is supported by the operator set before allocating.
IStrategy[] memory strategies = allocationManager.getStrategiesInOperatorSet(_operatorSet);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential Gas Inefficiency: Loop to check strategy support

This loop iterates through all strategies in the operator set to verify the vault is supported. For large operator sets, this could be gas-intensive.

Suggested optimization:
Consider caching the result after first verification, or use a try-catch approach:

// Option 1: Cache verification
bool private _strategySupportVerified;

function lock() external override onlyVaultAdmin {
    require(depositsOpen(), VaultAlreadyLocked());
    
    if (!_strategySupportVerified) {
        IStrategy[] memory strategies = allocationManager.getStrategiesInOperatorSet(_operatorSet);
        bool supported = false;
        for (uint256 i = 0; i < strategies.length; ++i) {
            if (strategies[i] == IStrategy(address(this))) {
                supported = true;
                break;
            }
        }
        require(supported, StrategyNotSupportedByOperatorSet());
        _strategySupportVerified = true;
    }
    // ... rest of function
}

Alternatively, since _allocateFullMagnitude() will revert if the strategy isn't supported, you could remove this check entirely and rely on that revert, adding a custom error handler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants