Skip to content

feat: added first iteration of super vault manager contract [SV-1580] #300

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.

Sign up for GitHub

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

Open
supervaulter wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

feat: added first iteration of super vault manager contract [SV-1580] #300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
216 changes: 216 additions & 0 deletions specs/security-reports/2026-03-17-session-key-manager.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,216 @@
# Security Analysis Report

## Metadata
- **Target:** `src/SuperVault/SessionKeyManager.sol`, `src/interfaces/SuperVault/ISessionKeyManager.sol`
- **Mode:** review
- **Date:** 2026-03-17
- **Contract Types Detected:** General (Access Control / Delegation)
- **Files Analyzed:** 2
- **Vulnerability Database:** vulnerabilities.md (36 sections, 300+ patterns, 175+ exploits)

## Summary
| Severity | Count | Blocks Merge |
|----------|-------|-------------|
| P0 Critical | 0 | Yes |
| P1 High | 0 | Yes |
| P2 Medium | 5 | No |
| P3 Low | 11 | No |

## Verdict
**PASS** - No P0 or P1 findings. Safe to proceed.

---

## P0 Findings (Critical - Must Fix)
None found.

## P1 Findings (High - Must Fix)
None found.

---

## P2 Findings (Medium - Should Fix)

### [P2-1] Bare `require(success)` in rescueETH Instead of Custom Error
- **File:** `src/SuperVault/SessionKeyManager.sol:174`
- **SWC:** N/A
- **Category:** Logic
- **Description:** The `rescueETH` function uses `require(success)` which produces a generic revert without a reason string, inconsistent with the contract's custom error pattern used everywhere else.
- **Vulnerable Code:**
```solidity
(bool success,) = to.call{ value: balance }("");
require(success);
```
- **Secure Pattern:**
```solidity
(bool success,) = to.call{ value: balance }("");
if (!success) revert ETH_TRANSFER_FAILED();
```
- **Reference:** coding-rules.md (custom errors over require strings)

### [P2-2] Redundant `_getAggregator()` External Calls in Batch Loops
- **File:** `src/SuperVault/SessionKeyManager.sol:72-75, 89-92`
- **SWC:** N/A
- **Category:** Gas
- **Description:** `grantSessionKeysBatch` and `revokeSessionKeysBatch` call `_validatePrimaryManager()` in each loop iteration, which calls `_getAggregator()` each time — making 2 external calls per iteration (SuperGovernor + Aggregator). The aggregator address won't change mid-transaction.
- **Vulnerable Code:**
```solidity
for (uint256 i; i < len; ++i) {
_validatePrimaryManager(strategies[i]); // 2 external calls per iteration
_grantSessionKey(strategies[i], sessionKeys[i], expiries[i]);
}
```
- **Secure Pattern:**
```solidity
ISuperVaultAggregator aggregator = _getAggregator();
for (uint256 i; i < len; ++i) {
if (!aggregator.isMainManager(msg.sender, strategies[i])) {
revert CALLER_NOT_PRIMARY_MANAGER();
}
_grantSessionKey(strategies[i], sessionKeys[i], expiries[i]);
}
```
- **Reference:** vulnerabilities.md Section 13 (Gas Optimization)

### [P2-3] ETH Trapping in Forwarding Contract
- **File:** `src/SuperVault/SessionKeyManager.sol:100-103, 179`
- **SWC:** N/A
- **Category:** Logic
- **Description:** `executeHooks` forwards `msg.value` to the strategy, but if the strategy refunds ETH to `msg.sender` (which is the SessionKeyManager, not the session key holder), the ETH gets trapped. The `receive()` function accepts it, but only the admin can rescue it via `rescueETH`.
- **Exploit Scenario:** A session key holder calls `executeHooks{value: 1 ether}(...)`. The strategy partially uses the ETH and refunds 0.5 ETH to `msg.sender` (SessionKeyManager). The 0.5 ETH is now stuck until admin calls `rescueETH`.
- **Secure Pattern:** This is an accepted trade-off given the `rescueETH` function exists. Consider documenting this behavior and ensuring `rescueETH` is called periodically, or forwarding refunds back to `msg.sender` if the strategy supports a recipient parameter.
- **Reference:** EVM Security Research (ETH forwarding patterns)

