Add StakeDAO Escrow#139
Conversation
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
N E M E S I S — Verified Findings: StakeDaoEscrow.sol (current branch)Re-run confirmation — 2026-06-03Independently re-ran the full Nemesis iterative loop (Feynman ↔ State, 4 passes, converged) against the current
Scope
Diff vs prior auditsTwo prior audit files exist in this directory:
What still applies from prior audits and is re-verified against the current code:
What is new in this audit (introduced by, or only surfaced after, the recent fixes):
Phase 0 Recon — Attacker Hit ListAttack goals:
Novel surface vs
Phase 1 Cross-Reference (Nemesis Map)
Coupled state pairs
Verification Summary
No CRITICAL or HIGH findings in the current code. The previously-CRITICAL NM-001 — Conditional bad debt:
|
| Candidate | Verdict | Reason |
|---|---|---|
Front-run initialize() on a freshly-created proxy |
False positive | Market.getEscrow() (Market.sol:253-259) deploys via CREATE2 and calls initialize atomically in the same tx — no mempool window. |
Front-run initialize() on the implementation contract |
Non-issue | Impl contract holds no funds. Attacker would also need a contract whose collateral() returns rewardVault.asset() to pass L77 — and even then, the impl proxy has no shares/tokens to drain. |
Reentry from IMarket(msg.sender).collateral() at L77 (after market = msg.sender at L76) |
False positive | Reentry into initialize is blocked by L74 (market != 0). Reentry into pay/onDeposit/balance reverts because token is still address(0) at this point. Reentry into claim/setClaimer reverts because beneficiary is still address(0) and the caller is not allowlisted. No state mutation is observable before the call, and the call is fail-closed. |
Market.collateral() triggers arbitrary code during initialize |
False positive | Market.collateral is IERC20 public immutable (Market.sol:41) — the auto-generated getter is a pure SLOAD of an immutable bytecode constant, no fallback or hook. |
claim(tokens) with tokens.length != amounts.length returned by vault |
False positive | The escrow emits amounts directly from rewardVault.claim; mismatched length is not exploitable, only emits noisy events. |
| Donation of tokens or shares directly to the escrow inflates collateral | Working as intended | Donations increase the borrower's effective collateral. Net positive to the borrower; the donor pays. Cannot be exploited against the borrower. |
onDeposit callable by anyone enables griefing |
Intentional | Documented in NatSpec ("should remain callable by anyone"); matches peer design. Direct transfers become a donation to the beneficiary. |
pay() ignores rewardVault.withdraw return value |
False positive | ERC4626 withdraw(assets, receiver, owner) guarantees assets of the underlying are delivered to receiver or the call reverts. The returned shares value is informational. The subsequent safeTransfer enforces final solvency. |
accountant.claim(_gaugeArray(), new bytes[](0), to) with mismatched array lengths |
False positive | Empirically verified by fork test testAllowlistedClaimerReceivesCrvBaseRewardToken against the live mainnet accountant (block 25_076_112) — call succeeds and CRV reaches the receiver. The deployed Stake DAO accountant accepts the empty harvestData array. |
setClaimer race — beneficiary revokes after auto-claimer tx is mined |
Not a bug | Standard allowlist semantics. |
gaugeArray could be larger than 1 |
Working as intended | Single-vault design — each escrow corresponds to one Stake DAO vault with one gauge. |
Multi-Transaction Adversarial Sequences (Phase 5)
For each TRUE POSITIVE, the minimal trigger sequence:
NM-001:
Market.deposit(100)— escrow stakes 100 in vault, shares = 100,previewRedeem(100) = 100.Market.borrow(80)— protocol believes collateral = 100, debt = 80.- Vault enters paused or capped state (external action — not attacker-controlled, but possible).
previewRedeem(100) == 100still, butvault.withdraw(N)reverts for any positiveN. - Position becomes under-collateralized due to price move;
Market.liquidate(user, debt)→escrow.pay(liquidator, ~88)→vault.withdraw(88)reverts → liquidation reverts. - Bad debt accrues until vault unpauses.
NM-002 (alone): No adversarial sequence — only an information-disclosure / observability gap. Rewards are recoverable on next claim.
NM-002 × NM-004 (combined):
- Many users open positions; each gets an escrow with
gauge = G_oldcached. - Stake DAO governance migrates the vault to
G_new. - Users call
claim→try accountant.claim([G_old], ...)reverts (gauge stale) → catch swallows →rewardVault.claim(extras, to)proceeds → event emitted with no base reward delivered. - Users see "Claim" event with no CRV/BAL transfer and have no way to recover via the escrow.
Feedback-Loop Discoveries
- NM-002 was a cross-feed finding. Pass 1 Feynman did not flag the try/catch because the catch's purpose is plausible (tolerate "no pending rewards" reverts). Pass 2 State did not flag it because no internal escrow state is corrupted by a swallow. Only Pass 3 Feynman targeted re-interrogation — asking "WHY does the dev assume every revert from
accountant.claimis benign?" — surfaced the silent-failure surface. Pass 4 State then linked it to NM-004 (the stale-gauge case where the catch becomes permanent loss). - NM-004 emerged from Pass 3 when Feynman asked "why are
gaugeandaccountantcached at all, given they're already proxied viarewardVault?". Pass 4 State then traced the cache as a frozen coupled state with no reconciliation path.
These two findings are the iterative loop's value-add over running either auditor alone.
False Positives Eliminated
See "Findings explicitly considered and dismissed" above (10 items). The most important are:
- Implementation-contract initialization hijack (not exploitable — impl holds no funds).
- Reentry during
IMarket(msg.sender).collateral()(every callable path inside the escrow is fail-closed at this point in initialize). - Length-mismatched
harvestDataarray onaccountant.claim(empirically verified by fork test on real mainnet vault).
Summary
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| HIGH | 0 |
| MEDIUM | 0–1 (NM-001, conditional on deployed vault) |
| LOW | 1 (NM-002) |
| INFO | 5 (NM-003, NM-004, NM-005, NM-006, NM-007) |
The two CRITICAL/HIGH findings present in the prior audit reports for older versions of this contract (initialize always-revert; base-reward claim using msg.sender) have both been fixed in the current branch. The current code's only material concern is the conditional bad-debt risk from previewRedeem ↔ withdraw asymmetry (NM-001) — same trust assumption as any vault-backed escrow, mitigable by clamping with maxWithdraw. NM-002 + NM-004 are paired operational concerns about silent failure if Stake DAO ever reconfigures the vault's gauge or accountant; worth adding an event and/or reading gauge dynamically to eliminate the silent-loss case.
|
| import "openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol"; | ||
| import {IMarket} from "src/interfaces/IMarket.sol"; | ||
|
|
||
| interface IRewardVault is IERC20 { |
There was a problem hiding this comment.
Please provide reference contract address for interface review
| function gauge() external view returns (address); | ||
| } | ||
|
|
||
| interface IAccountant { |
There was a problem hiding this comment.
Please provide reference contract address for interface review
Escrow interfacing with the StakeDAO rewardVault model