Skip to content

Add StakeDAO Escrow#139

Open
08xmt wants to merge 9 commits into
devfrom
feat/stake-dao-escrow
Open

Add StakeDAO Escrow#139
08xmt wants to merge 9 commits into
devfrom
feat/stake-dao-escrow

Conversation

@08xmt

@08xmt 08xmt commented May 10, 2026

Copy link
Copy Markdown
Contributor

Escrow interfacing with the StakeDAO rewardVault model

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Code review skipped — your organization's overage spend limit has been reached.

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.

@08xmt

08xmt commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

N E M E S I S — Verified Findings: StakeDaoEscrow.sol (current branch)

Re-run confirmation — 2026-06-03

Independently re-ran the full Nemesis iterative loop (Feynman ↔ State, 4 passes, converged) against the current src/escrows/StakeDaoEscrow.sol, re-reading every consumer in Market.sol (getEscrow/deposit/withdrawInternal/liquidate) and the fork test. All findings below re-verified and still hold; no new Critical/High surfaced. Notes from this pass:

  • NM-001 (previewRedeem ↔ withdraw): Re-checked the ERC4626 rounding direction for a fee-less vault — withdrawing the full previewRedeem(shares) provably needs ≤ shares held, so there is no DoS from rounding alone. The fork test (testMarketWithdrawUnstakesAndPaysLp) shows the deployed vault is 1:1 at block 25_076_112 (previewRedeem is identity there). The real risk is unchanged: vault pause / withdraw-cap / fee / loss-realization between balance() (view) and pay() (action). Stays LOW–MED, conditional on the deployed vault.
  • NM-007 (beneficiary == 0) reachability sharpened: Market.deposit(address user, uint amount) is public with an arbitrary user, so getEscrow(address(0))initialize(collateral, address(0)) is reachable by calling deposit(address(0), x) — not only via a misconfigured caller. Impact is unchanged (self-harm: rewards for that escrow are permanently unclaimable; the caller is donating to a black hole), so it remains INFO, but the "only reachable via misconfigured caller" caveat in NM-007 is too weak — it is reachable through the normal deposit-on-behalf path.
  • No code changes detected since the prior audit; the two previously-fixed Critical/High issues remain fixed.

Scope

  • Target: src/escrows/StakeDaoEscrow.sol (163 LOC)
  • Branch: feat/stake-dao-escrow
  • Language: Solidity ^0.8.13
  • Peers compared: ConvexEscrowV2.sol, INVEscrow.sol, SimpleERC20Escrow.sol
  • Caller surface read: Market.sol (createEscrow, getEscrow, deposit, withdraw, liquidate)
  • Fork tests read: test/escrowForkTests/StakeDaoEscrowFork.t.sol (live mainnet vault + accountant)
  • Functions analyzed: 9 (constructor, initialize, pay, balance, onDeposit, claim×2, _claim, _gaugeArray, setClaimer)
  • Coupled state pairs mapped: 4
  • Nemesis loop passes run: 4 (Feynman → State → Feynman → State). Converged with no new findings after Pass 4.

Diff vs prior audits

Two prior audit files exist in this directory:

File Targets which version? Critical/High issues Current status
nemesis-stakedaoescrow.md Pre-903c981 (had gaugeArray.push, ordering bug in initialize, pendingRewards(msg.sender) guard) NM-001 CRIT, NM-002 HIGH Both fixed. initialize now sets market = msg.sender before the IMarket.collateral() check (L76 before L77). Base-reward gating replaced by try accountant.claim(...) {} catch {}.
nemesis-verified.md (prior) pay() used tokenBal <= amount (always called withdraw(0) at equality) NM-002 LOW (gas/DoS) Fixed. Current pay() uses <.

What still applies from prior audits and is re-verified against the current code:

  • previewRedeem ↔ withdraw asymmetry (conditional bad debt risk)
  • Arbitrary tokens[] array forwarded into rewardVault.claim (trust on vault)
  • token.approve vs safeApprove
  • Reentrancy DoS via onDeposit if vault performs callbacks