### [P2-4] Session Key Resurrection on Manager Reinstatement
- **File:** `src/SuperVault/SessionKeyManager.sol:193-200`
- **SWC:** N/A
- **Category:** Logic
- **Description:** If a primary manager is replaced and then reinstated (e.g., `A → B → A`), all session keys originally granted by `A` that haven't expired become valid again without `A` re-granting them. The `grantedByManager` check passes again.
- **Exploit Scenario:** Manager A grants a session key with 30-day expiry. Manager is changed to B (invalidating the key). Manager is changed back to A. The old session key is now valid again for the remaining duration — even if A didn't intend to re-authorize it.
- **Secure Pattern:** Add an epoch/nonce counter per strategy that increments on every manager change, and store the epoch at grant time. This would require aggregator changes. Alternatively, document as a known behavior and recommend explicit revocation before manager reinstatement.
- **Reference:** EVM Security Research (stale authorization patterns)

### [P2-5] TOCTOU Race on Session Key Validation During Manager Change
- **File:** `src/SuperVault/SessionKeyManager.sol:193-200`
- **SWC:** N/A
- **Category:** Logic
- **Description:** A session key holder's transaction can be front-run by a `executeChangePrimaryManager` call, causing the session key to become invalid mid-flight. This is a time-of-check-to-time-of-use race condition on the manager state. However, the impact is limited — the session key holder's tx simply reverts with `PRIMARY_MANAGER_CHANGED`.
- **Secure Pattern:** This is an inherent trade-off of the design (checking live state). The behavior is correct — invalid keys should revert. No fix needed, but document the race condition.
- **Reference:** EVM Security Research (TOCTOU patterns)

---

## P3 Findings (Low - Consider Fixing)

### [P3-1] `memory` Instead of `calldata` for fulfillCancelRedeemRequests Controllers
- **File:** `src/SuperVault/SessionKeyManager.sol:106`
- **SWC:** N/A
- **Category:** Gas
- **Description:** The `controllers` parameter uses `memory` instead of `calldata`, causing unnecessary memory copy. This matches the upstream interface signature so it's constrained by `ISuperVaultStrategy`.
- **Current Code:** `function fulfillCancelRedeemRequests(address strategy, address[] memory controllers)`
- **Note:** Cannot change without also changing the interface. Low priority.

### [P3-2] Missing Zero-Balance Check in rescueETH
- **File:** `src/SuperVault/SessionKeyManager.sol:170-176`
- **Category:** Gas
- **Description:** `rescueETH` will execute a zero-value ETH transfer if the contract has no balance, wasting gas. Consider adding `if (balance == 0) revert NO_ETH_TO_RESCUE()`.

### [P3-3] Missing Empty Array Check in Batch Functions
- **File:** `src/SuperVault/SessionKeyManager.sol:62-76, 85-93`
- **Category:** Logic
- **Description:** `grantSessionKeysBatch` and `revokeSessionKeysBatch` allow empty arrays, executing a no-op loop. Consider adding `if (len == 0) revert EMPTY_ARRAY()`.

### [P3-4] No Strategy Address Validation
- **File:** `src/SuperVault/SessionKeyManager.sol:56-59, 100-103`
- **Category:** Logic
- **Description:** No check that `strategy != address(0)` on grant or forwarding calls. However, `_validatePrimaryManager` and `_validateSessionKey` will revert for `address(0)` since `isMainManager` would return false, providing implicit protection.

### [P3-5] Block Timestamp Manipulation
- **File:** `src/SuperVault/SessionKeyManager.sol:148, 196, 206`
- **Category:** Logic
- **Description:** Expiry checks use `block.timestamp` which validators can manipulate by ~15 seconds. Given that session keys have hours/days-long expiries, this is negligible.

### [P3-6] Unbounded Batch Operations
- **File:** `src/SuperVault/SessionKeyManager.sol:62-76, 85-93`
- **Category:** DoS
- **Description:** No upper bound on batch array length. Extremely large arrays could hit block gas limits. In practice, callers are primary managers who won't DOS themselves.

### [P3-7] msg.value Forwarding in Batched Context
- **File:** `src/SuperVault/SessionKeyManager.sol:100-103`
- **Category:** Logic
- **Description:** If `executeHooks` were called multiple times in a multicall pattern, `msg.value` would be reused. However, SessionKeyManager doesn't have a multicall function, so this is not exploitable.

### [P3-8] Dynamic Registry Resolution Trust Dependency
- **File:** `src/SuperVault/SessionKeyManager.sol:220-222`
- **Category:** Logic
- **Description:** `_getAggregator()` resolves the aggregator dynamically via SuperGovernor. If SuperGovernor is compromised, the aggregator address could be changed to a malicious contract. This is an accepted trust assumption — SuperGovernor is a trusted governance contract.

