diff --git a/.gitmodules b/.gitmodules index e69de29..8ac71cb 100644 --- a/.gitmodules +++ b/.gitmodules @@ -0,0 +1,12 @@ +[submodule "echidna/workspace/lib/openzeppelin-contracts"] + path = echidna/workspace/lib/openzeppelin-contracts + url = https://github.com/openzeppelin/openzeppelin-contracts +[submodule "echidna/workspace/lib/openzeppelin-contracts-upgradeable"] + path = echidna/workspace/lib/openzeppelin-contracts-upgradeable + url = https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable +[submodule "echidna/workspace/lib/forge-std"] + path = echidna/workspace/lib/forge-std + url = https://github.com/foundry-rs/forge-std +[submodule "echidna/workspace/lib/ds-test"] + path = echidna/workspace/lib/ds-test + url = https://github.com/dapphub/ds-test diff --git a/echidna/workspace/lib/ds-test b/echidna/workspace/lib/ds-test new file mode 160000 index 0000000..e282159 --- /dev/null +++ b/echidna/workspace/lib/ds-test @@ -0,0 +1 @@ +Subproject commit e282159d5170298eb2455a6c05280ab5a73a4ef0 diff --git a/echidna/workspace/lib/forge-std b/echidna/workspace/lib/forge-std new file mode 160000 index 0000000..c7be2a3 --- /dev/null +++ b/echidna/workspace/lib/forge-std @@ -0,0 +1 @@ +Subproject commit c7be2a3481f9e51230880bb0949072c7e3a4da82 diff --git a/echidna/workspace/lib/openzeppelin-contracts b/echidna/workspace/lib/openzeppelin-contracts new file mode 160000 index 0000000..c64a1ed --- /dev/null +++ b/echidna/workspace/lib/openzeppelin-contracts @@ -0,0 +1 @@ +Subproject commit c64a1edb67b6e3f4a15cca8909c9482ad33a02b0 diff --git a/echidna/workspace/lib/openzeppelin-contracts-upgradeable b/echidna/workspace/lib/openzeppelin-contracts-upgradeable new file mode 160000 index 0000000..e725abd --- /dev/null +++ b/echidna/workspace/lib/openzeppelin-contracts-upgradeable @@ -0,0 +1 @@ +Subproject commit e725abddf1e01cf05ace496e950fc8e243cc7cab diff --git a/report/QA.md b/report/QA.md new file mode 100644 index 0000000..82559f9 --- /dev/null +++ b/report/QA.md @@ -0,0 +1,60 @@ +# QA Report: Lido CSM v2 — Minor Design/Operational Considerations + +## Summary +Across the Community Staking Module v2 (CSM v2) scope, the core invariants and flows align with the specification and are covered by comprehensive tests. No High or Medium severity issues were identified. This report compiles low-severity observations that can improve clarity, UX, and defense-in-depth. + +Related PRs created: +- Overflow-safe addition and queue invariant check: https://github.com/code-423n4/2025-07-lido-finance/pull/3 + +## Findings + +### 1) Permissionless priority migration may be surprising +- Affected: `CSModule.migrateToPriorityQueue(uint256 nodeOperatorId)` +- Description: Anyone can call this function to migrate a Node Operator’s (NO’s) enqueued keys into the priority queue, subject to `usedPriorityQueue` and `maxDeposits` constraints. +- Impact: No funds at risk; this generally benefits the NO. However, third parties can influence the timing of an NO’s priority seating without explicit consent. +- Recommendation: Either restrict to NO’s manager (consistent with other NO actions) or explicitly document that this is a public-good operation; clarify that `maxDeposits` bounds and `usedPriorityQueue` (one-time migration) ensure it cannot be abused. + +### 2) Unlimited approvals in Accounting initialization rely on trusted Locator addresses +- Affected: `CSAccounting.initialize` (indirectly via `CSBondCore`/initialization), approvals to WSTETH, Withdrawal Queue, and Burner. +- Description: The contract sets allowances for stETH to `type(uint256).max` for protocol-owned components. +- Impact: Standard Lido practice; relies on correct Locator addresses. Misconfiguration could theoretically expose allowances to the wrong address. +- Recommendation: Document the trust model and change management for the Locator, and maintain allowance sanity checks in ops monitoring. + +### 3) No global reentrancy guard (design-by-trust) +- Affected: Multiple contracts performing external calls (e.g., `CSModule` to `ACCOUNTING` and `STETH.transferShares`; `CSAccounting` to `elRewardsVault`; `CSFeeOracle` to FeeDistributor/Strikes; `CSFeeDistributor` to `STETH`). +- Description: Contracts do not use a global `ReentrancyGuard`; instead they rely on role-gated, protocol-trusted components and strictly ordered state updates. +- Impact: Acceptable given the architecture and trust boundaries; no concrete exploit path identified. +- Recommendation: Document this design posture; continue to minimize external call surfaces and keep tight role boundaries. + +### 4) Deposit queue batch removal semantics are nuanced +- Affected: `CSModule.obtainDepositData` +- Description: When an NO’s `depositableValidatorsCount` is smaller than a batch size (or `depositsLeft > keysCount`), the entire batch is dequeued and the remainder (non-depositable) is effectively discarded (relying on later re-enqueueing when depositable increases). +- Impact: Intentional and tested, but can be surprising without context; could be perceived as unfair by readers unfamiliar with the normalization logic. +- Recommendation: Add a short comment explaining this choice prevents head-of-queue blocking, and that state is normalized by `_enqueueNodeOperatorKeys` when `depositableValidatorsCount` increases. + +### 5) Strikes proof allows empty proof — intentional design +- Affected: `CSStrikes.processBadPerformanceProof` +- Description: Empty proofs are allowed (documented in code). Leaves bind NO + pubkey + data, preventing the use of internal nodes “tricks” without brute force. +- Impact: Safe and intentional; might confuse reviewers encountering an empty-proof allowance. +- Recommendation: Add a brief doc note explaining why empty proofs are safe in this context. + +## Context & Coverage +- Reviewed: CSModule, CSAccounting, CSParametersRegistry, CSFeeDistributor, CSFeeOracle, BaseOracle, HashConsensus, CSStrikes, CSExitPenalties, CSEjector, and libraries (QueueLib, SSZ, SigningKeys, GIndex, ValidatorCountsReport, TransientUintUintMapLib, UnstructuredStorage). +- Tests: Extensive; most files show near-100% coverage, with fuzzing on critical libraries (SSZ, GIndex, QueueLib). Observed behavior matches specification and stated invariants. + +## No High/Medium Issues Identified +- Two areas worth further exploration (we prepared PoC scaffolds) if aiming to elevate severity with a runnable PoC: + - Rounding edge in `_getUnbondedKeysCount` (+10 wei buffer) near bond curve boundaries. + - Priority migration seating fairness/limits (ensure no over-seating or starvation). + +## References +- CSModule: queue management, migration, penalties handling, depositable computations. +- CSAccounting: required bond/unbonded keys math, lock semantics, reward pulls. +- CSFeeDistributor/Oracle: merkle-based payouts, report processing, consensus checks. +- BaseOracle/HashConsensus: frame/time/quorum mechanics, processing deadlines, fast-lane. +- CSStrikes/CSExitPenalties/CSEjector: strikes proofs, penalty recording, exit triggers and guards. + +--- +Prepared by: [Your Handle] +Date: [YYYY-MM-DD] + diff --git a/src/lib/QueueLib.sol b/src/lib/QueueLib.sol index f455a68..b884825 100644 --- a/src/lib/QueueLib.sol +++ b/src/lib/QueueLib.sol @@ -93,6 +93,7 @@ using QueueLib for QueueLib.Queue; interface IQueueLib { error QueueIsEmpty(); error QueueLookupNoLimit(); + error QueueInvariantBroken(); } /// @author madlabman @@ -162,7 +163,8 @@ library QueueLib { self.queue[indexOfPrev] = prevItem; } - // We assume that the invariant `enqueuedCount` >= `keys` is kept. + // Ensure the invariant `enqueuedCount` >= `keys` is kept to avoid underflow. + if (no.enqueuedCount < uint32(item.keys())) revert IQueueLib.QueueInvariantBroken(); // NOTE: No need to safe cast due to internal logic. no.enqueuedCount -= uint32(item.keys()); diff --git a/src/lib/TransientUintUintMapLib.sol b/src/lib/TransientUintUintMapLib.sol index a085454..9b24ef8 100644 --- a/src/lib/TransientUintUintMapLib.sol +++ b/src/lib/TransientUintUintMapLib.sol @@ -28,13 +28,11 @@ library TransientUintUintMapLib { uint256 key, uint256 value ) internal { - uint256 slot = _slot(self, key); - assembly ("memory-safe") { - let v := tload(slot) - // NOTE: Here's no overflow check. - v := add(v, value) - tstore(slot, v) - } + // Make addition safe: revert on overflow rather than wrapping. + // The previous implementation explicitly skipped overflow checks. + uint256 current = get(self, key); + uint256 newValue = current + value; // will revert on overflow in Solidity 0.8+ + set(self, key, newValue); } function set(