What is new in this audit (introduced by, or only surfaced after, the recent fixes):

  • Silent base-reward failure via try/catch swallowing all errors (NM-002 below)
  • Immutable cache of gauge / accountant / baseRewardToken has no recovery path if the underlying Stake DAO vault is reconfigured (NM-004 below)
  • _beneficiary and _treasury accept address(0) without validation (NM-007 below)

Phase 0 Recon — Attacker Hit List

Attack goals:

  1. Inflate balance() → over-borrow → bad debt to protocol.
  2. Block pay() → unliquidatable position → unrecoverable debt.
  3. Drain/freeze beneficiary rewards (base + extra).
  4. Hijack initialization (proxy or implementation).

Novel surface vs ConvexEscrowV2:

  • Share-based ERC4626-like vault — previewRedeem semantics, share ≠ asset.
  • Uses an Accountant sidecar contract that pays the base reward (CRV/BAL) outside the vault's claim path.
  • Caller-supplied tokens[] array on _claim instead of an internally iterated reward set.
  • accountant.claim(...) wrapped in try/catch — any failure is swallowed.
  • initialize makes an external call to msg.sender (IMarket(market).collateral()) while still inside initialize.

Phase 1 Cross-Reference (Nemesis Map)

Function Writes init-state (market/token/beneficiary) Writes allowlist Touches vault shares Touches token balance External calls
constructor ✗ (immutables only) rewardVault.ACCOUNTANT, rewardVault.gauge, accountant.REWARD_TOKEN
initialize ✓ (one-shot, atomic) ✗ (approve only) rewardVault.asset, IMarket(msg.sender).collateral(), token.approve
pay ✓ (burn via withdraw) ✓ (transfer out) rewardVault.withdraw, token.safeTransfer
onDeposit ✓ (mint via deposit) ✓ (deposit in) rewardVault.deposit
balance (view) reads reads previewRedeem, balanceOf×2
claim×2 / _claim accountant.claim (try/catch), rewardVault.claim
setClaimer

Coupled state pairs

Pair Invariant Sync check
markettokenbeneficiary All three set in a single initialize() call or none ✓ atomic; one-shot guard at L74
token balance(escrow) ↔ vault shares(escrow) balance() = previewRedeem(shares) + token.balanceOf — both legs accounted ✓ both legs read in balance(); pay() drains both legs; onDeposit() reconciles loose tokens into shares
Staker identity ↔ accountant pending-rewards key accountant.pendingRewards(vault, account) must use account == address(this) since the escrow is the staker ✓ Implicit. The accountant's claim reads pending under msg.sender, and the escrow's call makes msg.sender = address(this) from the accountant's perspective. The prior bug at L151 was removed; the new pattern is correct.
gauge (cached) ↔ rewardVault.gauge() (live) Must remain equal for accountant.claim to claim rewards from the right gauge No reconciliation path. See NM-004.

Verification Summary

ID Source Coupled Pair / Concern Severity Verdict
NM-001 Pass 1 Feynman (Cat 4) → Pass 2 State (precision coupling) balance() (previewRedeem) ↔ pay() (actual withdraw) LOW–MED (conditional) TRUE POSITIVE
NM-002 Pass 3 Feynman targeted re-interrogation of try/catch accountant errors are silently swallowed LOW TRUE POSITIVE
NM-003 Pass 2 State parallel-path comparison vs peer _claim forwards arbitrary tokens[] to vault INFO TRUE POSITIVE (vault-trust dependent)
NM-004 Pass 3 Feynman targeted (Cat 1/4) on cached state gauge/accountant immutable; no recovery on vault reconfigure INFO TRUE POSITIVE (operational)
NM-005 Pass 2 State parallel-path comparison token.approve not safeApprove INFO DOWNGRADE → INFO
NM-006 Pass 2 State + Pass 3 Feynman ordering check onDeposit unguarded → reentrancy DoS if vault performs callbacks INFO DOWNGRADE → INFO
NM-007 Pass 1 Feynman (Cat 4) on constructor + initialize _treasury and _beneficiary accept address(0) INFO TRUE POSITIVE (deployment hygiene)