### [P3-9] AccessControl Without DefaultAdminRules
- **File:** `src/SuperVault/SessionKeyManager.sol:14`
- **Category:** Access Control
- **Description:** Uses `AccessControl` instead of `AccessControlDefaultAdminRules` which adds a 2-step admin transfer with timelock. Given this is a single-role contract (only DEFAULT_ADMIN_ROLE for rescueETH), the additional complexity is likely unnecessary.

### [P3-10] SessionKeyData Struct Defined in Implementation, Not Interface
- **File:** `src/SuperVault/SessionKeyManager.sol:19-22`
- **Category:** Code Quality
- **Description:** The `SessionKeyData` struct is defined in the implementation contract rather than the interface. Since it's only used internally (private mapping), this is acceptable.

### [P3-11] Cross-Strategy Reentrancy via executeHooks
- **File:** `src/SuperVault/SessionKeyManager.sol:100-103`
- **Category:** Reentrancy
- **Description:** A malicious hook could re-enter `executeHooks` for a different strategy during execution. However, the session key validation is read-only (view) and no state is modified before the external call, so reentrancy cannot manipulate SessionKeyManager state. The strategy itself has its own reentrancy guards.

---

## Attack Surface Summary

### External Entry Points
| Function | Access | Modifies State |
|----------|--------|---------------|
| `grantSessionKey` | Primary manager | Yes (storage) |
| `grantSessionKeysBatch` | Primary manager | Yes (storage) |
| `revokeSessionKey` | Primary manager | Yes (storage) |
| `revokeSessionKeysBatch` | Primary manager | Yes (storage) |
| `executeHooks` | Session key holder | No (forwards) |
| `fulfillCancelRedeemRequests` | Session key holder | No (forwards) |
| `fulfillRedeemRequests` | Session key holder | No (forwards) |
| `skimPerformanceFee` | Session key holder | No (forwards) |
| `pauseStrategy` | Session key holder | No (forwards) |
| `unpauseStrategy` | Session key holder | No (forwards) |
| `rescueETH` | DEFAULT_ADMIN_ROLE | Yes (ETH transfer) |
| `receive` | Anyone | Yes (ETH receive) |

### Value Transfer Points
- `executeHooks`: Forwards `msg.value` to strategy
- `rescueETH`: Sends contract ETH balance to recipient
- `receive()`: Accepts ETH (for refunds from strategies)

### Cross-Contract Interactions
- `ISuperGovernor.getAddress()` — resolves aggregator address
- `ISuperGovernor.SUPER_VAULT_AGGREGATOR()` — gets aggregator key
- `ISuperVaultAggregator.isMainManager()` — validates primary manager
- `ISuperVaultAggregator.pauseStrategy()` / `unpauseStrategy()` — forwards pause/unpause
- `ISuperVaultStrategy.executeHooks()` — forwards hook execution
- `ISuperVaultStrategy.fulfillCancelRedeemRequests()` — forwards cancel requests
- `ISuperVaultStrategy.fulfillRedeemRequests()` — forwards redeem fulfillment
- `ISuperVaultStrategy.skimPerformanceFee()` — forwards fee skimming

### Trust Assumptions
- SuperGovernor is trusted and not compromised
- Aggregator resolved via SuperGovernor returns correct address
- Primary managers are trusted actors who grant keys responsibly
- Strategy contracts are trusted (deployed by the protocol)

## Coding Standards Findings
- Custom errors used consistently (except `require(success)` in rescueETH — P2-1)
- Events emitted for all state changes
- NatSpec documentation complete via `@inheritdoc`
- Import organization follows project conventions
- Naming conventions followed (SCREAMING_SNAKE for errors, camelCase for functions)
- Checks-Effects-Interactions pattern followed (validation before external calls)

## Security Knowledge Sources
- **vulnerabilities.md sections referenced:** 1 (Reentrancy), 2 (Access Control), 3 (Arithmetic), 8 (Unchecked Returns), 9 (Encoding), 13 (Gas), 15 (Code Quality), 36 (Pre-PR Checklist)
- **evmresearch.io patterns checked:** Delegation patterns, session key implementations, ETH forwarding, TOCTOU in authorization
- **Coding rules validated:** Custom errors, events, NatSpec, naming, imports, CEI pattern
- **Historical exploits cross-referenced:** Access control bypasses, stale authorization resurrection, ETH trapping in proxy/forwarder contracts
Loading
Loading