No CRITICAL or HIGH findings in the current code. The previously-CRITICAL initialize ordering bug and previously-HIGH pendingRewards(msg.sender) bug have both been fixed.


NM-001 — Conditional bad debt: balance() may overstate what pay() can actually deliver

Severity: LOW–MEDIUM (conditional on the deployed Stake DAO reward vault's behavior)
Source: Pass 1 Feynman (Cat 4 implicit-trust assumption) + Pass 2 State cross-feed (view path ↔ action path coupling)
Verification: Method A — deep code trace + peer comparison.

Coupled pair: balance() view ↔ pay() actual delivery.
Invariant: For any amount ≤ balance(), pay(recipient, amount) must deliver exactly amount tokens.

Code:

// L104-106
function balance() public view returns (uint256) {
    return rewardVault.previewRedeem(rewardVault.balanceOf(address(this)))
         + token.balanceOf(address(this));
}

// L88-98
function pay(address recipient, uint256 amount) public {
    if (msg.sender != market) revert OnlyMarket();
    uint256 tokenBal = token.balanceOf(address(this));

    if (tokenBal < amount) {
        rewardVault.withdraw(amount - tokenBal, address(this), address(this));
    }

    token.safeTransfer(recipient, amount);
}

Why it's a coupling concern:
ConvexEscrowV2.balance() uses rewardPool.balanceOf(this) directly — Convex staking is 1:1 with the underlying. There is no view/action asymmetry.

Stake DAO's IRewardVault is share-based: previewRedeem(shares) returns a snapshot of the convertible asset value, but does not guarantee that withdraw(assets, ...) for the same amount can succeed if the vault has:

  • a withdraw fee not reflected in previewRedeem (asymmetric ERC4626 variants),
  • a paused state or temporary withdraw cap (maxWithdraw(this) < previewRedeem(shares)),
  • a loss-realization event between view and write.

Market.getCollateralValue(user) (Market.sol:322) feeds balance() directly into borrow-capacity and liquidation pricing. An inflated balance() translates 1:1 into bad-debt exposure if pay() cannot redeem the asserted value.

Trigger sequence (vault paused or capped):

  1. User deposits 100 tokens; escrow holds 100 shares; previewRedeem(100) == 100.
  2. User borrows 80 against balance() == 100.
  3. Vault enters a temporary withdraw-paused / capped state. previewRedeem still returns 100; maxWithdraw(escrow) == 0.
  4. Liquidator calls Market.liquidate(user, debt)escrow.pay(liquidator, ~88)vault.withdraw(88, …) reverts.
  5. Liquidation reverts. The position remains under-collateralized but uncoverable.
  6. Bad debt accrues until the vault unpauses.

Mitigations already present / context:

  • ERC4626 spec requires previewRedeem to "not favor the user" — well-behaved vaults satisfy withdrawable ≥ previewRedeem(shares) in normal operation.
  • This trust assumption is standard across vault-backed escrows.

Recommendation:

  • Document the deployed-vault assumption explicitly in NatSpec.
  • Consider clamping: balance() = min(previewRedeem(shares), maxWithdraw(this)) + token.balanceOf(this) if the deployed vault implements maxWithdraw. This makes the view conservative and aligns with what pay() can deliver.
  • Add a fork test that asserts balance() matches the result of a follow-up pay() across the vault's pause/fee modes.

NM-002 — Silent failure of base-reward claim via try/catch

Severity: LOW
Source: Pass 3 Feynman targeted re-interrogation. Cross-feed from Pass 2 (which flagged the previous version's pendingRewards(msg.sender) bug; the fix introduced the new try/catch).
Verification: Method A — code trace + fork-test inspection.

Code:

// L139-146
function _claim(address[] calldata tokens, address to) internal {
    if (to == address(0) || to == address(this)) revert InvalidReceiver();
    //Claim base reward token (crv, bal, etc.) when available.
    try accountant.claim(_gaugeArray(), new bytes[](0), to) {} catch {}
    //Claim extra reward tokens (cvx, etc.)
    uint256[] memory amounts = rewardVault.claim(tokens, to);
    emit Claim(msg.sender, to, tokens, baseRewardToken, amounts);
}

Concern:
The try/catch block swallows every revert from accountant.claim, including reverts that legitimately should be surfaced (accountant paused, gauge mis-registered, harvestData/length mismatch in a future accountant upgrade, etc.). The Claim event is emitted unconditionally — beneficiaries and external monitoring see "success" with baseRewardToken named even when no base reward was actually transferred.

The pattern is intentional: when there are no pending base rewards, some accountants revert; swallowing makes the function tolerant of that. But the catch is unscoped — it cannot distinguish "no pending rewards" from "accountant is in a bad state."

Impact:

  • Visibility: Users cannot tell whether their base-reward claim succeeded. There is no event for "accountant.claim attempted but failed."
  • Recovery: Pending base rewards remain in the accountant for the escrow's account and can be re-claimed on the next successful call. So this is generally a temporary visibility loss, not a permanent fund loss — unless the failure is structurally permanent (NM-004: stale gauge after a vault reconfigure).
  • Combined with NM-004: If the cached gauge ever becomes stale, every claim silently fails and users will not know base rewards are stuck.

Fork-test coverage: testAllowlistedClaimerReceivesCrvBaseRewardToken asserts CRV reaches the receiver on a healthy mainnet fork (block 25_076_112). It does not cover the failure path; if the accountant ever fails, current tests will not detect the silent swallow.

Recommendation:

  • Replace the silent catch with an event:
    try accountant.claim(_gaugeArray(), new bytes[](0), to) {
        emit BaseRewardClaimed(to);
    } catch (bytes memory reason) {
        emit BaseRewardClaimFailed(to, reason);
    }
    No state change, but the failure becomes observable.
  • Or, scope the catch to expected error selectors (e.g., a NoPendingRewards error) and re-throw everything else.

NM-003 — _claim forwards arbitrary tokens[] to rewardVault.claim

Severity: INFORMATIONAL (vault-trust dependent — carried forward from prior audit)
Source: Pass 2 State parallel-path comparison vs ConvexEscrowV2.
Verification: Method A — peer comparison + fork-test inspection.

Code:

// L144
uint256[] memory amounts = rewardVault.claim(tokens, to);

The escrow forwards an arbitrary tokens[] (chosen by the caller — beneficiary or any allowlisted address) into the Stake DAO reward vault's claim. The escrow does no filtering.

ConvexEscrowV2.claimTo iterates a vault-defined reward set (rewardPool.extraRewards) instead of trusting caller-supplied tokens; the caller cannot influence which tokens are touched.

Security model: Relies entirely on the Stake DAO reward vault enforcing that claim(tokens, receiver):

  • only pays registered reward tokens (never the staked asset),
  • never affects share accounting,
  • never reverts on unknown tokens in a way that could DoS legitimate claims.

If any of those invariants fail in the deployed vault, an allowlisted address could (a) drain non-reward balances via the claim path or (b) grief other claimers.

Fork-test coverage: testClaimCvxRewardsThroughStakeDaoEscrow confirms a real Stake DAO vault only pays the requested registered reward token (CVX) and leaves no escrow dust. So the assumption holds for the current deployed vault. INFO only.

Recommendation: Either accept the trust assumption (and document the vault requirements in NatSpec), or maintain a gov-settable allowlist of reward tokens in the escrow factory and reject anything outside it before calling rewardVault.claim.


NM-004 — Cached gauge/accountant/baseRewardToken have no recovery path if the Stake DAO vault is reconfigured

Severity: INFORMATIONAL (operational; severity escalates if Stake DAO ever migrates gauges/accountants)
Source: Pass 3 Feynman targeted (Cat 1: "why is this cached?" and Cat 4: "what is implicitly trusted about vault immutability?")
Verification: Method A — code trace + interface inspection.

Code:

// L59-65
constructor(address _rewardVault, address _treasury) {
    rewardVault     = IRewardVault(_rewardVault);
    accountant      = IAccountant(rewardVault.ACCOUNTANT());
    gauge           = rewardVault.gauge();
    baseRewardToken = accountant.REWARD_TOKEN();
    treasury        = _treasury;
}

accountant, gauge, and baseRewardToken are read once at construction of the implementation contract, then frozen as immutable. The cloned proxies inherit the same immutables (they live in the implementation bytecode, not storage). No setter exists; every per-user proxy uses the snapshot.

Why this matters:

  • If Stake DAO governance ever swaps the vault's gauge (e.g., emergency migration to a new Curve gauge), every existing escrow proxy continues to call accountant.claim(_gaugeArray(), ...) with the stale gauge. The accountant will likely revert (stale gauge not registered) → try/catch swallows (NM-002) → base rewards are silently and permanently lost for every position created before the swap.
  • Similarly, if the accountant is rotated by the vault, escrow.accountant is stuck pointing at the old contract.
  • There is no recovery path: Market.getEscrow() reuses an existing escrow per user (Market.sol:254). Even closing and reopening the position returns the same proxy.

Mitigating context: In current Curve/Stake DAO architecture, gauges are typically tied to long-lived Curve gauge contracts and rarely change. This is a "what if" rather than an active bug.

Recommendation:

  • Read gauge dynamically from rewardVault.gauge() inside _gaugeArray() (a single SLOAD-like call, gas-cheap on a warm slot, and resilient to vault reconfigure):
    function _gaugeArray() internal view returns (address[] memory gauges) {
        gauges = new address[](1);
        gauges[0] = rewardVault.gauge();
    }
  • Or, add a governance-callable migrateGauge() setter, gated to only update when rewardVault.gauge() changes.
  • Or, document the immutability assumption explicitly in NatSpec and require off-chain monitoring for any vault reconfigure.

NM-005 — token.approve(rewardVault, max) not safeApprove

Severity: INFORMATIONAL (downgrade)
Source: Pass 2 State parallel-path comparison.
Verification: Code trace.

initialize uses token.approve directly (L79) rather than SafeERC20.forceApprove / safeApprove, despite importing SafeERC20. For canonical ERC20s this is fine. For tokens that return false instead of reverting, the approval would silently fail and onDeposit's rewardVault.deposit would revert on transferFrom. For tokens that return () (USDT-style), Solidity 0.8's ABI decoder reverts during decode — initialize would revert, which fails-closed.

Same pattern as ConvexEscrowV2.initialize. Since _token is curated (must match rewardVault.asset() and market.collateral()), this is style only — not a bug. Worth a one-line change if the codebase standardizes on safeApprove everywhere.


NM-006 — Reentrancy DoS through onDeposit if the vault performs callbacks

Severity: INFORMATIONAL (downgrade; DoS-only, trust-on-vault)
Source: Pass 2 State (which functions touch token balance + vault shares) + Pass 3 Feynman ordering (Cat 7).
Verification: Code trace of every external-call boundary.

onDeposit is intentionally unguarded ("should remain callable by anyone to handle direct inbound transfers"). The reentrancy surface:

  • pay()vault.withdraw(...) → if the vault calls back into arbitrary code during withdraw (atypical for ERC4626), the callee could call onDeposit() which would re-stake the just-withdrawn tokens. pay's subsequent safeTransfer then reverts on insufficient balance. DoS, not value loss.
  • pay()token.safeTransfer(recipient, amount) → if token has hooks (ERC777-style), recipient could re-enter onDeposit with the same outcome. The collateral token is curated, so this requires admin onboarding a hook-bearing token.
  • _claimrewardVault.claim(...) → no escrow state mutated after the call (only emit Claim). Safe.

Same trust posture as ConvexEscrowV2. Worth a one-line @dev noting the vault-callback assumption.


NM-007 — _treasury and _beneficiary accept address(0) without validation

Severity: INFORMATIONAL (deployment hygiene)
Source: Pass 1 Feynman Cat 4 (implicit-trust on caller-supplied data).
Verification: Code trace.

Code:

// L59-65
constructor(address _rewardVault, address _treasury) {
    ...
    treasury = _treasury;  // no zero-check
}

// L73-81
function initialize(IERC20 _token, address _beneficiary) public {
    ...
    beneficiary = _beneficiary;  // no zero-check
}

Effects of bad inputs:

  • treasury == address(0): rewardVault.deposit(tokenBal, address(this), treasury) (L115) may revert in vaults that reject a zero referrer, bricking every onDeposit and therefore every market deposit that calls back into the escrow.
  • beneficiary == address(0): both onlyBeneficiary and onlyBeneficiaryOrAllowlist checks compare msg.sender != beneficiary. With beneficiary == 0, no EOA can pass the check (msg.sender is never zero), so the user and any allowlist they would have set are permanently locked out of claim. setClaimer is also gated by onlyBeneficiary, so the allowlist can never be populated either. Rewards accrue but can never be claimed.

Both are deployment-time mistakes rather than exploitation vectors, but they are silent — there's no revert at the point of misconfiguration, only later when the escrow is used.

Recommendation:

if (_treasury == address(0)) revert InvalidTreasury();   // in constructor
if (_beneficiary == address(0)) revert InvalidBeneficiary();  // in initialize

(Market.getEscrow always passes user = msg.sender of the deposit, which can never be zero — so _beneficiary == 0 is only reachable via a misconfigured caller, but the check is cheap defense-in-depth.)


Findings explicitly considered and dismissed

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:

  1. Market.deposit(100) — escrow stakes 100 in vault, shares = 100, previewRedeem(100) = 100.
  2. Market.borrow(80) — protocol believes collateral = 100, debt = 80.
  3. Vault enters paused or capped state (external action — not attacker-controlled, but possible). previewRedeem(100) == 100 still, but vault.withdraw(N) reverts for any positive N.
  4. Position becomes under-collateralized due to price move; Market.liquidate(user, debt)escrow.pay(liquidator, ~88)vault.withdraw(88) reverts → liquidation reverts.
  5. 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):

  1. Many users open positions; each gets an escrow with gauge = G_old cached.
  2. Stake DAO governance migrates the vault to G_new.
  3. Users call claimtry accountant.claim([G_old], ...) reverts (gauge stale) → catch swallows → rewardVault.claim(extras, to) proceeds → event emitted with no base reward delivered.
  4. 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.claim is 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 gauge and accountant cached at all, given they're already proxied via rewardVault?". 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 harvestData array on accountant.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 previewRedeemwithdraw 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.

@08xmt

08xmt commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author
  • NM-001: not materially dangerous for this specific Stake DAO vault. The Stake DAO RewardVault has 1:1 previewRedeem, withdraw, and maxWithdraw == balanceOf
  • NM-002: Silent accountant.claim failure can hide missed base reward claims, but it is not a collateral or bad-debt issue.

import "openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol";
import {IMarket} from "src/interfaces/IMarket.sol";

interface IRewardVault is IERC20 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please provide reference contract address for interface review

function gauge() external view returns (address);
}

interface IAccountant {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please provide reference contract address for interface review

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