From 6b34ebd279483e352cfa4d1bebc146958b383483 Mon Sep 17 00:00:00 2001 From: pashov Date: Sat, 7 Mar 2026 03:55:01 +0200 Subject: [PATCH 1/4] update(solidity-auditor): optimize agent workflow and report pipeline - Merge triage + tier 1 into single scan pass, silent skip for irrelevant vectors - Add parallel verification read step in Turn 4 before FP gate - Remove model choice from agent spawning - Remove Below Confidence Threshold separator from report template - Remove redundant bundle exclusion instruction - Expand attack vectors and adversarial agent instructions Co-Authored-By: Claude Opus 4.6 --- solidity-auditor/SKILL.md | 17 ++-- solidity-auditor/VERSION | 2 +- .../agents/adversarial-reasoning-agent.md | 42 +++++++-- .../references/agents/vector-scan-agent.md | 33 ++++--- .../attack-vectors/attack-vectors-1.md | 85 +++++++++++++++++-- .../attack-vectors/attack-vectors-2.md | 85 +++++++++++++++++-- .../attack-vectors/attack-vectors-3.md | 68 ++++++++++++++- .../attack-vectors/attack-vectors-4.md | 53 +++++++++++- .../references/report-formatting.md | 16 +++- 9 files changed, 356 insertions(+), 45 deletions(-) diff --git a/solidity-auditor/SKILL.md b/solidity-auditor/SKILL.md index 62967c5..8744a59 100644 --- a/solidity-auditor/SKILL.md +++ b/solidity-auditor/SKILL.md @@ -29,16 +29,23 @@ Then continue normally. If the fetch fails (offline, timeout), skip silently. ## Orchestration -**Turn 1 — Discover.** Print the banner, then in the same message make parallel tool calls: (a) Bash `find` for in-scope `.sol` files per mode selection, (b) Glob for `**/references/attack-vectors/attack-vectors-1.md` and extract the `references/` directory path (two levels up). Use this resolved path as `{resolved_path}` for all subsequent references. +**Turn 1 — Discover.** Print the banner, then in the same message make parallel tool calls: (a) Bash `find` for in-scope `.sol` files per mode selection, (b) Glob for `**/references/attack-vectors/attack-vectors-1.md` and extract the `references/` directory path (two levels up), (c) ToolSearch `select:Agent` to pre-load the Agent tool for Turn 3. Use this resolved path as `{resolved_path}` for all subsequent references. -**Turn 2 — Prepare.** In a single message, make three parallel tool calls: (a) Read `{resolved_path}/agents/vector-scan-agent.md`, (b) Read `{resolved_path}/report-formatting.md`, (c) Bash: create four per-agent bundle files (`/tmp/audit-agent-{1,2,3,4}-bundle.md`) in a **single command** — each concatenates **all** in-scope `.sol` files (with `### path` headers and fenced code blocks), then `{resolved_path}/judging.md`, then `{resolved_path}/report-formatting.md`, then `{resolved_path}/attack-vectors/attack-vectors-N.md`; print line counts. Every agent receives the full codebase — only the attack-vectors file differs per agent. Do NOT read or inline any file content into agent prompts — the bundle files replace that entirely. +**Turn 2 — Prepare.** In a single message, make parallel tool calls: (a) Read `{resolved_path}/report-formatting.md`, (b) Bash: create per-agent bundle files in a **single command**. Always create `/tmp/audit-agent-{1,2,3,4}-bundle.md` — each concatenates **all** in-scope `.sol` files (with `### path` headers and fenced code blocks), then `{resolved_path}/attack-vectors/attack-vectors-N.md`, then `{resolved_path}/agents/vector-scan-agent.md`. In **DEEP** mode, also create `/tmp/audit-agent-5-bundle.md` — concatenates all in-scope `.sol` files (same format), then `{resolved_path}/agents/adversarial-reasoning-agent.md`. Print line counts for every bundle created. Every agent receives the full codebase — only the trailing reference file differs. Do NOT read or inline any file content into agent prompts — the bundle files replace that entirely. **Turn 3 — Spawn.** In a single message, spawn all agents as parallel foreground Agent tool calls (do NOT use `run_in_background`). Always spawn Agents 1–4. Only spawn Agent 5 when the mode is **DEEP**. -- **Agents 1–4** (vector scanning) — spawn with `model: "sonnet"`. Each agent prompt must contain the full text of `vector-scan-agent.md` (read in Turn 2, paste into every prompt). After the instructions, add: `Your bundle file is /tmp/audit-agent-N-bundle.md (XXXX lines).` (substitute the real line count). -- **Agent 5** (adversarial reasoning, DEEP only) — spawn with `model: "opus"`. Receives the in-scope `.sol` file paths and the instruction: your reference directory is `{resolved_path}`. Read `{resolved_path}/agents/adversarial-reasoning-agent.md` for your full instructions. +- **Agents 1–4** (vector scanning) — Do NOT paste agent instructions into the prompt — they are already inside each bundle. Prompt: `Your bundle file is /tmp/audit-agent-N-bundle.md (XXXX lines).` (substitute the real agent number and line count). +- **Agent 5** (adversarial reasoning, DEEP only) — Do NOT paste agent instructions into the prompt — they are already inside the bundle. Prompt: `Your bundle file is /tmp/audit-agent-5-bundle.md (XXXX lines).` (substitute the real line count). -**Turn 4 — Report.** Merge all agent results: deduplicate by root cause (keep the higher-confidence version), sort by confidence highest-first, re-number sequentially, and insert the **Below Confidence Threshold** separator row. Print findings directly — do not re-draft or re-describe them. Use report-formatting.md (read in Turn 2) for the scope table and output structure. If `--file-output` is set, write the report to a file (path per report-formatting.md) and print the path. +**Turn 4 — Report.** Process agent findings in this strict order: + +1. **Pre-filter.** Scan all raw findings and immediately drop any that: (a) are informational-only (error messages, naming, gas, NatSpec), or (b) match the "Do Not Report" list in `judging.md` (admin privileges, missing events, centralization without exploit path). One word per drop — no analysis. +2. **Deduplicate.** Group surviving findings by root cause (same contract + same function + same bug class). Keep only the most detailed version of each group, drop the rest. List groups: `"Chainlink staleness: Agent 2, Agent 3, Agent 4 → keep Agent 3"`. +3. **Verification read.** Collect every contract file referenced by the surviving deduplicated findings. Read all of them in a single parallel tool call (one Read per file, targeting the relevant function). Do NOT read files sequentially — batch everything into one message. +4. **FP gate.** Using the code now in context, apply the three checks from `judging.md` (concrete path, reachable entry point, no existing guard) to each deduplicated finding. One line per finding: pass or drop with reason. Do not deliberate — if a check clearly fails, drop immediately. +5. **Confidence score.** For each finding that passed the FP gate, start at 100 and apply each deduction as a yes/no (privileged caller? partial path? self-contained impact?). State the deductions in one line, compute the final score. Do not reconsider or re-derive scores. +6. **Format.** Sort by confidence highest-first, re-number sequentially. Format per `report-formatting.md` (read in Turn 2) — scope table, formatted findings with description and fix diffs (omit fix for findings below 80 confidence), and findings list table. If `--file-output` is set, write the report to a file (path per report-formatting.md) and print the path. ## Banner diff --git a/solidity-auditor/VERSION b/solidity-auditor/VERSION index 56a6051..d8263ee 100644 --- a/solidity-auditor/VERSION +++ b/solidity-auditor/VERSION @@ -1 +1 @@ -1 \ No newline at end of file +2 \ No newline at end of file diff --git a/solidity-auditor/references/agents/adversarial-reasoning-agent.md b/solidity-auditor/references/agents/adversarial-reasoning-agent.md index 6216757..2df5edc 100644 --- a/solidity-auditor/references/agents/adversarial-reasoning-agent.md +++ b/solidity-auditor/references/agents/adversarial-reasoning-agent.md @@ -2,14 +2,46 @@ You are an adversarial security researcher trying to exploit these contracts. There are bugs here — find them. Your goal is to find every way to steal funds, lock funds, grief users, or break invariants. Do not give up. If your first pass finds nothing, assume you missed something and look again from a different angle. +## Tool-Call Budget — MANDATORY + +You have a **hard cap of 1 tool-call turn**: the initial parallel Read of your bundle file. After that turn, you make **ZERO additional tool calls** — no Read, no Grep, no Glob, no Bash, nothing. All analysis happens from the bundle content already in your context. Any extra tool call is wasted time and money. Violating this rule is a critical failure. + ## Critical Output Rule You communicate results back ONLY through your final text response. Do not output findings during analysis. Collect all findings internally and include them ALL in your final response message. Your final response IS the deliverable. Do NOT write any files — no report files, no output files. Your only job is to return findings as text. ## Workflow -1. Read all in-scope `.sol` files, plus `judging.md` and `report-formatting.md` from the reference directory provided in your prompt, in a single parallel batch. Do not use any attack vector reference files. -2. Reason freely about the code — look for logic errors, unsafe external interactions, access control gaps, economic exploits, and any other vulnerability you can construct a concrete attack path for. For each potential finding, apply the FP gate from `judging.md` immediately (three checks). If any check fails → drop and move on without elaborating. Only if all three pass → trace the full attack path, apply score deductions, and format the finding. -3. Your final response message MUST contain every finding **already formatted per `report-formatting.md`** — indicator + bold numbered title, location · confidence line, **Description** with one-sentence explanation, and **Fix** with diff block (omit fix for findings below 80 confidence). Use placeholder sequential numbers (the main agent will re-number). -4. Do not output findings during analysis — compile them all and return them together as your final response. -5. If you find NO findings, respond with "No findings." +1. Read your bundle file in **parallel 1000-line chunks** on your first turn. The line count is in your prompt — compute the offsets and issue all Read calls at once (e.g., for a 5000-line file: `Read(file, limit=1000)`, `Read(file, offset=1000, limit=1000)`, `Read(file, offset=2000, limit=1000)`, `Read(file, offset=3000, limit=1000)`, `Read(file, offset=4000, limit=1000)`). Do NOT read without a limit. These are your ONLY file reads — do NOT read any other file after this step. **After this step you must not call any tool.** + +2. **Pass 1 — Invariant extraction.** Before hunting bugs, list every invariant the system assumes: conservation of value (mints == deposits, burns == withdrawals), access control boundaries (who can call what), ordering assumptions (init before use, approve before transfer), mathematical identities (round-trip conversions return ≤ original), and rate/bound constraints. Write each as a one-line assertion: `INVARIANT: totalSupply == sum(deposits) - sum(withdrawals)`. These are your attack targets. + +3. **Pass 2 — Function-level mutation tracing.** For every state-changing external/public function: + 1. **Map external calls.** List every point where control leaves the contract — including inside libraries and helpers. Read helper source; never assume a wrapper is side-effect-free. + 2. **Trace through callbacks.** For each external call, determine whether it can execute code back on `address(this)` before returning (token callbacks, flash-loan/swap hooks, low-level calls, etc.). If so, inline-trace the callback at that point in the caller's execution using the caller's partially-updated state. Check whether the caller's reentrancy guard also covers the callback — a `nonReentrant` on function A does not protect function B. + 3. **Tally net effect.** Count every mutation (mints, burns, transfers, storage writes) across the full execution including callbacks. The net effect must match the function's intended single operation — any duplication, omission, or stale-state read is a candidate finding against Pass 1 invariants. + +4. **Pass 3 — Cross-contract and multi-step attack paths.** Think like an attacker who can call any combination of contracts in any order across multiple transactions. Look for: + - **State desynchronization**: contract A updates its state but contract B still holds a stale view (cached balances, outdated oracle, lagging approval). + - **Privilege chaining**: combining permissions across contracts to reach a state no single contract intended (e.g., minter role on token A + depositor on pool B = unbacked mint). + - **Sequencing attacks**: operations safe individually but dangerous in a specific order (deposit→oracle update→withdraw, or init→upgrade→re-init). + +5. **Pass 4 — Economic and edge-case reasoning.** Assume the attacker has unlimited capital and can sandwich any transaction. Look for: + - Rounding or precision exploits profitable over many iterations. + - First-depositor / empty-state manipulation. + - Fee parameter extremes (zero fees, max fees) that break assumptions. + - Interactions between rate limits, timelocks, and pause mechanisms that can lock funds or bypass controls. + - Oracle manipulation or stale-price windows that allow value extraction. + +6. **Pass 5 — Steal-the-funds challenge.** For each contract that holds or controls value, ask: "If I wanted to drain this contract, what is my best path?" Try at least two distinct approaches per value-holding contract. If both fail, explain in one line why. If either succeeds, it is a finding. + +7. For each confirmed finding, output in this exact format — nothing more: + ``` + FINDING | location: Contract.function + path: caller → function → state change → impact + description: + fix: + ``` + +8. Do not output findings during analysis — compile them all and return them together as your final response. +9. If you find NO findings, respond with "No findings." diff --git a/solidity-auditor/references/agents/vector-scan-agent.md b/solidity-auditor/references/agents/vector-scan-agent.md index 3d5ae15..eb5f009 100644 --- a/solidity-auditor/references/agents/vector-scan-agent.md +++ b/solidity-auditor/references/agents/vector-scan-agent.md @@ -2,25 +2,34 @@ You are a security auditor scanning Solidity contracts for vulnerabilities. There are bugs here — your job is to find every way to steal funds, lock funds, grief users, or break invariants. Do not accept "no findings" easily. +## Tool-Call Budget — MANDATORY + +You have a **hard cap of 1 tool-call turn**: the initial parallel Read of your bundle file. After that turn, you make **ZERO additional tool calls** — no Read, no Grep, no Glob, no Bash, nothing. All analysis happens from the bundle content already in your context. Any extra tool call is wasted time and money. Violating this rule is a critical failure. + ## Critical Output Rule You communicate results back ONLY through your final text response. Do not output findings during analysis. Collect all findings internally and include them ALL in your final response message. Your final response IS the deliverable. Do NOT write any files — no report files, no output files. Your only job is to return findings as text. ## Workflow -1. Read your bundle file in **parallel 1000-line chunks** on your first turn. The line count is in your prompt — compute the offsets and issue all Read calls at once (e.g., for a 5000-line file: `Read(file, limit=1000)`, `Read(file, offset=1000, limit=1000)`, `Read(file, offset=2000, limit=1000)`, `Read(file, offset=3000, limit=1000)`, `Read(file, offset=4000, limit=1000)`). Do NOT read without a limit. These are your ONLY file reads — do NOT read any other file after this step. -2. **Triage pass.** For each vector, classify into three tiers: - - **Skip** — the named construct AND underlying concept are both absent (e.g., ERC721 vectors when there are no NFTs at all). - - **Borderline** — the named construct is absent but the underlying vulnerability concept could manifest through a different mechanism in this codebase (e.g., "stale cached ERC20 balance" when the code caches cross-contract AMM reserves; "ERC777 reentrancy" when there are flash-swap callbacks). - - **Survive** — the construct or pattern is clearly present. - Output all three tiers — every vector must appear in exactly one: `Skip: V1, V2 ...`, `Surviving: V3, V16 ...`, `Borderline: V8, V22 ...`. End with `Total: N classified` and verify it matches your vector count. Borderline vectors get a 1-sentence relevance check: only promote if you can (a) name the specific function where the concept manifests AND (b) describe in one sentence how the exploit would work; otherwise drop. -3. **Deep pass.** Only for surviving vectors. Use this **structured one-liner format** for each vector's analysis — do NOT write free-form paragraphs: +1. Read your bundle file in **parallel 2000-line chunks** on your first turn. The line count is in your prompt — compute the offsets and issue all Read calls at once (e.g., for a 6000-line file: `Read(file, limit=2000)`, `Read(file, offset=2000, limit=2000)`, `Read(file, offset=4000, limit=2000)`). Do NOT read without a limit. These are your ONLY file reads — do NOT read any other file or re-read any chunk after this step. **After this step you must not call any tool.** +2. **Scan pass.** For each vector, silently skip it if the named construct AND underlying concept are both absent — produce **zero output** for skipped vectors, do not list them, do not explain why they were skipped. For every remaining vector, write a 1-line path trace: ``` - V15: path: deposit() → _expandLock() → lockStart reset | guard: none | verdict: CONFIRM [85] - V22: path: deposit() → _distributeDepositFee() → token.transfer | guard: nonReentrant + require | verdict: DROP (FP gate 3: guarded) + V22: path: deposit() → _distributeDepositFee() → token.transfer | guard: nonReentrant + require | verdict: DROP + V15: path: deposit() → _expandLock() → lockStart reset | guard: none | verdict: INVESTIGATE ``` - For each vector: trace the call chain from external entry point to the vulnerable line — check every modifier, caller restriction, and state guard. Consider alternate manifestations, not just the literal construct named. Confirm the path involves a state-changing external entry point (not a view/pure function). If no match or FP conditions fully apply → DROP in one line (never reconsider). If match → apply the FP gate from `judging.md` (three checks). If any check fails → DROP in one line. Only if all three pass → write CONFIRM with score deductions, then expand into the formatted finding below. **Budget: ≤1 line per dropped vector, ≤3 lines per confirmed vector before its formatted finding.** + Trace the call chain from external entry point to the vulnerable line, list every modifier/require/state guard on that path. Then verdict: + - **DROP** — guard unambiguously prevents the attack. One line, never reconsider. + - **INVESTIGATE** — no guard, partial guard, or guard that might not cover all paths. + +3. **Deep analysis (INVESTIGATE only).** For each INVESTIGATE vector, expand to ≤5 lines: verify the entry point is state-changing, trace the full attack path including cross-function and cross-contract interactions, and check whether any indirect guard (CEI pattern, mutex in a parent call, arithmetic that reverts) closes the gap. Final verdict: **CONFIRM** or **DROP**. 4. **Composability check.** Only if you have 2+ confirmed findings: do any two compound (e.g., DoS + access control = total fund lockout)? If so, note the interaction in the higher-confidence finding's description. -5. Your final response message MUST contain every finding **already formatted per `report-formatting.md`** — indicator + bold numbered title, location · confidence line, **Description** with one-sentence explanation, and **Fix** with diff block (omit fix for findings below 80 confidence). Use placeholder sequential numbers (the main agent will re-number). +5. For each confirmed finding, output in this exact format — nothing more: + ``` + FINDING | location: Contract.function + path: caller → function → state change → impact + description: + fix: + ``` 6. Do not output findings during analysis — compile them all and return them together as your final response. -7. **Hard stop.** After the deep pass, STOP — do not re-examine eliminated vectors, scan outside your assigned vector set, or "revisit"/"reconsider" anything. Output your formatted findings, or "No findings." if none survive. +7. **Hard stop.** After the deep pass, STOP — do not re-examine eliminated vectors, scan outside your assigned vector set, or "revisit"/"reconsider" anything. Output your findings, or "No findings." if none survive. diff --git a/solidity-auditor/references/attack-vectors/attack-vectors-1.md b/solidity-auditor/references/attack-vectors/attack-vectors-1.md index 2f50536..3e8cc0e 100644 --- a/solidity-auditor/references/attack-vectors/attack-vectors-1.md +++ b/solidity-auditor/references/attack-vectors/attack-vectors-1.md @@ -1,6 +1,6 @@ # Attack Vectors Reference (1/4) -170 total attack vectors +219 total attack vectors --- @@ -24,10 +24,6 @@ - **D:** Cross-token math uses hardcoded `1e18` or assumes identical decimals. Pattern: collateral/LTV/rate calculations combining token amounts without per-token `decimals()` normalization. - **FP:** Amounts normalized to canonical precision (WAD/RAY) using each token's `decimals()`. Explicit `10 ** (18 - decimals())` scaling. Protocol only supports tokens with identical verified decimals. -**5. Block Timestamp Dependence** - -- **D:** `block.timestamp` used for game outcomes, randomness (`block.timestamp % N`), or auction timing where ~15s manipulation changes outcome. -- **FP:** Timestamp used only for hour/day-scale periods. Timestamp used only for event logging with no state effect. **6. Beacon Proxy Single-Point-of-Failure Upgrade** @@ -132,10 +128,6 @@ - **D:** Deployment tx replayed on another chain. Same deployer nonce on both chains produces same CREATE address under different control. No EIP-155 chain ID protection. Ref: Wintermute. - **FP:** EIP-155 signatures. `CREATE2` via deterministic factory at same address on all chains. Per-chain deployer EOAs. -**25. DoS via Unbounded Loop** - -- **D:** Loop over user-growable unbounded array: `for (uint i = 0; i < users.length; i++)`. Eventually hits block gas limit. -- **FP:** Array length capped at insertion: `require(arr.length < MAX)`. Loop iterates fixed small constant. **26. Precision Loss - Division Before Multiplication** @@ -229,3 +221,78 @@ - **D:** OApp uses ordered nonce execution. If one message permanently reverts on destination (e.g., recipient contract reverts, invalid state), ALL subsequent messages from that source are blocked. Attacker intentionally sends a poison message to freeze the entire channel. - **FP:** Unordered nonce mode used (LayerZero V2 default). `_lzReceive` wrapped in try/catch with fallback logic. `NonblockingLzApp` pattern (V1). Admin can `skipPayload` / `clearPayload` to unblock. Ref: Code4rena Maia DAO finding #883. + +**178. Memory Struct Copy Not Written Back to Storage** + +- **D:** `MyStruct memory s = myMapping[key]` creates a memory copy. Modifications to `s` do not persist — storage remains unchanged. Common in reward updates, position tracking, and config changes where the developer assumes memory aliases storage. +- **FP:** Uses `storage` keyword: `MyStruct storage s = myMapping[key]`. Explicitly writes back: `myMapping[key] = s` after modification. + +**179. On-Chain Slippage Computed from Manipulated Pool** + +- **D:** `amountOutMin` calculated on-chain by querying the same pool that will execute the swap. Attacker manipulates the pool before the tx, making both the quote and the swap reflect the manipulated state — slippage check passes despite sandwich. +- **FP:** `amountOutMin` supplied by the caller (off-chain quote). Uses a separate oracle for the floor price. TWAP-based minimum. + +**180. Withdrawal Queue Bricked by Zero-Amount Entry** + +- **D:** FIFO withdrawal queue processes entries sequentially. A cancelled or zeroed-out entry causes the loop to `break` or revert on zero amount instead of skipping it, permanently blocking all subsequent withdrawals behind it. +- **FP:** Queue skips zero-amount entries. Cancellation removes the entry or marks it processed. Queue uses linked list allowing removal. + +**181. Lazy Epoch Advancement Skips Reward Periods** + +- **D:** Epoch counter advances only on user interaction. If no one interacts during an epoch, it is never advanced — rewards for that period are miscalculated, lost, or retroactively applied to the wrong epoch when the next interaction occurs. +- **FP:** Keeper or automation advances epochs independently. Epoch catch-up loop processes all skipped epochs on next interaction. Continuous (non-epoch) reward accrual. + +**182. Reward Rate Changed Without Settling Accumulator** + +- **D:** Admin updates the emission rate but `updateReward()` / `updatePool()` is not called first. The new rate is retroactively applied to the entire elapsed period since the last update, overpaying or underpaying rewards for that window. +- **FP:** Rate-change function calls `updateReward()` before applying the new rate. Modifier auto-settles on every state change. + +**183. Liquidated Position Continues Accruing Rewards** + +- **D:** Position is liquidated (balance zeroed, collateral seized) but is not removed from the reward distribution system. `rewardDebt` and accumulated rewards are not reset — the liquidated user earns phantom rewards, or the rewards are locked permanently. +- **FP:** Liquidation calls `_withdrawRewards()` or equivalent before zeroing the position. Reward system checks `balance > 0` before accruing. + +**184. Cached Reward Debt Not Reset After Claim** + +- **D:** After `claimRewards()`, the cached reward amount (`pendingReward` or `rewardDebt`) is not zeroed. On the next claim cycle, the full cached amount is paid again — double (or repeated) payout. +- **FP:** `pendingReward[user] = 0` after transfer. `rewardDebt` recalculated from current balance and accumulator after claim. + +**185. Emission Distribution Before Period Update** + +- **D:** `distribute()` reads the contract's token balance before `updatePeriod()` mints or transfers new emissions. New rewards arrive after distribution already executed — they sit idle until the next cycle, underpaying the current period. +- **FP:** `updatePeriod()` called before `distribute()` in the same tx. Emissions pre-funded before distribution window opens. + +**186. Pause Modifier Blocks Liquidations** + +- **D:** `whenNotPaused` applied broadly to all external functions including `liquidate()`. During a pause, interest accrues and prices move but positions cannot be liquidated — bad debt accumulates unchecked until unpause. +- **FP:** Liquidation functions exempt from pause. Separate `pauseLiquidations` flag with independent governance. Interest accrual also paused. + +**187. Liquidation Arithmetic Reverts at Extreme Price Drops** + +- **D:** When collateral price drops 95%+, liquidation math overflows or underflows — e.g., `collateralNeeded = debt / price` becomes enormous, exceeding available collateral. The liquidation function reverts, making the position unliquidatable and locking bad debt. +- **FP:** Liquidation caps `collateralSeized` at position's total collateral. Graceful handling of underwater positions (full seizure, remaining bad debt socialized). + +**188. Borrower Front-Runs Liquidation** + +- **D:** Borrower observes pending `liquidate()` tx in mempool, front-runs with minimal repayment or collateral top-up to push health factor above threshold. Immediately re-borrows after liquidation tx fails. Repeated indefinitely to maintain risky position. +- **FP:** Liquidation uses flash-loan-resistant health check (same-block deposit doesn't count). Minimum repayment cooldown. Dutch auction liquidation (no fixed threshold to game). + +**189. Liquidation Discount Applied Inconsistently Across Code Paths** + +- **D:** One code path calculates debt at face value, another applies a liquidation discount. When the discounted amount is subtracted from the non-discounted amount, the result underflows or leaves residual bad debt unaccounted. +- **FP:** Discount applied consistently in all paths touching liquidation accounting. Single source of truth for discounted value. + +**190. No Buffer Between Max LTV and Liquidation Threshold** + +- **D:** Max borrowable LTV equals the liquidation threshold. A borrower at max LTV is immediately liquidatable on any adverse price tick. Combined with origination fees, positions can be born underwater. +- **FP:** Max LTV is meaningfully lower than liquidation threshold (e.g., 80% vs 85%). Origination fee deducted from borrowed amount, not added to debt. + +**191. Same-Block Vote-Transfer-Vote** + +- **D:** Governance reads voting power at current block, not a past snapshot. User votes, transfers tokens to a second wallet in the same block, and votes again — doubling their effective vote weight. +- **FP:** `getPastVotes(block.number - 1)` or proposal-creation snapshot. Voting power locked on first vote until proposal closes. ERC20Votes with checkpoint-based historical balances. + +**192. Quorum Computed from Live Supply, Not Snapshot** + +- **D:** `quorum = totalSupply() * quorumBps / 10000` reads current supply. After a proposal is created, an attacker mints tokens (or deposits to inflate supply), lowering the effective quorum percentage needed to pass. +- **FP:** Quorum snapshotted at proposal creation: `totalSupply(proposalSnapshot)`. Fixed absolute quorum amount. Supply changes do not affect active proposals. diff --git a/solidity-auditor/references/attack-vectors/attack-vectors-2.md b/solidity-auditor/references/attack-vectors/attack-vectors-2.md index cb36d20..f4dbe25 100644 --- a/solidity-auditor/references/attack-vectors/attack-vectors-2.md +++ b/solidity-auditor/references/attack-vectors/attack-vectors-2.md @@ -1,6 +1,6 @@ # Attack Vectors Reference (2/4) -170 total attack vectors +219 total attack vectors --- @@ -78,10 +78,6 @@ - **D:** `previewDeposit` returns more shares than `deposit` mints, or `previewMint` charges fewer assets than `mint`. Custom `_convertToShares`/`_convertToAssets` with wrong `Math.mulDiv` rounding direction. - **FP:** OZ ERC4626 base without overriding conversion functions. Custom impl explicitly uses `Floor` for share issuance, `Ceil` for share burning. -**57. Block Number as Timestamp Approximation** - -- **D:** Time computed as `(block.number - startBlock) * 13` assuming fixed block times. Variable across chains/post-Merge. Wrong interest/vesting/rewards. -- **FP:** `block.timestamp` used for all time-sensitive calculations. **58. Transparent Proxy Admin Routing Confusion** @@ -186,10 +182,6 @@ - **D:** Assembly loads a value as a full 32-byte word (`calldataload`, `sload`, `mload`) but treats it as a smaller type (`address`, `uint128`, `uint8`, `bool`) without masking upper bits. Dirty bits cause incorrect comparisons, mapping key mismatches, or storage corruption. Pattern: `let addr := calldataload(4)` used directly without `and(addr, 0xffffffffffffffffffffffffffffffffffffffff)`. - **FP:** Explicit bitmask applied: `and(value, mask)` immediately after load. Value produced by a prior Solidity expression that already cleaned it. `shr(96, calldataload(offset))` pattern that naturally zeros upper bits for addresses. -**77. Griefing via Dust Deposits Resetting Timelocks or Cooldowns** - -- **D:** Timelock/cooldown resets on any deposit with no minimum: `lastActionTime[user] = block.timestamp` inside `deposit(uint256 amount)` without `require(amount >= MIN)`. Attacker calls `deposit(1)` to reset victim's lock indefinitely. -- **FP:** Minimum deposit enforced unconditionally. Cooldown resets only for depositing user. Lock assessed independently of deposit amounts per-user. **78. Returndatasize-as-Zero Assumption** @@ -229,3 +221,78 @@ - **D:** Contract holds rebasing tokens (stETH, AMPL, aTokens) and caches `balanceOf(this)`. After rebase, cached value diverges from actual balance. - **FP:** Rebasing tokens blocked at code level (revert/whitelist). Accounting reads `balanceOf` live. Wrapper tokens (wstETH) used. + +**193. Checkpoint Overwrite on Same-Block Operations** + +- **D:** Multiple delegate/transfer operations in the same block call `_writeCheckpoint()` with the same `block.number` key. Each overwrites the previous — binary search returns the first (incomplete) checkpoint, losing intermediate state. +- **FP:** Checkpoint appends only when `block.number > lastCheckpoint.blockNumber`. Same-block operations accumulate into the existing checkpoint. Off-chain indexer used instead of on-chain lookups. + +**194. Self-Delegation Doubles Voting Power** + +- **D:** Delegating to self adds votes to the delegate (self) but does not subtract the undelegated balance. Voting power is counted twice — once as held tokens, once as delegated votes. +- **FP:** Delegation logic subtracts from holder's direct balance when adding to delegate. Self-delegation is a no-op or explicitly handled. OZ Votes implementation used correctly. + +**195. Nonce Not Incremented on Reverted Execution** + +- **D:** Meta-transaction or permit nonce is checked before execution but only incremented on success. If the inner call reverts (after nonce check, before increment), the same signed message can be replayed until it eventually succeeds. +- **FP:** Nonce incremented before execution (check-effects-interaction). Nonce incremented in both success and failure paths. Deadline-based expiry on signed messages. + +**197. Bridge Global Rate Limit Griefing** + +- **D:** Bridge enforces a global throughput cap (total value per window) not segmented by user. Attacker fills the rate limit by bridging cheap tokens back and forth, blocking all legitimate users from bridging during the cooldown window. +- **FP:** Per-user rate limits. Rate limit segmented by token or route. Whitelist for high-value transfers. No global rate limit. + +**199. Self-Matched Orders Enable Wash Trading** + +- **D:** Order matching does not verify `maker != taker`. A user submits both sides of a trade to farm trading rewards, inflate volume metrics, bypass royalties (NFT), or extract fee rebates. +- **FP:** `require(makerOrder.signer != takerOrder.signer)`. Volume-based rewards use time-weighted averages resistant to single-block spikes. Royalty enforced regardless of counterparty. + +**200. Dutch Auction Price Decay Underflow** + +- **D:** `currentPrice = startPrice - (decayRate * elapsed)`. When the auction runs past the point where price should reach zero, the subtraction underflows — reverting on 0.8+ or wrapping to `type(uint256).max` on <0.8. Auction becomes unfinishable. +- **FP:** `currentPrice = elapsed >= duration ? reservePrice : startPrice - (decayRate * elapsed)`. Floor price enforced. `min()` used to cap decay. + +**201. Timelock Anchored to Deployment, Not Action** + +- **D:** Recovery or admin timelock measured from contract deployment or initialization, not from when the action was queued. Once the initial delay elapses, all future actions can execute instantly — the timelock is effectively permanent bypass. +- **FP:** Timelock resets on each queued action. `executeAfter = block.timestamp + delay` set at queue time. OZ TimelockController pattern. + +**202. Withdrawal Rate Limit Bypassed via Share Transfer** + +- **D:** Per-address withdrawal limit: `require(withdrawn[msg.sender] + amount <= limitPerPeriod)`. User transfers vault shares to a fresh address and withdraws from there — each new address gets a fresh limit. +- **FP:** Limit tracks the underlying position, not the address. Shares are non-transferable or transfer resets withdrawal allowance. KYC-bound withdrawal limits. + +**203. Blacklist and Whitelist Not Mutually Exclusive** + +- **D:** An address can hold both `BLACKLISTED` and `WHITELISTED` roles simultaneously. Whitelist-gated paths do not check the blacklist — a blacklisted address bypasses restrictions by also being whitelisted. +- **FP:** Adding to blacklist auto-removes from whitelist (and vice versa). Single enum role per address. Both checks applied on every restricted path. + +**204. Dead Code After Return Statement** + +- **D:** Critical state update or validation (`require(success)`, `emit Event`, `nonce++`) placed after a `return` statement. The code is unreachable — failures go undetected, events are never emitted, state is never updated. +- **FP:** All critical logic precedes `return`. Compiler warnings for unreachable code are addressed. Linter enforces no-unreachable-code rule. + +**205. Partial Redemption Fails to Reduce Tracked Total** + +- **D:** Withdrawal queue partially fills a redemption request but does not proportionally reduce `totalQueuedShares` or `totalPendingAssets`. The vault's tracked total remains inflated, skewing share price for all other depositors. +- **FP:** Partial fill reduces tracked totals proportionally. Queue uses per-request tracking, not a global aggregate. Atomic full-or-nothing redemptions. + +**206. TWAP Accumulator Not Updated During Sync or Skim** + +- **D:** Pool's `sync()` or `skim()` function updates reserves but does not call `_update()` to advance the TWAP cumulative price accumulator. TWAP observations return stale values, enabling price manipulation through sync-then-trade sequences. +- **FP:** `sync()` calls `_update()` before overwriting reserves. TWAP sourced from external oracle, not internal accumulator. Uniswap V3 `observe()` used (accumulator updated on every swap). + +**207. Expired Oracle Version Silently Assigned Previous Price** + +- **D:** In request-commit oracle patterns (Pyth, custom keepers), an expired or unfulfilled price request is assigned the last valid price instead of reverting or returning invalid. Pending orders execute at stale prices rather than being cancelled. +- **FP:** Expired versions return `price = 0` or `valid = false`, forcing order cancellation. Staleness threshold enforced per-request. Fallback oracle used for expired primaries. + +**208. Funding Rate Derived from Single Trade Price** + +- **D:** Perpetual swap funding rate uses the last trade price as the mark price. A single self-trade at an extreme price skews the funding rate — the attacker profits from funding payments on their opposing position. +- **FP:** Mark price derived from TWAP or external oracle index. Funding rate capped per period. Volume-weighted average price used. + +**209. Open Interest Tracked with Pre-Fee Position Size** + +- **D:** Open interest incremented by the full position size before fees are deducted. Actual exposure is smaller than recorded OI. Aggregate OI is permanently inflated, eventually hitting caps and blocking new positions. +- **FP:** OI incremented by post-fee position size. OI decremented on close by same amount used at open. Periodic OI reconciliation. diff --git a/solidity-auditor/references/attack-vectors/attack-vectors-3.md b/solidity-auditor/references/attack-vectors/attack-vectors-3.md index 51ac5e2..9d0ab1b 100644 --- a/solidity-auditor/references/attack-vectors/attack-vectors-3.md +++ b/solidity-auditor/references/attack-vectors/attack-vectors-3.md @@ -1,6 +1,6 @@ # Attack Vectors Reference (3/4) -170 total attack vectors +219 total attack vectors --- @@ -48,6 +48,7 @@ - **D:** (a) Chainlink aggregator address hardcoded/immutable with no update path — deprecated feed returns stale/zero price. (b) Assumes `feed.decimals() == 8` without runtime check — some feeds return 18 decimals, causing 10^10 scaling error. - **FP:** Feed address updatable via governance. `feed.decimals()` called and used for normalization. Secondary oracle deviation check. +- **Scope:** Covers feed deprecation and decimal assumptions only. Basic staleness checks (`updatedAt`, `answeredInRound >= roundId`, `answer > 0`) are covered by V69 — do not duplicate findings for missing staleness validation under this vector. **94. Upgrade Race Condition / Front-Running** @@ -229,3 +230,68 @@ - **D:** `depositor[tokenId] = msg.sender` without checking `nft.ownerOf(tokenId)`. Approved operator (not owner) calls stake — transfer succeeds via approval, operator credited as depositor. - **FP:** Reads `nft.ownerOf(tokenId)` before transfer and records actual owner. Or `require(nft.ownerOf(tokenId) == msg.sender)`. + +**211. Interest Accrual Rounds to Zero but Timestamp Advances** + +- **D:** `interest = rate * timeDelta / SECONDS_PER_YEAR` rounds to zero when `timeDelta` is small (e.g., <207s at 15% APR). But `lastAccrualTime = block.timestamp` is still updated — the fractional interest is permanently lost, not deferred to the next accrual. +- **FP:** Accumulator uses sufficient precision (e.g., RAY = 1e27) to avoid zero rounding at per-block intervals. `lastAccrualTime` only advances when computed interest > 0. + +**212. Permissionless accrueInterest Griefing** + +- **D:** `accrueInterest()` is permissionless and updates `lastAccrualTime` on every call. Attacker calls it at very short intervals — each call computes zero interest (rounding) but advances the timestamp, systematically suppressing interest accumulation to near-zero. +- **FP:** Minimum accrual interval enforced: `require(block.timestamp - lastAccrualTime >= MIN_INTERVAL)`. Precision high enough that per-block interest > 0. Access-restricted accrual. + +**213. notifyRewardAmount Overwrites Active Reward Period** + +- **D:** Calling `notifyRewardAmount(newAmount)` replaces the current reward period. Remaining undistributed rewards from the old period are silently lost — not carried forward. Admin or attacker can erase pending rewards by notifying a smaller amount. +- **FP:** New notification adds to remaining: `rewardRate = (newAmount + remaining) / duration`. Only callable by designated distributor with timelock. Remaining rewards refunded before reset. + +**214. Governance Proposal Executable Before Voting Period Ends** + +- **D:** `execute()` checks quorum and majority but not `block.timestamp >= proposal.endTime`. Once quorum is met, the proposal can be executed immediately — cutting the voting window short, preventing opposing votes from being cast. +- **FP:** `require(block.timestamp >= proposal.endTime)` in execute. OZ Governor enforces `ProposalState.Succeeded` which requires voting period to have ended. + +**215. Partial Liquidation Leaves Position in Worse State** + +- **D:** Partial liquidation seizes some collateral but does not enforce a minimum post-liquidation health factor. Liquidator cherry-picks the most valuable collateral, leaving the position with worse health than before — approaching full insolvency without triggering full liquidation. +- **FP:** Post-liquidation health factor check: `require(healthFactor(position) >= MIN_HF)`. Full liquidation triggered below a floor threshold. Liquidator must bring position to target health factor. + +**216. Delegation to address(0) Blocks Token Transfers** + +- **D:** Delegating votes to `address(0)` causes `_beforeTokenTransfer` or `_update` hooks to revert when attempting to modify the zero-address delegate's checkpoint. All subsequent transfers and burns for that token holder permanently revert. +- **FP:** Delegation to `address(0)` treated as undelegation (no-op or clears delegation). Hook skips checkpoint update when delegate is `address(0)`. OZ Votes implementation handles this case. + +**217. ERC4626 maxDeposit Returns Non-Zero When Paused** + +- **D:** `maxDeposit()` returns `type(uint256).max` even when the vault is paused. Integrating protocols (aggregators, routers) read this as "deposits accepted," attempt a deposit, and revert. Per ERC4626, `maxDeposit` must return 0 when deposits would revert. +- **FP:** `maxDeposit()` returns 0 when paused. OZ ERC4626 with pausing override. Integrators use `try deposit()` with fallback. + +**219. Deprecated Gauge Blocks Claiming Accrued Rewards** + +- **D:** Killing or deprecating a gauge clears future distributions but also blocks the `claimReward()` path for already-accrued, unclaimed rewards. Users who earned rewards before deprecation cannot retrieve them. +- **FP:** Kill only stops future accrual — claim function remains active for pre-kill balances. Rewards swept to fallback address on deprecation. Emergency claim path bypasses active-gauge check. + +**221. Liquidation Blocked by External Pool Illiquidity** + +- **D:** Liquidation function swaps collateral for debt token via an external DEX. If the DEX pool is drained or lacks liquidity, the swap reverts, making liquidation impossible. Bad debt accumulates while the pool remains illiquid. +- **FP:** Liquidation accepts collateral directly without swap. Fallback liquidation path uses a different DEX or oracle price. Liquidator provides debt token and receives collateral. + +**222. No-Bid Auction Fails to Clear State** + +- **D:** Auction expires without any bids. The finalization function does not clear lien, auction, or escrow data — collateral remains locked in the auction contract with no path to return it to the owner or trigger a new auction. +- **FP:** No-bid finalization returns collateral to owner and clears all associated state. Re-auction mechanism triggered automatically. Timeout-based collateral release. + +**223. Position Reduction Triggers Liquidation** + +- **D:** User attempts to improve health by partially repaying debt or withdrawing a small amount of excess collateral. The intermediate state (after collateral removal, before debt reduction) crosses the liquidation threshold — a bot liquidates the position mid-transaction or in the same block. +- **FP:** Repay and collateral changes are atomic (single function). Health check applied only to final state, not intermediate. Liquidation grace period after position modification. + +**224. Repeated Liquidation of Same Position** + +- **D:** Liquidation function does not flag the position as liquidated. After partial liquidation, the position still appears undercollateralized — a second liquidator (or the same one) liquidates again, seizing collateral beyond what was intended. +- **FP:** Position marked as `liquidated` or deleted after processing. Liquidation requires `status != Liquidated`. Post-liquidation health check prevents re-triggering. + +**225. Loan State Transition Before Interest Settlement** + +- **D:** Repaying principal sets the loan state to `Repaid` before accrued interest is settled. Once in `Repaid` state, the interest accrual function skips the loan — all accumulated interest becomes permanently uncollectable. +- **FP:** `settleInterest()` called before state transition. Interest added to repayment amount: `require(msg.value >= principal + accruedInterest)`. State transition only after full settlement. diff --git a/solidity-auditor/references/attack-vectors/attack-vectors-4.md b/solidity-auditor/references/attack-vectors/attack-vectors-4.md index ac5be21..124d373 100644 --- a/solidity-auditor/references/attack-vectors/attack-vectors-4.md +++ b/solidity-auditor/references/attack-vectors/attack-vectors-4.md @@ -1,6 +1,6 @@ # Attack Vectors Reference (4/4) -170 total attack vectors +219 total attack vectors --- @@ -82,6 +82,7 @@ - **D:** Oracle returns a technically valid but extreme price (e.g., ETH at $0.01 during a flash crash). No min/max sanity bound or deviation check against historical/secondary price. Protocol executes liquidations or swaps at wildly incorrect prices. - **FP:** Circuit breaker: `require(price >= MIN_PRICE && price <= MAX_PRICE)`. Deviation check against secondary oracle source. Heartbeat + price-change-rate limiting. +- **Scope:** Covers missing min/max price bounds and deviation checks only. Basic staleness checks (`updatedAt`, `answeredInRound >= roundId`, `answer > 0`) are covered by V69 — do not duplicate findings for missing staleness validation under this vector. **142. DVN Collusion or Insufficient DVN Diversity** @@ -239,3 +240,53 @@ - **D:** Contract hashes raw calldata for uniqueness (`processedHashes[keccak256(msg.data)]`). Dynamic-type ABI encoding uses offset pointers — multiple distinct calldata layouts decode to identical values. Attacker bypasses dedup with semantically equivalent but bytewise-different calldata. - **FP:** Uniqueness check hashes decoded parameters: `keccak256(abi.encode(decodedParams))`. Nonce-based replay protection. Only fixed-size types in signature (no encoding ambiguity). + +**171. Reward Accrual During Zero-Depositor Period** + +- **D:** Time-based reward distribution starts at vault deployment but no depositors exist yet. First depositor claims all rewards accumulated during the empty period regardless of deposit size or timing. +- **FP:** Rewards only accrue when `totalSupply > 0`. Reward start time set on first deposit. Unclaimed pre-deposit rewards sent to treasury or burned. + +**172. MEV Withdrawal Before Bad Debt Socialization** + +- **D:** External event (liquidation, exploit, depeg) causes vault loss. MEV actor observes pending loss-causing tx in mempool and front-runs a withdrawal at pre-loss share price, leaving remaining depositors to absorb the full loss. +- **FP:** Withdrawals require time-delayed request queue (epoch-based or cooldown). Loss realization and share price update are atomic. Private mempool used for liquidation txs. + +**173. Vault Insolvency via Accumulated Rounding Dust** + +- **D:** Vault tracks `totalAssets` as a storage variable separate from `token.balanceOf(vault)`. Solidity's floor rounding on each deposit/withdrawal creates tiny overages — user receives 1 wei more than burned shares represent. Over many operations `totalAssets` exceeds actual balance, causing last withdrawers to revert. +- **FP:** Rounding consistently favors the vault (round shares up on deposit, round assets down on withdrawal). OZ Math with `Rounding.Ceil`/`Rounding.Floor` applied correctly. + +**174. FIFO Withdrawal Ordering Degrades Yield** + +- **D:** Aggregator vault withdraws from sub-vaults in fixed FIFO order, depleting highest-APY vaults first. Remaining capital concentrates in lowest-yield positions, reducing overall returns for all depositors. +- **FP:** Withdrawal ordering sorted by APY ascending (lowest-yield first). Dynamic rebalancing after withdrawals. Single underlying vault (no ordering issue). + +**175. ERC4626 convertToAssets Used Instead of previewWithdraw** + +- **D:** Integration calls `convertToAssets(shares)` to estimate withdrawal proceeds. Per ERC4626 spec this excludes fees and slippage — actual redeemable amount is lower. Downstream logic (health checks, rebalancing, UI) operates on inflated values. +- **FP:** `previewWithdraw()` or `previewRedeem()` used for actual withdrawal estimates. Vault charges no withdrawal fees. Fee delta accounted separately. + +**176. Unclaimed Reward Tokens from Underlying Protocol** + +- **D:** Vault deposits into yield protocol (Morpho, Aave, Convex) that emits reward tokens. Vault never calls `claim()` or lacks logic to distribute claimed rewards to depositors. Rewards accumulate indefinitely in the vault or underlying protocol, inaccessible to shareholders. +- **FP:** Explicit `claimRewards()` function harvests and distributes. Reward tokens tracked dynamically (mapping, not hardcoded list). Vault sweeps unexpected token balances to treasury. + +**177. Idle Asset Dilution from Sub-Vault Deposit Caps** + +- **D:** Parent/aggregator vault accepts deposits without checking sub-vault capacity. When sub-vaults hit their deposit caps, excess assets sit idle in the parent — earning zero yield but diluting share price for all depositors. +- **FP:** `maxDeposit()` reflects combined sub-vault remaining capacity. Deposits revert when no productive capacity remains. Idle assets auto-routed to fallback yield source. + +**227. Protocol Fee Inflates Reward Accumulator** + +- **D:** Protocol fee routed to treasury is processed through the same `rewardPerToken` accumulator as staker rewards. The accumulator increments as if all distributed tokens go to stakers, but part went to treasury — stakers' `earned()` returns more than the contract holds. +- **FP:** Fee deducted before updating accumulator: `rewardPerToken += (reward - fee) / totalStaked`. Separate accounting for fees and rewards. Fee transferred directly, not through reward distribution. + +**228. Profit Tracking Underflow Blocks Withdrawals** + +- **D:** Vault tracks cumulative strategy profit. When a strategy reports a loss exceeding its recorded profit, `totalProfit -= loss` underflows (reverts on 0.8+). All withdrawal functions that read `totalProfit` are permanently bricked. +- **FP:** Loss capped: `totalProfit -= min(loss, totalProfit)`. Signed integer used for profit/loss tracking. Per-strategy profit tracking (one strategy's loss doesn't affect others). + +**229. Share Redemption at Optimistic Rate** + +- **D:** Shares redeemed at a projected end-of-term exchange rate rather than the current realized rate. Early redeemers receive more than their proportional share of actual assets — late redeemers find the vault depleted. +- **FP:** Redemption uses current `totalAssets / totalSupply`, not projected rate. Withdrawal queue with pro-rata distribution. Mark-to-market valuation updated on each interaction. \ No newline at end of file diff --git a/solidity-auditor/references/report-formatting.md b/solidity-auditor/references/report-formatting.md index 54e13c8..7150f2b 100644 --- a/solidity-auditor/references/report-formatting.md +++ b/solidity-auditor/references/report-formatting.md @@ -53,7 +53,20 @@ Save the report to `assets/findings/{project-name}-pashov-ai-audit-report-{times ``` --- -< ... all findings > +< ... all above-threshold findings > + +--- + +[75] **3. ** + +`ContractName.functionName` · Confidence: 75 + +**Description** +<The vulnerable code pattern and why it is exploitable, in 1 short sentence> + +--- + +< ... all below-threshold findings (description only, no Fix block) > --- @@ -63,7 +76,6 @@ Findings List |---|---|---| | 1 | [95] | <title> | | 2 | [82] | <title> | -| | | **Below Confidence Threshold** | | 3 | [75] | <title> | | 4 | [60] | <title> | From f91a47fb22e04965d56f52cb4ea8b8d6470c3512 Mon Sep 17 00:00:00 2001 From: pashov <krum@pashov.com> Date: Sat, 7 Mar 2026 16:13:15 +0200 Subject: [PATCH 2/4] update(solidity-auditor): add FP gate agent and structured scan output Move FP gate reasoning and report formatting from orchestrator to a dedicated validation agent, reducing orchestrator Turn 4 to mechanical pre-filter, dedup, and passthrough. Scan agents now output structured entry/guards fields per finding for faster downstream validation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --- solidity-auditor/SKILL.md | 10 +- .../references/agents/vector-scan-agent.md | 13 +- .../attack-vectors/attack-vectors-1.md | 182 ++++----- .../attack-vectors/attack-vectors-2.md | 312 ++++++++-------- .../attack-vectors/attack-vectors-3.md | 349 +++++++++--------- .../attack-vectors/attack-vectors-4.md | 337 ++++++++--------- solidity-auditor/references/judging.md | 10 +- .../references/report-formatting.md | 9 + 8 files changed, 584 insertions(+), 638 deletions(-) diff --git a/solidity-auditor/SKILL.md b/solidity-auditor/SKILL.md index 8744a59..7885982 100644 --- a/solidity-auditor/SKILL.md +++ b/solidity-auditor/SKILL.md @@ -31,7 +31,7 @@ Then continue normally. If the fetch fails (offline, timeout), skip silently. **Turn 1 — Discover.** Print the banner, then in the same message make parallel tool calls: (a) Bash `find` for in-scope `.sol` files per mode selection, (b) Glob for `**/references/attack-vectors/attack-vectors-1.md` and extract the `references/` directory path (two levels up), (c) ToolSearch `select:Agent` to pre-load the Agent tool for Turn 3. Use this resolved path as `{resolved_path}` for all subsequent references. -**Turn 2 — Prepare.** In a single message, make parallel tool calls: (a) Read `{resolved_path}/report-formatting.md`, (b) Bash: create per-agent bundle files in a **single command**. Always create `/tmp/audit-agent-{1,2,3,4}-bundle.md` — each concatenates **all** in-scope `.sol` files (with `### path` headers and fenced code blocks), then `{resolved_path}/attack-vectors/attack-vectors-N.md`, then `{resolved_path}/agents/vector-scan-agent.md`. In **DEEP** mode, also create `/tmp/audit-agent-5-bundle.md` — concatenates all in-scope `.sol` files (same format), then `{resolved_path}/agents/adversarial-reasoning-agent.md`. Print line counts for every bundle created. Every agent receives the full codebase — only the trailing reference file differs. Do NOT read or inline any file content into agent prompts — the bundle files replace that entirely. +**Turn 2 — Prepare.** In a single message, make parallel tool calls: (a) Read `{resolved_path}/report-formatting.md`, (b) Bash: create per-agent bundle files in a **single command**. Always create `/tmp/audit-agent-{1,2,3,4}-bundle.md` — each concatenates **all** in-scope `.sol` files (with `### path` headers and fenced code blocks), then `{resolved_path}/attack-vectors/attack-vectors-N.md`, then `{resolved_path}/agents/vector-scan-agent.md`. In **DEEP** mode, also create `/tmp/audit-agent-5-bundle.md` — concatenates all in-scope `.sol` files (same format), then `{resolved_path}/agents/adversarial-reasoning-agent.md`. Also create `/tmp/audit-fp-gate-bundle.md` — concatenates all in-scope `.sol` files (same format), then `{resolved_path}/judging.md`, then `{resolved_path}/report-formatting.md`. Print line counts for every bundle created. Every agent receives the full codebase — only the trailing reference file differs. Do NOT read or inline any file content into agent prompts — the bundle files replace that entirely. **Turn 3 — Spawn.** In a single message, spawn all agents as parallel foreground Agent tool calls (do NOT use `run_in_background`). Always spawn Agents 1–4. Only spawn Agent 5 when the mode is **DEEP**. @@ -40,12 +40,10 @@ Then continue normally. If the fetch fails (offline, timeout), skip silently. **Turn 4 — Report.** Process agent findings in this strict order: -1. **Pre-filter.** Scan all raw findings and immediately drop any that: (a) are informational-only (error messages, naming, gas, NatSpec), or (b) match the "Do Not Report" list in `judging.md` (admin privileges, missing events, centralization without exploit path). One word per drop — no analysis. +1. **Pre-filter.** Scan all raw findings and immediately drop any that are informational-only (error messages, naming, gas, NatSpec, admin-only parameter setting, missing events, centralization without concrete exploit path). One word per drop — no analysis. 2. **Deduplicate.** Group surviving findings by root cause (same contract + same function + same bug class). Keep only the most detailed version of each group, drop the rest. List groups: `"Chainlink staleness: Agent 2, Agent 3, Agent 4 → keep Agent 3"`. -3. **Verification read.** Collect every contract file referenced by the surviving deduplicated findings. Read all of them in a single parallel tool call (one Read per file, targeting the relevant function). Do NOT read files sequentially — batch everything into one message. -4. **FP gate.** Using the code now in context, apply the three checks from `judging.md` (concrete path, reachable entry point, no existing guard) to each deduplicated finding. One line per finding: pass or drop with reason. Do not deliberate — if a check clearly fails, drop immediately. -5. **Confidence score.** For each finding that passed the FP gate, start at 100 and apply each deduction as a yes/no (privileged caller? partial path? self-contained impact?). State the deductions in one line, compute the final score. Do not reconsider or re-derive scores. -6. **Format.** Sort by confidence highest-first, re-number sequentially. Format per `report-formatting.md` (read in Turn 2) — scope table, formatted findings with description and fix diffs (omit fix for findings below 80 confidence), and findings list table. If `--file-output` is set, write the report to a file (path per report-formatting.md) and print the path. +3. **Validate.** Spawn a single foreground Agent. Paste all deduplicated findings verbatim into the prompt, then add: `Validation bundle: /tmp/audit-fp-gate-bundle.md (XXXX lines). Read the entire bundle in parallel 2000-line chunks on your first tool-call turn — these are your ONLY reads, make ZERO additional tool calls after. Then for each finding: verify the path against the actual source code, apply the 3 FP gate checks, and compute the confidence score. Finally, format the surviving findings into the final report per the report-formatting template at the end of the bundle — sort PASS findings by confidence highest-first, re-number sequentially, include scope table + findings with description and fix diffs (omit fix for findings below 80 confidence) + findings list table. Route LEAD items to the Leads section. Return the complete formatted report as your response.` Substitute the real line count. In the prompt, also include the mode and the list of files reviewed so the agent can fill in the scope table. +4. **Output.** Print the validation agent's formatted report verbatim. If `--file-output` is set, also write it to a file (path per report-formatting.md) and print the path. ## Banner diff --git a/solidity-auditor/references/agents/vector-scan-agent.md b/solidity-auditor/references/agents/vector-scan-agent.md index eb5f009..3f41a79 100644 --- a/solidity-auditor/references/agents/vector-scan-agent.md +++ b/solidity-auditor/references/agents/vector-scan-agent.md @@ -13,14 +13,13 @@ You communicate results back ONLY through your final text response. Do not outpu ## Workflow 1. Read your bundle file in **parallel 2000-line chunks** on your first turn. The line count is in your prompt — compute the offsets and issue all Read calls at once (e.g., for a 6000-line file: `Read(file, limit=2000)`, `Read(file, offset=2000, limit=2000)`, `Read(file, offset=4000, limit=2000)`). Do NOT read without a limit. These are your ONLY file reads — do NOT read any other file or re-read any chunk after this step. **After this step you must not call any tool.** -2. **Scan pass.** For each vector, silently skip it if the named construct AND underlying concept are both absent — produce **zero output** for skipped vectors, do not list them, do not explain why they were skipped. For every remaining vector, write a 1-line path trace: +2. **Scan pass.** For each vector, silently skip it if the named construct AND underlying concept are both absent — produce **zero output** for skipped vectors, do not list them, do not explain why they were skipped. For every remaining vector: + - **DROP** — guard unambiguously prevents the attack. Output ONLY the vector ID: `V22: DROP`. No path, no explanation, never reconsider. + - **INVESTIGATE** — no guard, partial guard, or guard that might not cover all paths. Write a 1-line path trace: ``` - V22: path: deposit() → _distributeDepositFee() → token.transfer | guard: nonReentrant + require | verdict: DROP V15: path: deposit() → _expandLock() → lockStart reset | guard: none | verdict: INVESTIGATE ``` - Trace the call chain from external entry point to the vulnerable line, list every modifier/require/state guard on that path. Then verdict: - - **DROP** — guard unambiguously prevents the attack. One line, never reconsider. - - **INVESTIGATE** — no guard, partial guard, or guard that might not cover all paths. + Trace the call chain from external entry point to the vulnerable line, list every modifier/require/state guard on that path. 3. **Deep analysis (INVESTIGATE only).** For each INVESTIGATE vector, expand to ≤5 lines: verify the entry point is state-changing, trace the full attack path including cross-function and cross-contract interactions, and check whether any indirect guard (CEI pattern, mutex in a parent call, arithmetic that reverts) closes the gap. Final verdict: **CONFIRM** or **DROP**. 4. **Composability check.** Only if you have 2+ confirmed findings: do any two compound (e.g., DoS + access control = total fund lockout)? If so, note the interaction in the higher-confidence finding's description. @@ -28,8 +27,10 @@ You communicate results back ONLY through your final text response. Do not outpu ``` FINDING | location: Contract.function path: caller → function → state change → impact + entry: <permissionless | admin-only | restricted to ContractName> + guards: <none | guard1, guard2, ...> description: <one sentence> fix: <one-sentence suggestion or short diff> ``` 6. Do not output findings during analysis — compile them all and return them together as your final response. -7. **Hard stop.** After the deep pass, STOP — do not re-examine eliminated vectors, scan outside your assigned vector set, or "revisit"/"reconsider" anything. Output your findings, or "No findings." if none survive. +7. **Hard stop.** After the deep pass, STOP — do not re-examine eliminated vectors, do not produce "additional findings" or scan outside your assigned vector set, do not "revisit"/"reconsider" anything. Output your findings, or "No findings." if none survive. diff --git a/solidity-auditor/references/attack-vectors/attack-vectors-1.md b/solidity-auditor/references/attack-vectors/attack-vectors-1.md index 3e8cc0e..18057d0 100644 --- a/solidity-auditor/references/attack-vectors/attack-vectors-1.md +++ b/solidity-auditor/references/attack-vectors/attack-vectors-1.md @@ -1,6 +1,6 @@ # Attack Vectors Reference (1/4) -219 total attack vectors +220 total attack vectors --- @@ -24,275 +24,257 @@ - **D:** Cross-token math uses hardcoded `1e18` or assumes identical decimals. Pattern: collateral/LTV/rate calculations combining token amounts without per-token `decimals()` normalization. - **FP:** Amounts normalized to canonical precision (WAD/RAY) using each token's `decimals()`. Explicit `10 ** (18 - decimals())` scaling. Protocol only supports tokens with identical verified decimals. - -**6. Beacon Proxy Single-Point-of-Failure Upgrade** +**5. Beacon Proxy Single-Point-of-Failure Upgrade** - **D:** Multiple proxies read implementation from single Beacon. Compromising Beacon owner upgrades all proxies at once. `UpgradeableBeacon.owner()` returns single EOA. - **FP:** Beacon owner is multisig + timelock. `Upgraded` events monitored. Per-proxy upgrade authority where isolation required. -**7. lzCompose Sender Impersonation (Composed Message Spoofing)** +**6. lzCompose Sender Impersonation (Composed Message Spoofing)** - **D:** `lzCompose` implementation does not validate `msg.sender == endpoint` or does not check the `_from` parameter against the expected OFT address. Attacker calls `lzCompose` directly or via nested compose messages where sender context degrades to `address(this)`, impersonating the OFT contract. - **FP:** `require(msg.sender == address(endpoint))` and `require(_from == expectedOFT)` validated. No nested compose message support. Standard `OAppReceiver` modifier used. Ref: Tapioca USDO/TOFT exploit (HIGH severity). -**8. Invariant or Cap Enforced on One Code Path But Not Another** +**7. Invariant or Cap Enforced on One Code Path But Not Another** - **D:** A constraint (pool cap, max supply, position limit, collateral ratio) is enforced during normal operation (e.g., `deposit()`) but not during settlement, reward distribution, interest accrual, or emergency paths. Constraint violated through the unguarded path. - **FP:** Invariant check applied in a shared modifier/internal function called by all relevant paths. Post-condition assertion validates invariant after every state change. Comprehensive integration tests verify invariant across all entry points. -**9. msg.value Reuse in Loop / Multicall** +**8. msg.value Reuse in Loop / Multicall** - **D:** `msg.value` read inside a loop or `delegatecall`-based multicall. Each iteration/sub-call sees the full original value — credits `n * msg.value` for one payment. - **FP:** `msg.value` captured to local variable, decremented per iteration, total enforced. Function non-payable. Multicall uses `call` not `delegatecall`. -**10. Zero-Amount Transfer Revert** +**9. Zero-Amount Transfer Revert** - **D:** `token.transfer(to, amount)` where `amount` can be zero (rounded fee, unclaimed yield). Some tokens (LEND, early BNB) revert on zero-amount transfers, DoS-ing distribution loops. - **FP:** `if (amount > 0)` guard before all transfers. Minimum claim amount enforced. Token whitelist verified to accept zero transfers. ---- - ---- - -**11. ERC1155 safeBatchTransferFrom Unchecked Array Lengths** +**10. ERC1155 safeBatchTransferFrom Unchecked Array Lengths** - **D:** Custom `_safeBatchTransferFrom` iterates `ids`/`amounts` without `require(ids.length == amounts.length)`. Assembly-optimized paths may silently read uninitialized memory. - **FP:** OZ ERC1155 base used unmodified. Custom override asserts equal lengths as first statement. -**12. ERC721/ERC1155 Callback Reentrancy** +**11. ERC721/ERC1155 Callback Reentrancy** - **D:** `safeTransferFrom`/`safeMint` called before state updates. Callbacks (`onERC721Received`/`onERC1155Received`) enable reentry. - **FP:** All state committed before safe transfer. `nonReentrant` applied. -**13. Depeg of Pegged or Wrapped Asset Breaking Protocol Assumptions** +**12. Depeg of Pegged or Wrapped Asset Breaking Protocol Assumptions** - **D:** Protocol assumes 1:1 peg between assets (stETH:ETH, WBTC:BTC, USDC:USD) in pricing, collateral valuation, or swap routing. No depeg tolerance or independent oracle for the derivative. During depeg, collateral is overvalued, enabling undercollateralized borrows or incorrect swaps. - **FP:** Independent price feed per asset (not assumed 1:1). Configurable depeg threshold triggering protective measures (pause, adjusted LTV). Protocol documentation explicitly acknowledges and accepts depeg risk. -**14. Missing onERC1155BatchReceived Causes Token Lock** +**13. Missing onERC1155BatchReceived Causes Token Lock** - **D:** Contract implements `onERC1155Received` but not `onERC1155BatchReceived` (or returns wrong selector). `safeBatchTransferFrom` reverts, blocking batch settlement/distribution. - **FP:** Both callbacks implemented correctly, or inherits OZ `ERC1155Holder`. Protocol exclusively uses single-item `safeTransferFrom`. -**15. Missing or Incorrect Access Modifier** +**14. Missing or Incorrect Access Modifier** - **D:** State-changing function (`setOwner`, `withdrawFunds`, `mint`, `pause`, `setOracle`) has no access guard or modifier references uninitialized variable. `public`/`external` on privileged operations with no restriction. - **FP:** Function is intentionally permissionless with non-critical worst-case outcome (e.g., advancing a public time-locked process). -**16. extcodesize Zero in Constructor** +**15. extcodesize Zero in Constructor** - **D:** `require(msg.sender.code.length == 0)` as EOA check. Contract constructors have `extcodesize == 0` during execution, bypassing the check. - **FP:** Check is non-security-critical. Function protected by merkle proof, signed permit, or other mechanism unsatisfiable from constructor. -**17. Solmate SafeTransferLib Missing Contract Existence Check** +**16. Solmate SafeTransferLib Missing Contract Existence Check** - **D:** Protocol uses Solmate's `SafeTransferLib` for ERC20 transfers. Unlike OZ `SafeERC20`, Solmate does not verify target address contains code — `transfer`/`transferFrom` to an EOA or not-yet-deployed `CREATE2` address returns success silently, crediting a phantom deposit. - **FP:** OZ `SafeERC20` used instead. Manual `require(token.code.length > 0)` check. Token addresses verified at construction/initialization. -**18. Re-initialization Attack** +**17. Re-initialization Attack** - **D:** V2 uses `initializer` instead of `reinitializer(2)`. Or upgrade resets initialized counter / storage-collides bool to false. Ref: AllianceBlock (2024). - **FP:** `reinitializer(version)` with correctly incrementing versions for V2+. Tests verify `initialize()` reverts after first call. -**19. ERC1155 uri() Missing {id} Substitution** +**18. ERC1155 uri() Missing {id} Substitution** - **D:** `uri(uint256 id)` returns fully resolved URL instead of template with literal `{id}` placeholder per EIP-1155. Clients expect to substitute zero-padded hex ID client-side. Static/empty return collapses all token metadata. - **FP:** Returns string containing literal `{id}`. Or per-ID on-chain URI with documented deviation from substitution spec. -**20. Immutable Variable Context Mismatch** +**19. Immutable Variable Context Mismatch** - **D:** Implementation uses `immutable` variables (embedded in bytecode, not storage). Proxy `delegatecall` gets implementation's hardcoded values regardless of per-proxy needs. E.g., `immutable WETH` — every proxy gets same address. - **FP:** Immutable values intentionally identical across all proxies. Per-proxy config uses storage via `initialize()`. ---- - ---- - -**21. validateUserOp Signature Not Bound to nonce or chainId** +**20. validateUserOp Signature Not Bound to nonce or chainId** - **D:** `validateUserOp` reconstructs digest manually (not via `entryPoint.getUserOpHash`) omitting `userOp.nonce` or `block.chainid`. Enables cross-chain or in-chain replay. - **FP:** Digest from `entryPoint.getUserOpHash(userOp)` (includes sender, nonce, chainId). Custom digest explicitly includes both. -**22. Blacklistable or Pausable Token in Critical Payment Path** +**21. Blacklistable or Pausable Token in Critical Payment Path** - **D:** Push-model transfer `token.transfer(recipient, amount)` with USDC/USDT or other blacklistable token. Blacklisted recipient reverts entire function, DOSing withdrawals/liquidations/fees. - **FP:** Pull-over-push pattern (recipients withdraw own funds). Skip-on-failure `try/catch` on fee distribution. Token whitelist excludes blacklistable tokens. -**23. Improper Flash Loan Callback Validation** +**22. Improper Flash Loan Callback Validation** - **D:** `onFlashLoan` callback doesn't verify `msg.sender == lendingPool`, or doesn't check `initiator`/`token`/`amount`. Callable directly without real flash loan. - **FP:** Both `msg.sender == address(lendingPool)` and `initiator == address(this)` validated. Token/amount checked. -**24. Cross-Chain Deployment Replay** +**23. Cross-Chain Deployment Replay** - **D:** Deployment tx replayed on another chain. Same deployer nonce on both chains produces same CREATE address under different control. No EIP-155 chain ID protection. Ref: Wintermute. - **FP:** EIP-155 signatures. `CREATE2` via deterministic factory at same address on all chains. Per-chain deployer EOAs. - -**26. Precision Loss - Division Before Multiplication** +**24. Precision Loss - Division Before Multiplication** - **D:** `(a / b) * c` — truncation before multiplication amplifies error. E.g., `fee = (amount / 10000) * bps`. Correct: `(a * c) / b`. - **FP:** `a` provably divisible by `b` (enforced by `require(a % b == 0)` or mathematical construction). -**27. ecrecover Returns address(0) on Invalid Signature** +**25. ecrecover Returns address(0) on Invalid Signature** - **D:** Raw `ecrecover` without `require(recovered != address(0))`. If `authorizedSigner` is uninitialized or `permissions[address(0)]` is non-zero, garbage signature gains privileges. - **FP:** OZ `ECDSA.recover()` used (reverts on address(0)). Explicit zero-address check present. -**28. Function Selector Clash in Proxy** +**26. Function Selector Clash in Proxy** - **D:** Proxy and implementation share a 4-byte selector collision. Call intended for implementation routes to proxy's function (or vice versa). - **FP:** Transparent proxy pattern (admin/user call routing separates namespaces). UUPS with no custom proxy functions — all calls delegate unconditionally. -**29. CREATE2 Address Squatting (Counterfactual Front-Running)** +**27. CREATE2 Address Squatting (Counterfactual Front-Running)** - **D:** CREATE2 salt not bound to `msg.sender`. Attacker precomputes address and deploys first. For AA wallets: attacker deploys wallet to user's counterfactual address with attacker as owner. - **FP:** Salt incorporates `msg.sender`: `keccak256(abi.encodePacked(msg.sender, userSalt))`. Factory restricts deployer. Different owner in constructor produces different address. -**30. Return Bomb (Returndata Copy DoS)** +**28. Return Bomb (Returndata Copy DoS)** - **D:** `(bool success, bytes memory data) = target.call(payload)` where `target` is user-supplied. Malicious target returns huge returndata; copying costs enormous gas. - **FP:** Returndata not copied (assembly call without copy, or gas-limited). Callee is hardcoded trusted contract. ---- - ---- - -**31. Immutable / Constructor Argument Misconfiguration** +**29. Immutable / Constructor Argument Misconfiguration** - **D:** Constructor sets `immutable` values (admin, fee, oracle, token) that can't change post-deploy. Multiple same-type `address` params where order can be silently swapped. No post-deploy verification. - **FP:** Deployment script reads back and asserts every configured value. Constructor validates: `require(admin != address(0))`, `require(feeBps <= 10000)`. -**32. Small-Type Arithmetic Overflow Before Upcast** +**30. Small-Type Arithmetic Overflow Before Upcast** - **D:** Arithmetic on `uint8`/`uint16`/`uint32` before assigning to wider type: `uint256 result = a * b` where `a`,`b` are `uint8`. Overflow happens in narrow type before widening. Solidity 0.8 overflow check is still on the narrow type. - **FP:** Operands explicitly upcast before operation: `uint256(a) * uint256(b)`. SafeCast used. -**33. ERC4626 Missing Allowance Check in withdraw() / redeem()** +**31. ERC4626 Missing Allowance Check in withdraw() / redeem()** - **D:** `withdraw(assets, receiver, owner)` / `redeem(shares, receiver, owner)` where `msg.sender != owner` but no allowance check/decrement before burning shares. Any address can burn arbitrary owner's shares. - **FP:** `_spendAllowance(owner, caller, shares)` called unconditionally when `caller != owner`. OZ ERC4626 without custom overrides. -**34. mstore8 Partial Write Leaving Dirty Bytes** +**32. mstore8 Partial Write Leaving Dirty Bytes** - **D:** `mstore8` writes a single byte at a memory offset, but subsequent `mload` reads the full 32-byte word containing that byte. The remaining 31 bytes retain prior memory contents (potentially uninitialized or stale data). Pattern: building a byte array with `mstore8` in a loop, then hashing or returning the full memory region — dirty bytes corrupt the result. - **FP:** Full word zeroed with `mstore(ptr, 0)` before byte-level writes. `mload` result masked to extract only the written bytes. `mstore` used instead of `mstore8` with proper shifting. -**35. Batch Distribution Dust Residual** +**33. Batch Distribution Dust Residual** - **D:** Loop distributes funds proportionally: `share = total * weight[i] / totalWeight`. Cumulative rounding causes `sum(shares) < total`, leaving dust locked in contract. Pattern: N recipients each computed independently without remainder handling. - **FP:** Last recipient gets `total - sumOfPrevious`. Dust swept to treasury. `mulDiv` with accumulator tracking. Protocol accepts bounded dust loss by design. -**36. Arbitrary `delegatecall` in Implementation** +**34. Arbitrary `delegatecall` in Implementation** - **D:** Implementation exposes `delegatecall` to user-supplied address without restriction. Pattern: `target.delegatecall(data)` where `target` is caller-controlled. Ref: Furucombo (2021). - **FP:** Target is hardcoded immutable address. Whitelist of approved targets enforced. `call` used instead. -**37. Commit-Reveal Scheme Not Bound to msg.sender** +**35. Commit-Reveal Scheme Not Bound to msg.sender** - **D:** Commitment hash does not include `msg.sender`: `commit = keccak256(abi.encodePacked(value, salt))`. Attacker copies a victim's commitment from the chain/mempool and submits their own reveal for the same hash from a different address. Affects auctions, governance votes, randomness. - **FP:** Commitment includes sender: `keccak256(abi.encodePacked(msg.sender, value, salt))`. Reveal validates `msg.sender` matches stored committer. -**38. Delegate Privilege Escalation** +**36. Delegate Privilege Escalation** - **D:** `setDelegate()` appoints an address that can manage OApp configurations including DVNs, Executors, message libraries, and can skip/clear payloads. If delegate is set to an insecure address (EOA, unrelated contract) or differs from owner without governance controls, the delegate can silently reconfigure the OApp's entire security stack. - **FP:** Delegate == owner. Delegate is a governance timelock or multisig. `setDelegate` protected by the same access controls as `setPeer`. -**39. Cross-Chain Supply Accounting Invariant Violation** +**37. Cross-Chain Supply Accounting Invariant Violation** - **D:** The fundamental invariant `total_locked_source >= total_minted_destination` is violated. Can occur through: decimal conversion errors between chains, `_credit` callable without corresponding `_debit`, race conditions in multi-chain deployments, or any bug that allows minting without locking. Minted tokens become partially or fully unbacked. - **FP:** Invariant verified via monitoring/alerting. `_credit` only callable from verified `lzReceive` path. Decimal conversion tested across all supported chains. Rate limits cap maximum exposure per time window. ---- - -**40. ERC1155 onERC1155Received Return Value Not Validated** +**38. ERC1155 onERC1155Received Return Value Not Validated** - **D:** Custom ERC1155 calls `onERC1155Received` but doesn't check returned `bytes4` equals `0xf23a6e61`. Non-compliant recipient silently accepts tokens it can't handle. - **FP:** OZ ERC1155 base validates selector. Custom impl explicitly checks return value. ---- - -**41. Small Positions Unliquidatable Due to Insufficient Incentive (Bad Debt)** +**39. Small Positions Unliquidatable Due to Insufficient Incentive (Bad Debt)** - **D:** Positions below a certain USD value cost more gas to liquidate than the liquidation reward. During market downturns, these "dust positions" accumulate bad debt that no liquidator will process, eroding protocol solvency. - **FP:** Minimum position size enforced at borrow time. Protocol-operated liquidation bot covers dust positions. Socialized bad debt mechanism (insurance fund, haircuts). -**42. Ordered Message Channel Blocking (Nonce DoS)** +**40. Ordered Message Channel Blocking (Nonce DoS)** - **D:** OApp uses ordered nonce execution. If one message permanently reverts on destination (e.g., recipient contract reverts, invalid state), ALL subsequent messages from that source are blocked. Attacker intentionally sends a poison message to freeze the entire channel. - **FP:** Unordered nonce mode used (LayerZero V2 default). `_lzReceive` wrapped in try/catch with fallback logic. `NonblockingLzApp` pattern (V1). Admin can `skipPayload` / `clearPayload` to unblock. Ref: Code4rena Maia DAO finding #883. -**178. Memory Struct Copy Not Written Back to Storage** +**41. Self-Liquidation Profit Extraction** -- **D:** `MyStruct memory s = myMapping[key]` creates a memory copy. Modifications to `s` do not persist — storage remains unchanged. Common in reward updates, position tracking, and config changes where the developer assumes memory aliases storage. -- **FP:** Uses `storage` keyword: `MyStruct storage s = myMapping[key]`. Explicitly writes back: `myMapping[key] = s` after modification. +- **D:** Borrower liquidates their own undercollateralized position from a second address, collecting the liquidation bonus/discount on their own collateral. Profitable whenever liquidation incentive exceeds the cost of being slightly undercollateralized. +- **FP:** `require(msg.sender != borrower)` on liquidation. Liquidation penalty exceeds any collateral bonus. Liquidation incentive is small enough that self-liquidation is net-negative after gas. -**179. On-Chain Slippage Computed from Manipulated Pool** +**42. State-Time Lag Exploitation (lzRead Stale State)** -- **D:** `amountOutMin` calculated on-chain by querying the same pool that will execute the swap. Attacker manipulates the pool before the tx, making both the quote and the swap reflect the manipulated state — slippage check passes despite sandwich. -- **FP:** `amountOutMin` supplied by the caller (off-chain quote). Uses a separate oracle for the floor price. TWAP-based minimum. +- **D:** `lzRead` queries state on a remote chain, but there is a latency window between query and delivery of the result via `lzReceive`. During this window, the queried state may change (token transferred, position closed, price moved). Protocol makes irreversible decisions based on the stale read result. +- **FP:** Read targets immutable or slowly-changing state (contract code, historical data). Read result treated as a hint with on-chain re-validation. Time-sensitive operations require fresh on-chain state, not cross-chain reads. -**180. Withdrawal Queue Bricked by Zero-Amount Entry** +**43. Integer Overflow / Underflow** -- **D:** FIFO withdrawal queue processes entries sequentially. A cancelled or zeroed-out entry causes the loop to `break` or revert on zero amount instead of skipping it, permanently blocking all subsequent withdrawals behind it. -- **FP:** Queue skips zero-amount entries. Cancellation removes the entry or marks it processed. Queue uses linked list allowing removal. +- **D:** Arithmetic in `unchecked {}` (>=0.8) without prior bounds check: subtraction without `require(amount <= balance)`, large multiplications. Any arithmetic in <0.8 without SafeMath. +- **FP:** Range provably bounded by earlier checks in same function. `unchecked` only for `++i` loop increments where `i < arr.length`. -**181. Lazy Epoch Advancement Skips Reward Periods** +**44. Function Selector Clashing (Proxy Backdoor)** -- **D:** Epoch counter advances only on user interaction. If no one interacts during an epoch, it is never advanced — rewards for that period are miscalculated, lost, or retroactively applied to the wrong epoch when the next interaction occurs. -- **FP:** Keeper or automation advances epochs independently. Epoch catch-up loop processes all skipped epochs on next interaction. Continuous (non-epoch) reward accrual. +- **D:** Proxy contains a function whose 4-byte selector collides with an implementation function. User calls route to proxy logic instead of delegating. +- **FP:** Transparent proxy pattern separates admin/user routing. UUPS proxy has no custom functions — all calls delegate. -**182. Reward Rate Changed Without Settling Accumulator** +**45. OFT Shared Decimals Truncation (uint64 Overflow)** -- **D:** Admin updates the emission rate but `updateReward()` / `updatePool()` is not called first. The new rate is retroactively applied to the entire elapsed period since the last update, overpaying or underpaying rewards for that window. -- **FP:** Rate-change function calls `updateReward()` before applying the new rate. Modifier auto-settles on every state change. +- **D:** OFT converts between local decimals and shared decimals (typically 6). `_toSD()` casts to `uint64`. If `sharedDecimals >= localDecimals` (both 18), no decimal conversion occurs but the `uint64` cast silently truncates amounts exceeding ~18.4e18. Also: custom fee logic applied BEFORE `_removeDust()` produces incorrect fee calculations. +- **FP:** Standard OFT with `sharedDecimals = 6` and `localDecimals = 18` (default). Fee logic applied after dust removal. Transfer amounts validated against `uint64.max` before conversion. -**183. Liquidated Position Continues Accruing Rewards** +**46. UUPS Upgrade Logic Removed in New Implementation** -- **D:** Position is liquidated (balance zeroed, collateral seized) but is not removed from the reward distribution system. `rewardDebt` and accumulated rewards are not reset — the liquidated user earns phantom rewards, or the rewards are locked permanently. -- **FP:** Liquidation calls `_withdrawRewards()` or equivalent before zeroing the position. Reward system checks `balance > 0` before accruing. +- **D:** New UUPS implementation doesn't inherit `UUPSUpgradeable` or removes `upgradeTo`/`upgradeToAndCall`. Proxy permanently loses upgrade capability. Pattern: V2 missing `_authorizeUpgrade` override. +- **FP:** Every version inherits `UUPSUpgradeable`. Tests verify `upgradeTo` works after each upgrade. OZ upgrades plugin checks in CI. -**184. Cached Reward Debt Not Reset After Claim** +**47. ERC721 onERC721Received Arbitrary Caller Spoofing** -- **D:** After `claimRewards()`, the cached reward amount (`pendingReward` or `rewardDebt`) is not zeroed. On the next claim cycle, the full cached amount is paid again — double (or repeated) payout. -- **FP:** `pendingReward[user] = 0` after transfer. `rewardDebt` recalculated from current balance and accumulator after claim. +- **D:** `onERC721Received` uses parameters (`from`, `tokenId`) to update state without verifying `msg.sender` is the expected NFT contract. Anyone calls directly with fabricated parameters. +- **FP:** `require(msg.sender == address(nft))` before state update. Function is view-only or reverts unconditionally. -**185. Emission Distribution Before Period Update** +**48. ERC1155 totalSupply Inflation via Reentrancy Before Supply Update** -- **D:** `distribute()` reads the contract's token balance before `updatePeriod()` mints or transfers new emissions. New rewards arrive after distribution already executed — they sit idle until the next cycle, underpaying the current period. -- **FP:** `updatePeriod()` called before `distribute()` in the same tx. Emissions pre-funded before distribution window opens. +- **D:** `totalSupply[id]` incremented AFTER `_mint` callback. During `onERC1155Received`, `totalSupply` is stale-low, inflating caller's share in any supply-dependent formula. Ref: OZ GHSA-9c22-pwxw-p6hx (2021). +- **FP:** OZ >= 4.3.2 (patched ordering). `nonReentrant` on all mint functions. No supply-dependent logic callable from mint callback. -**186. Pause Modifier Blocks Liquidations** +**49. Missing Nonce (Signature Replay)** -- **D:** `whenNotPaused` applied broadly to all external functions including `liquidate()`. During a pause, interest accrues and prices move but positions cannot be liquidated — bad debt accumulates unchecked until unpause. -- **FP:** Liquidation functions exempt from pause. Separate `pauseLiquidations` flag with independent governance. Interest accrual also paused. +- **D:** Signed message has no per-user nonce, or nonce present but never stored/incremented after use. Same signature resubmittable. +- **FP:** Monotonic per-signer nonce in signed payload, checked and incremented atomically. Or `usedSignatures[hash]` mapping. -**187. Liquidation Arithmetic Reverts at Extreme Price Drops** +**50. Single-Function Reentrancy** -- **D:** When collateral price drops 95%+, liquidation math overflows or underflows — e.g., `collateralNeeded = debt / price` becomes enormous, exceeding available collateral. The liquidation function reverts, making the position unliquidatable and locking bad debt. -- **FP:** Liquidation caps `collateralSeized` at position's total collateral. Graceful handling of underwater positions (full seizure, remaining bad debt socialized). +- **D:** External call (`call{value:}`, `safeTransfer`, etc.) before state update — check-external-effect instead of check-effect-external (CEI). +- **FP:** State updated before call (CEI followed). `nonReentrant` modifier. Callee is hardcoded immutable with known-safe receive/fallback. -**188. Borrower Front-Runs Liquidation** +**51. Diamond Proxy Cross-Facet Storage Collision** -- **D:** Borrower observes pending `liquidate()` tx in mempool, front-runs with minimal repayment or collateral top-up to push health factor above threshold. Immediately re-borrows after liquidation tx fails. Repeated indefinitely to maintain risky position. -- **FP:** Liquidation uses flash-loan-resistant health check (same-block deposit doesn't count). Minimum repayment cooldown. Dutch auction liquidation (no fixed threshold to game). +- **D:** EIP-2535 Diamond facets declare storage variables without EIP-7201 namespaced storage. Multiple facets independently start at slot 0, writing to same slots. +- **FP:** All facets use single `DiamondStorage` struct at namespaced position (EIP-7201). No top-level state variables in facets. -**189. Liquidation Discount Applied Inconsistently Across Code Paths** +**52. Force-Feeding ETH via selfdestruct / Coinbase / CREATE2 Pre-Funding** -- **D:** One code path calculates debt at face value, another applies a liquidation discount. When the discounted amount is subtracted from the non-discounted amount, the result underflows or leaves residual bad debt unaccounted. -- **FP:** Discount applied consistently in all paths touching liquidation accounting. Single source of truth for discounted value. +- **D:** Contract uses `address(this).balance` for accounting or gates logic on exact balance (e.g., `require(balance == totalDeposits)`). `selfdestruct(target)`, coinbase rewards, or pre-computed `CREATE2` deposits force ETH in without calling `receive()`/`fallback()`, breaking invariants. +- **FP:** Internal accounting only (`totalDeposited` state variable, never reads `address(this).balance`). Contract designed to accept arbitrary ETH (e.g., WETH wrapper). -**190. No Buffer Between Max LTV and Liquidation Threshold** +**53. Wrong Price Feed for Derivative or Wrapped Asset** -- **D:** Max borrowable LTV equals the liquidation threshold. A borrower at max LTV is immediately liquidatable on any adverse price tick. Combined with origination fees, positions can be born underwater. -- **FP:** Max LTV is meaningfully lower than liquidation threshold (e.g., 80% vs 85%). Origination fee deducted from borrowed amount, not added to debt. +- **D:** Protocol uses ETH/USD feed to price stETH collateral, or BTC/USD feed for WBTC. During normal conditions the error is small, but during depeg events the mispricing enables undercollateralized borrows or incorrect liquidations. +- **FP:** Dedicated feed for the actual derivative asset (e.g., stETH/USD, WBTC/BTC). Deviation check against secondary oracle. Protocol documentation explicitly accepts depeg risk. -**191. Same-Block Vote-Transfer-Vote** +**54. ERC4626 Preview Rounding Direction Violation** -- **D:** Governance reads voting power at current block, not a past snapshot. User votes, transfers tokens to a second wallet in the same block, and votes again — doubling their effective vote weight. -- **FP:** `getPastVotes(block.number - 1)` or proposal-creation snapshot. Voting power locked on first vote until proposal closes. ERC20Votes with checkpoint-based historical balances. +- **D:** `previewDeposit` returns more shares than `deposit` mints, or `previewMint` charges fewer assets than `mint`. Custom `_convertToShares`/`_convertToAssets` with wrong `Math.mulDiv` rounding direction. +- **FP:** OZ ERC4626 base without overriding conversion functions. Custom impl explicitly uses `Floor` for share issuance, `Ceil` for share burning. -**192. Quorum Computed from Live Supply, Not Snapshot** +**55. Transparent Proxy Admin Routing Confusion** -- **D:** `quorum = totalSupply() * quorumBps / 10000` reads current supply. After a proposal is created, an attacker mints tokens (or deposits to inflate supply), lowering the effective quorum percentage needed to pass. -- **FP:** Quorum snapshotted at proposal creation: `totalSupply(proposalSnapshot)`. Fixed absolute quorum amount. Supply changes do not affect active proposals. +- **D:** Admin address also used for regular protocol interactions. Calls from admin route to proxy admin functions instead of delegating — silently failing or executing unintended logic. +- **FP:** Dedicated `ProxyAdmin` contract used exclusively for admin calls. OZ `TransparentUpgradeableProxy` enforces separate admin. diff --git a/solidity-auditor/references/attack-vectors/attack-vectors-2.md b/solidity-auditor/references/attack-vectors/attack-vectors-2.md index f4dbe25..605653e 100644 --- a/solidity-auditor/references/attack-vectors/attack-vectors-2.md +++ b/solidity-auditor/references/attack-vectors/attack-vectors-2.md @@ -1,298 +1,280 @@ # Attack Vectors Reference (2/4) -219 total attack vectors +220 total attack vectors --- -**43. Self-Liquidation Profit Extraction** - -- **D:** Borrower liquidates their own undercollateralized position from a second address, collecting the liquidation bonus/discount on their own collateral. Profitable whenever liquidation incentive exceeds the cost of being slightly undercollateralized. -- **FP:** `require(msg.sender != borrower)` on liquidation. Liquidation penalty exceeds any collateral bonus. Liquidation incentive is small enough that self-liquidation is net-negative after gas. - -**44. State-Time Lag Exploitation (lzRead Stale State)** - -- **D:** `lzRead` queries state on a remote chain, but there is a latency window between query and delivery of the result via `lzReceive`. During this window, the queried state may change (token transferred, position closed, price moved). Protocol makes irreversible decisions based on the stale read result. -- **FP:** Read targets immutable or slowly-changing state (contract code, historical data). Read result treated as a hint with on-chain re-validation. Time-sensitive operations require fresh on-chain state, not cross-chain reads. - -**45. Integer Overflow / Underflow** - -- **D:** Arithmetic in `unchecked {}` (>=0.8) without prior bounds check: subtraction without `require(amount <= balance)`, large multiplications. Any arithmetic in <0.8 without SafeMath. -- **FP:** Range provably bounded by earlier checks in same function. `unchecked` only for `++i` loop increments where `i < arr.length`. - -**46. Function Selector Clashing (Proxy Backdoor)** - -- **D:** Proxy contains a function whose 4-byte selector collides with an implementation function. User calls route to proxy logic instead of delegating. -- **FP:** Transparent proxy pattern separates admin/user routing. UUPS proxy has no custom functions — all calls delegate. - -**47. OFT Shared Decimals Truncation (uint64 Overflow)** - -- **D:** OFT converts between local decimals and shared decimals (typically 6). `_toSD()` casts to `uint64`. If `sharedDecimals >= localDecimals` (both 18), no decimal conversion occurs but the `uint64` cast silently truncates amounts exceeding ~18.4e18. Also: custom fee logic applied BEFORE `_removeDust()` produces incorrect fee calculations. -- **FP:** Standard OFT with `sharedDecimals = 6` and `localDecimals = 18` (default). Fee logic applied after dust removal. Transfer amounts validated against `uint64.max` before conversion. - -**48. UUPS Upgrade Logic Removed in New Implementation** - -- **D:** New UUPS implementation doesn't inherit `UUPSUpgradeable` or removes `upgradeTo`/`upgradeToAndCall`. Proxy permanently loses upgrade capability. Pattern: V2 missing `_authorizeUpgrade` override. -- **FP:** Every version inherits `UUPSUpgradeable`. Tests verify `upgradeTo` works after each upgrade. OZ upgrades plugin checks in CI. - -**49. ERC721 onERC721Received Arbitrary Caller Spoofing** - -- **D:** `onERC721Received` uses parameters (`from`, `tokenId`) to update state without verifying `msg.sender` is the expected NFT contract. Anyone calls directly with fabricated parameters. -- **FP:** `require(msg.sender == address(nft))` before state update. Function is view-only or reverts unconditionally. - -**50. ERC1155 totalSupply Inflation via Reentrancy Before Supply Update** - -- **D:** `totalSupply[id]` incremented AFTER `_mint` callback. During `onERC1155Received`, `totalSupply` is stale-low, inflating caller's share in any supply-dependent formula. Ref: OZ GHSA-9c22-pwxw-p6hx (2021). -- **FP:** OZ >= 4.3.2 (patched ordering). `nonReentrant` on all mint functions. No supply-dependent logic callable from mint callback. - -**51. Missing Nonce (Signature Replay)** - -- **D:** Signed message has no per-user nonce, or nonce present but never stored/incremented after use. Same signature resubmittable. -- **FP:** Monotonic per-signer nonce in signed payload, checked and incremented atomically. Or `usedSignatures[hash]` mapping. - -**52. Single-Function Reentrancy** - -- **D:** External call (`call{value:}`, `safeTransfer`, etc.) before state update — check-external-effect instead of check-effect-external (CEI). -- **FP:** State updated before call (CEI followed). `nonReentrant` modifier. Callee is hardcoded immutable with known-safe receive/fallback. - ---- - ---- - -**53. Diamond Proxy Cross-Facet Storage Collision** - -- **D:** EIP-2535 Diamond facets declare storage variables without EIP-7201 namespaced storage. Multiple facets independently start at slot 0, writing to same slots. -- **FP:** All facets use single `DiamondStorage` struct at namespaced position (EIP-7201). No top-level state variables in facets. - -**54. Force-Feeding ETH via selfdestruct / Coinbase / CREATE2 Pre-Funding** - -- **D:** Contract uses `address(this).balance` for accounting or gates logic on exact balance (e.g., `require(balance == totalDeposits)`). `selfdestruct(target)`, coinbase rewards, or pre-computed `CREATE2` deposits force ETH in without calling `receive()`/`fallback()`, breaking invariants. -- **FP:** Internal accounting only (`totalDeposited` state variable, never reads `address(this).balance`). Contract designed to accept arbitrary ETH (e.g., WETH wrapper). - -**55. Wrong Price Feed for Derivative or Wrapped Asset** - -- **D:** Protocol uses ETH/USD feed to price stETH collateral, or BTC/USD feed for WBTC. During normal conditions the error is small, but during depeg events the mispricing enables undercollateralized borrows or incorrect liquidations. -- **FP:** Dedicated feed for the actual derivative asset (e.g., stETH/USD, WBTC/BTC). Deviation check against secondary oracle. Protocol documentation explicitly accepts depeg risk. - -**56. ERC4626 Preview Rounding Direction Violation** - -- **D:** `previewDeposit` returns more shares than `deposit` mints, or `previewMint` charges fewer assets than `mint`. Custom `_convertToShares`/`_convertToAssets` with wrong `Math.mulDiv` rounding direction. -- **FP:** OZ ERC4626 base without overriding conversion functions. Custom impl explicitly uses `Floor` for share issuance, `Ceil` for share burning. - - -**58. Transparent Proxy Admin Routing Confusion** - -- **D:** Admin address also used for regular protocol interactions. Calls from admin route to proxy admin functions instead of delegating — silently failing or executing unintended logic. -- **FP:** Dedicated `ProxyAdmin` contract used exclusively for admin calls. OZ `TransparentUpgradeableProxy` enforces separate admin. - -**59. Cross-Chain Address Ownership Variance** +**56. Cross-Chain Address Ownership Variance** - **D:** Same address has different owners on different chains (EOA private key not used on all chains, or `CREATE`-deployed contract at same nonce but different deployer). Cross-chain logic that assumes `address(X) on Chain A == address(X) on Chain B` implies same owner enables impersonation. Pattern: `lzRead` checking `ownerOf(tokenId)` cross-chain and granting rights to the same address locally. - **FP:** `CREATE2`-deployed contracts with same factory + salt are safe. Peer mapping explicitly binds (chainId, address) pairs. Authorization uses cross-chain messaging (not address equality) to prove ownership. -**60. Read-Only Reentrancy** +**57. Read-Only Reentrancy** - **D:** Protocol calls `view` function (`get_virtual_price()`, `totalAssets()`) on external contract from within a callback. External contract has no reentrancy guard on view functions — returns transitional/manipulated value mid-execution. - **FP:** External view functions are `nonReentrant`. Chainlink oracle used instead. External contract's reentrancy lock checked before calling view. -**61. Bytecode Verification Mismatch** +**58. Bytecode Verification Mismatch** - **D:** Verified source doesn't match deployed bytecode behavior: different compiler settings, obfuscated constructor args, or `--via-ir` vs legacy pipeline mismatch. No reproducible build (no pinned compiler in config). - **FP:** Deterministic build with pinned compiler/optimizer in committed config. Verification in deployment script (Foundry `--verify`). Sourcify full match. Constructor args published. -**62. Scratch Space Corruption Across Assembly Blocks** +**59. Scratch Space Corruption Across Assembly Blocks** - **D:** Data written to scratch space (`0x00`–`0x3f`) in one assembly block is expected to persist and be read in a later assembly block, but intervening Solidity code (or compiler-generated code for `keccak256`, `abi.encode`, etc.) overwrites scratch space between the two blocks. Pattern: `mstore(0x00, key); mstore(0x20, slot)` in block A, then `keccak256(0x00, 0x40)` in block B with Solidity statements between them. - **FP:** All scratch space reads occur within the same contiguous assembly block as the writes. Developer explicitly rewrites scratch space before each use. No intervening Solidity code between blocks. ---- - ---- - -**63. ERC1155 Custom Burn Without Caller Authorization** +**60. ERC1155 Custom Burn Without Caller Authorization** - **D:** Public `burn(address from, uint256 id, uint256 amount)` callable by anyone without verifying `msg.sender == from` or operator approval. Any caller burns another user's tokens. - **FP:** `require(from == msg.sender || isApprovedForAll(from, msg.sender))` before `_burn`. OZ `ERC1155Burnable` used. -**64. ERC721 Unsafe Transfer to Non-Receiver** +**61. ERC721 Unsafe Transfer to Non-Receiver** - **D:** `_transfer()`/`_mint()` used instead of `_safeTransfer()`/`_safeMint()`, sending NFTs to contracts without `IERC721Receiver`. Tokens permanently locked. - **FP:** All paths use `safeTransferFrom`/`_safeMint`. Function is `nonReentrant`. -**65. ERC1155 Fungible / Non-Fungible Token ID Collision** +**62. ERC1155 Fungible / Non-Fungible Token ID Collision** - **D:** ERC1155 represents both fungible and unique items with no enforcement: missing `require(totalSupply(id) == 0)` before NFT mint, or no cap preventing additional copies of supply-1 IDs. - **FP:** `require(totalSupply(id) + amount <= maxSupply(id))` with `maxSupply=1` for NFTs. Fungible/NFT ID ranges disjoint and enforced. Role tokens non-transferable. -**66. ERC4626 Deposit/Withdraw Share-Count Asymmetry** +**63. ERC4626 Deposit/Withdraw Share-Count Asymmetry** - **D:** `_convertToShares` uses `Rounding.Floor` for both deposit and withdraw paths. `withdraw(a)` burns fewer shares than `deposit(a)` minted, manufacturing free shares. Single rounding helper called on both paths without distinct rounding args. - **FP:** `deposit` uses `Floor`, `withdraw` uses `Ceil` (vault-favorable both directions). OZ ERC4626 without custom conversion overrides. -**67. ERC4626 Mint/Redeem Asset-Cost Asymmetry** +**64. ERC4626 Mint/Redeem Asset-Cost Asymmetry** - **D:** `redeem(s)` returns more assets than `mint(s)` costs — cycling yields net profit. Root cause: `_convertToAssets` rounds up in `redeem` and down in `mint` (opposite of EIP-4626 spec). Pattern: `previewRedeem` uses `Rounding.Ceil`, `previewMint` uses `Rounding.Floor`. - **FP:** `redeem` uses `Math.Rounding.Floor`, `mint` uses `Math.Rounding.Ceil`. OZ ERC4626 without custom conversion overrides. -**68. ERC1155 Batch Transfer Partial-State Callback Window** +**65. ERC1155 Batch Transfer Partial-State Callback Window** - **D:** Custom batch mint/transfer updates `_balances` and calls `onERC1155Received` per ID in loop, instead of committing all updates first then calling `onERC1155BatchReceived` once. Callback reads stale balances for uncredited IDs. - **FP:** All balance updates committed before any callback (OZ pattern). `nonReentrant` on all transfer/mint entry points. -**69. Chainlink Staleness / No Validity Checks** +**66. Chainlink Staleness / No Validity Checks** - **D:** `latestRoundData()` called but missing checks: `answer > 0`, `updatedAt > block.timestamp - MAX_STALENESS`, `answeredInRound >= roundId`, fallback on failure. - **FP:** All four checks present. Circuit breaker or fallback oracle on failure. -**70. Unsafe Downcast / Integer Truncation** +**67. Unsafe Downcast / Integer Truncation** - **D:** `uint128(largeUint256)` without bounds check. Solidity >= 0.8 silently truncates on downcast (no revert). Dangerous in price feeds, share calculations, timestamps. - **FP:** `require(x <= type(uint128).max)` before cast. OZ `SafeCast` used. -**71. Missing `enforcedOptions` — Insufficient Gas for lzReceive** +**68. Missing `enforcedOptions` — Insufficient Gas for lzReceive** - **D:** OApp does not call `setEnforcedOptions()` to mandate minimum gas for destination execution. User-supplied `_options` can specify insufficient gas, causing `lzReceive` to revert on destination. Funds debited on source but not credited on destination — stuck in limbo until manual recovery via `lzComposeAlert` or `skipPayload`. - **FP:** `enforcedOptions` configured with tested gas limits for each message type. `lzReceive` logic is simple (single mapping update) requiring minimal gas. Executor provides guaranteed minimum gas. -**72. Nonce Gap from Reverted Transactions (CREATE Address Mismatch)** +**69. Nonce Gap from Reverted Transactions (CREATE Address Mismatch)** - **D:** Deployment script uses `CREATE` and pre-computes addresses from deployer nonce. Reverted/extra tx advances nonce — subsequent deployments land at wrong addresses. Pre-configured references point to empty/wrong contracts. - **FP:** `CREATE2` used (nonce-independent). Script reads nonce from chain before computing. Addresses captured from deployment receipts, not pre-assumed. ---- - ---- - -**73. Fee-on-Transfer Token Accounting** +**70. Fee-on-Transfer Token Accounting** - **D:** Deposit records `deposits[user] += amount` then `transferFrom(..., amount)`. Fee-on-transfer tokens cause contract to receive less than recorded. - **FP:** Balance measured before/after: `uint256 before = token.balanceOf(this); transferFrom(...); received = balanceOf(this) - before;` and `received` used for accounting. -**74. Assembly Delegatecall Missing Return/Revert Propagation** +**71. Assembly Delegatecall Missing Return/Revert Propagation** - **D:** Proxy fallback written in assembly performs `delegatecall` but omits one or more required steps: (1) not copying full calldata via `calldatacopy`, (2) not copying return data via `returndatacopy(0, 0, returndatasize())`, (3) not branching on the result to `return(0, returndatasize())` on success or `revert(0, returndatasize())` on failure. Silent failures or swallowed reverts. - **FP:** Complete proxy pattern: `calldatacopy(0, 0, calldatasize())` → `delegatecall(gas(), impl, 0, calldatasize(), 0, 0)` → `returndatacopy(0, 0, returndatasize())` → `switch result case 0 { revert(0, returndatasize()) } default { return(0, returndatasize()) }`. OZ Proxy.sol used. -**75. Merkle Tree Second Preimage Attack** +**72. Merkle Tree Second Preimage Attack** - **D:** `MerkleProof.verify(proof, root, leaf)` where leaf derived from user input without double-hashing or type-prefixing. 64-byte input (two sibling hashes) passes as intermediate node. - **FP:** Leaves double-hashed or include type prefix. Input length enforced != 64 bytes. OZ MerkleProof >= v4.9.2 with sorted-pair variant. -**76. Dirty Higher-Order Bits on Sub-256-Bit Types** +**73. Dirty Higher-Order Bits on Sub-256-Bit Types** - **D:** Assembly loads a value as a full 32-byte word (`calldataload`, `sload`, `mload`) but treats it as a smaller type (`address`, `uint128`, `uint8`, `bool`) without masking upper bits. Dirty bits cause incorrect comparisons, mapping key mismatches, or storage corruption. Pattern: `let addr := calldataload(4)` used directly without `and(addr, 0xffffffffffffffffffffffffffffffffffffffff)`. - **FP:** Explicit bitmask applied: `and(value, mask)` immediately after load. Value produced by a prior Solidity expression that already cleaned it. `shr(96, calldataload(offset))` pattern that naturally zeros upper bits for addresses. - -**78. Returndatasize-as-Zero Assumption** +**74. Returndatasize-as-Zero Assumption** - **D:** Assembly uses `returndatasize()` as a gas-cheap substitute for `push 0` (saves 1 gas). If a prior `call`/`staticcall` in the same execution context returned data, `returndatasize()` is nonzero, corrupting the intended zero value. Pattern: `let ptr := returndatasize()` or `mstore(returndatasize(), value)` after an external call has been made. - **FP:** `returndatasize()` used as zero only at the very start of execution before any external calls. Used immediately after a controlled call where the return size is known. Used as an actual size measurement (its intended purpose). -**79. tx.origin Authentication** +**75. tx.origin Authentication** - **D:** `require(tx.origin == owner)` used for auth. Phishable via intermediary contract. - **FP:** `tx.origin == msg.sender` used only as anti-contract check, not auth. -**80. ERC20 Non-Compliant: Return Values / Events** +**76. ERC20 Non-Compliant: Return Values / Events** - **D:** Custom `transfer()`/`transferFrom()` doesn't return `bool` or always returns `true` on failure. `mint()`/`burn()` missing `Transfer` events. `approve()` missing `Approval` event. - **FP:** OZ `ERC20.sol` base with no custom overrides of transfer/approve/event logic. -**81. ERC721Enumerable Index Corruption on Burn or Transfer** +**77. ERC721Enumerable Index Corruption on Burn or Transfer** - **D:** Override of `_beforeTokenTransfer` (OZ v4) or `_update` (OZ v5) without calling `super`. Index structures (`_ownedTokens`, `_allTokens`) become stale — `tokenOfOwnerByIndex` returns wrong IDs, `totalSupply` diverges. - **FP:** Override always calls `super` as first statement. Contract doesn't inherit `ERC721Enumerable`. -**82. Block Stuffing / Gas Griefing on Subcalls** +**78. Block Stuffing / Gas Griefing on Subcalls** -- **D:** Time-sensitive function blockable by filling blocks. For relayer gas-forwarding griefing (63/64 rule), see Vector 30. +- **D:** Time-sensitive function blockable by filling blocks. Separate concern: relayer gas-forwarding griefing via 63/64 rule where subcall silently fails but outer tx succeeds. - **FP:** Function not time-sensitive or window long enough that block stuffing is economically infeasible. ---- - ---- - -**83. ERC777 tokensToSend / tokensReceived Reentrancy** +**79. ERC777 tokensToSend / tokensReceived Reentrancy** - **D:** Token `transfer()`/`transferFrom()` called before state updates on a token that may implement ERC777 (ERC1820 registry). ERC777 hooks fire on ERC20-style calls, enabling reentry from sender's `tokensToSend` or recipient's `tokensReceived`. - **FP:** CEI — all state committed before transfer. `nonReentrant` on all entry points. Token whitelist excludes ERC777. -**84. Rebasing / Elastic Supply Token Accounting** +**80. Rebasing / Elastic Supply Token Accounting** - **D:** Contract holds rebasing tokens (stETH, AMPL, aTokens) and caches `balanceOf(this)`. After rebase, cached value diverges from actual balance. - **FP:** Rebasing tokens blocked at code level (revert/whitelist). Accounting reads `balanceOf` live. Wrapper tokens (wstETH) used. -**193. Checkpoint Overwrite on Same-Block Operations** +**81. Assembly Arithmetic Silent Overflow and Division-by-Zero** + +- **D:** Arithmetic inside `assembly {}` (Yul) does not revert on overflow/underflow (wraps like `unchecked`) and division by zero returns 0 instead of reverting. Developers accustomed to Solidity 0.8 checked math may not expect this. +- **FP:** Manual overflow checks in assembly (`if gt(result, x) { revert(...) }`). Denominator checked before `div`. Assembly block is read-only (`mload`/`sload` only, no arithmetic). + +**82. Flash Loan-Assisted Price Manipulation** + +- **D:** Function reads price from on-chain source (AMM reserves, vault `totalAssets()`) manipulable atomically via flash loan + swap in same tx. +- **FP:** TWAP with >= 30min window. Multi-block cooldown between price reads. Separate-block enforcement. + +**83. Non-Standard Approve Behavior (Zero-First / Max-Approval Revert)** + +- **D:** (a) USDT-style: `approve()` reverts when changing from non-zero to non-zero allowance, requiring `approve(0)` first. (b) Some tokens (UNI, COMP) revert on `approve(type(uint256).max)`. Protocol calls `token.approve(spender, amount)` directly without these accommodations. +- **FP:** OZ `SafeERC20.forceApprove()` or `safeIncreaseAllowance()` used. Allowance always set from zero (fresh per-tx approval). Token whitelist excludes non-standard tokens. + +**84. Missing Chain ID Validation in Deployment Configuration** + +- **D:** Deploy script reads `$RPC_URL` from `.env` without `eth_chainId` assertion. Foundry script without `--chain-id` flag or `block.chainid` check. No dry-run before broadcast. +- **FP:** `require(block.chainid == expectedChainId)` at script start. CI validates chain ID before deployment. + +**85. Array `delete` Leaves Zero-Value Gap Instead of Removing Element** + +- **D:** `delete array[index]` resets element to zero but does not shrink the array or shift subsequent elements. Iteration logic treats the zeroed slot as a valid entry — phantom zero-address recipients, skipped distributions, or inflated `length`. +- **FP:** Swap-and-pop pattern used (`array[index] = array[length - 1]; array.pop()`). Iteration skips zero entries explicitly. EnumerableSet or similar library used. + +**86. Governance Flash-Loan Upgrade Hijack** + +- **D:** Proxy upgrades via governance using current-block vote weight (`balanceOf` or `getPastVotes(block.number)`). No voting delay or timelock. Flash-borrow, vote, execute upgrade in one tx. +- **FP:** `getPastVotes(block.number - 1)`. Timelock 24-72h. High quorum thresholds. Staking lockup required. + +**87. Write to Arbitrary Storage Location** + +- **D:** (1) `sstore(slot, value)` where `slot` derived from user input without bounds. (2) Solidity <0.6: direct `arr.length` assignment + indexed write at crafted large index wraps slot arithmetic. +- **FP:** Assembly is read-only (`sload` only). Slot is compile-time constant or non-user-controlled. Solidity >= 0.6 used. + +**88. Insufficient Return Data Length Validation** + +- **D:** Assembly `staticcall`/`call` writes return data into a fixed-size buffer (e.g., `staticcall(gas(), token, ptr, 4, ptr, 32)`) then reads `mload(ptr)` without checking `returndatasize() >= 32`. If the target is an EOA (no code, zero return data) or a non-compliant contract returning fewer bytes, `mload` reads stale memory at `ptr`, which may decode as a truthy value — silently treating a failed/absent call as success. +- **FP:** `if lt(returndatasize(), 32) { revert(0,0) }` checked before reading return data. `extcodesize(target)` verified > 0 before call. Safe ERC20 pattern that handles both zero-length and 32-byte returns. Ref: Consensys Diligence — 0x Exchange bug (real exploit from missing return data length check). + +**89. Chainlink Feed Deprecation / Wrong Decimal Assumption** + +- **D:** (a) Chainlink aggregator address hardcoded/immutable with no update path — deprecated feed returns stale/zero price. (b) Assumes `feed.decimals() == 8` without runtime check — some feeds return 18 decimals, causing 10^10 scaling error. +- **FP:** Feed address updatable via governance. `feed.decimals()` called and used for normalization. Secondary oracle deviation check. + +**90. Upgrade Race Condition / Front-Running** + +- **D:** `upgradeTo(V2)` and post-upgrade config calls are separate txs in public mempool. Window for front-running (exploit old impl) or sandwiching between upgrade and config. +- **FP:** `upgradeToAndCall()` bundles upgrade + init. Private mempool (Flashbots Protect). V2 safe with V1 state from block 0. Timelock makes execution predictable. + +**91. Missing or Expired Deadline on Swaps** + +- **D:** `deadline = block.timestamp` (always valid), `deadline = type(uint256).max`, or no deadline. Tx holdable in mempool indefinitely. +- **FP:** Deadline is calldata parameter validated as `require(deadline >= block.timestamp)`, not derived from `block.timestamp` internally. + +**92. Deployment Transaction Front-Running (Ownership Hijack)** + +- **D:** Deployment tx sent to public mempool without private relay. Attacker extracts bytecode and deploys first (or front-runs initialization). Pattern: `eth_sendRawTransaction` via public RPC, constructor sets `owner = msg.sender`. +- **FP:** Private relay used (Flashbots Protect, MEV Blocker). Owner passed as constructor arg, not `msg.sender`. Chain without public mempool. CREATE2 salt tied to deployer. + +**93. Duplicate Items in User-Supplied Array** + +- **D:** Function accepts array parameter (e.g., `claimRewards(uint256[] calldata tokenIds)`) without checking for duplicates. User passes same ID multiple times, claiming rewards/voting/withdrawing repeatedly in one call. +- **FP:** Duplicate check via mapping (`require(!seen[id]); seen[id] = true`). Sorted-unique input enforced (`require(ids[i] > ids[i-1])`). State zeroed on first claim (second iteration reverts naturally). + +**94. Transient Storage Low-Gas Reentrancy (EIP-1153)** + +- **D:** Contract uses `transfer()`/`send()` (2300-gas) as reentrancy guard + uses `TSTORE`/`TLOAD`. Post-Cancun, `TSTORE` succeeds under 2300 gas unlike `SSTORE`. Second pattern: transient reentrancy lock not cleared at call end — persists for entire tx, causing DoS via multicall/flash loan callback. +- **FP:** `nonReentrant` backed by regular storage slot (or transient mutex properly cleared). CEI followed unconditionally. + +**95. Calldataload / Calldatacopy Out-of-Bounds Read** + +- **D:** `calldataload(offset)` where `offset` is user-controlled or exceeds actual calldata length. Reading past `calldatasize()` returns zero-padded bytes silently (no revert), producing phantom zero values that pass downstream logic as valid inputs. Pattern: `calldataload(add(4, mul(index, 32)))` without `require(index < paramCount)`. +- **FP:** `calldatasize()` validated before assembly block (e.g., Solidity ABI decoder already checked). Static offsets into known fixed-length function signatures. Explicit `if lt(calldatasize(), minExpected) { revert(0,0) }` guard. + +**96. Banned Opcode in Validation Phase (Simulation-Execution Divergence)** -- **D:** Multiple delegate/transfer operations in the same block call `_writeCheckpoint()` with the same `block.number` key. Each overwrites the previous — binary search returns the first (incomplete) checkpoint, losing intermediate state. -- **FP:** Checkpoint appends only when `block.number > lastCheckpoint.blockNumber`. Same-block operations accumulate into the existing checkpoint. Off-chain indexer used instead of on-chain lookups. +- **D:** `validateUserOp`/`validatePaymasterUserOp` references `block.timestamp`, `block.number`, `block.coinbase`, `block.prevrandao`, `block.basefee`. Per ERC-7562, banned in validation — values differ between simulation and execution. +- **FP:** Banned opcodes only in execution phase (`execute`/`executeBatch`). Entity is staked under ERC-7562 reputation system. -**194. Self-Delegation Doubles Voting Power** +**97. Deployer Privilege Retention Post-Deployment** -- **D:** Delegating to self adds votes to the delegate (self) but does not subtract the undelegated balance. Voting power is counted twice — once as held tokens, once as delegated votes. -- **FP:** Delegation logic subtracts from holder's direct balance when adding to delegate. Self-delegation is a no-op or explicitly handled. OZ Votes implementation used correctly. +- **D:** Deployer EOA retains owner/admin/minter/pauser/upgrader after deployment script completes. Pattern: `Ownable` sets `owner = msg.sender` with no `transferOwnership()`. `AccessControl` grants `DEFAULT_ADMIN_ROLE` to deployer with no `renounceRole()`. +- **FP:** Script includes `transferOwnership(multisig)`. Admin role granted to timelock/governance, deployer renounces. `Ownable2Step` with pending owner set to multisig. -**195. Nonce Not Incremented on Reverted Execution** +**98. Non-Atomic Multi-Contract Deployment (Partial System Bootstrap)** -- **D:** Meta-transaction or permit nonce is checked before execution but only incremented on success. If the inner call reverts (after nonce check, before increment), the same signed message can be replayed until it eventually succeeds. -- **FP:** Nonce incremented before execution (check-effects-interaction). Nonce incremented in both success and failure paths. Deadline-based expiry on signed messages. +- **D:** Deployment script deploys interdependent contracts across separate transactions. Midway failure leaves half-deployed state with missing references or unwired contracts. Pattern: multiple `vm.broadcast()` blocks or sequential `await deploy()` with no idempotency checks. +- **FP:** Single `vm.startBroadcast()`/`vm.stopBroadcast()` block. Factory deploys+wires all in one tx. Script is idempotent. Hardhat-deploy with resumable migrations. -**197. Bridge Global Rate Limit Griefing** +**99. CREATE / CREATE2 Deployment Failure Silently Returns Zero** -- **D:** Bridge enforces a global throughput cap (total value per window) not segmented by user. Attacker fills the rate limit by bridging cheap tokens back and forth, blocking all legitimate users from bridging during the cooldown window. -- **FP:** Per-user rate limits. Rate limit segmented by token or route. Whitelist for high-value transfers. No global rate limit. +- **D:** Assembly `create(v, offset, size)` or `create2(v, offset, size, salt)` returns `address(0)` on failure (insufficient balance, collision, init code revert) but the code does not check for zero. The zero address is stored or used, and subsequent calls to `address(0)` silently succeed as no-ops (no code) or interact with precompiles. +- **FP:** Immediate check: `if iszero(addr) { revert(0, 0) }` after create/create2. Address validated downstream before any state-dependent operation. -**199. Self-Matched Orders Enable Wash Trading** +**100. ERC721 / ERC1155 Type Confusion in Dual-Standard Marketplace** -- **D:** Order matching does not verify `maker != taker`. A user submits both sides of a trade to farm trading rewards, inflate volume metrics, bypass royalties (NFT), or extract fee rebates. -- **FP:** `require(makerOrder.signer != takerOrder.signer)`. Volume-based rewards use time-weighted averages resistant to single-block spikes. Royalty enforced regardless of counterparty. +- **D:** Shared `buy`/`fill` function uses type flag for ERC721/ERC1155. `quantity` accepted for ERC721 without requiring == 1. `price * quantity` with `quantity = 0` yields zero payment. Ref: TreasureDAO (2022). +- **FP:** ERC721 branch `require(quantity == 1)`. Separate code paths for ERC721/ERC1155. -**200. Dutch Auction Price Decay Underflow** +**101. Cross-Contract Reentrancy** -- **D:** `currentPrice = startPrice - (decayRate * elapsed)`. When the auction runs past the point where price should reach zero, the subtraction underflows — reverting on 0.8+ or wrapping to `type(uint256).max` on <0.8. Auction becomes unfinishable. -- **FP:** `currentPrice = elapsed >= duration ? reservePrice : startPrice - (decayRate * elapsed)`. Floor price enforced. `min()` used to cap decay. +- **D:** Two contracts share logical state (balances in A, collateral check in B). A makes external call before syncing state B reads. A's `ReentrancyGuard` doesn't protect B. +- **FP:** State B reads is synchronized before A's external call. No re-entry path from A's callee into B. -**201. Timelock Anchored to Deployment, Not Action** +**102. Non-Atomic Proxy Initialization (Front-Running `initialize()`)** -- **D:** Recovery or admin timelock measured from contract deployment or initialization, not from when the action was queued. Once the initial delay elapses, all future actions can execute instantly — the timelock is effectively permanent bypass. -- **FP:** Timelock resets on each queued action. `executeAfter = block.timestamp + delay` set at queue time. OZ TimelockController pattern. +- **D:** Proxy deployed in one tx, `initialize()` called in separate tx. Uninitialized proxy front-runnable. Pattern: `new TransparentUpgradeableProxy(impl, admin, "")` with empty data, separate `initialize()`. Ref: Wormhole (2022). +- **FP:** Proxy constructor receives init calldata atomically: `new TransparentUpgradeableProxy(impl, admin, abi.encodeCall(...))`. OZ `deployProxy()` used. -**202. Withdrawal Rate Limit Bypassed via Share Transfer** +**103. EIP-2981 Royalty Signaled But Never Enforced** -- **D:** Per-address withdrawal limit: `require(withdrawn[msg.sender] + amount <= limitPerPeriod)`. User transfers vault shares to a fresh address and withdraws from there — each new address gets a fresh limit. -- **FP:** Limit tracks the underlying position, not the address. Shares are non-transferable or transfer resets withdrawal allowance. KYC-bound withdrawal limits. +- **D:** `royaltyInfo()` implemented and `supportsInterface(0x2a55205a)` returns true, but transfer/settlement logic never calls `royaltyInfo()` or routes payment. EIP-2981 is advisory only. +- **FP:** Settlement contract reads `royaltyInfo()` and transfers royalty on-chain. Royalties intentionally zero and documented. -**203. Blacklist and Whitelist Not Mutually Exclusive** +**104. Paymaster Gas Penalty Undercalculation** -- **D:** An address can hold both `BLACKLISTED` and `WHITELISTED` roles simultaneously. Whitelist-gated paths do not check the blacklist — a blacklisted address bypasses restrictions by also being whitelisted. -- **FP:** Adding to blacklist auto-removes from whitelist (and vice versa). Single enum role per address. Both checks applied on every restricted path. +- **D:** Paymaster prefund formula omits 10% EntryPoint penalty on unused execution gas (`postOpUnusedGasPenalty`). Large `executionGasLimit` with low usage drains paymaster deposit. +- **FP:** Prefund explicitly adds unused-gas penalty. Conservative overestimation covers worst case. -**204. Dead Code After Return Statement** +**105. ERC721 Approval Not Cleared in Custom Transfer Override** -- **D:** Critical state update or validation (`require(success)`, `emit Event`, `nonce++`) placed after a `return` statement. The code is unreachable — failures go undetected, events are never emitted, state is never updated. -- **FP:** All critical logic precedes `return`. Compiler warnings for unreachable code are addressed. Linter enforces no-unreachable-code rule. +- **D:** Custom `transferFrom` override skips `super._transfer()` or `super.transferFrom()`, missing the `delete _tokenApprovals[tokenId]` step. Previous approval persists under new owner. +- **FP:** Override calls `super.transferFrom` or `super._transfer` internally. Or explicitly deletes approval / calls `_approve(address(0), tokenId, owner)`. -**205. Partial Redemption Fails to Reduce Tracked Total** +**106. DoS via Push Payment to Rejecting Contract** -- **D:** Withdrawal queue partially fills a redemption request but does not proportionally reduce `totalQueuedShares` or `totalPendingAssets`. The vault's tracked total remains inflated, skewing share price for all other depositors. -- **FP:** Partial fill reduces tracked totals proportionally. Queue uses per-request tracking, not a global aggregate. Atomic full-or-nothing redemptions. +- **D:** ETH distribution in a single loop via `recipient.call{value:}("")`. Any reverting recipient blocks entire loop. +- **FP:** Pull-over-push pattern. Loop uses `try/catch` and continues on failure. -**206. TWAP Accumulator Not Updated During Sync or Skim** +**107. Weak On-Chain Randomness** -- **D:** Pool's `sync()` or `skim()` function updates reserves but does not call `_update()` to advance the TWAP cumulative price accumulator. TWAP observations return stale values, enabling price manipulation through sync-then-trade sequences. -- **FP:** `sync()` calls `_update()` before overwriting reserves. TWAP sourced from external oracle, not internal accumulator. Uniswap V3 `observe()` used (accumulator updated on every swap). +- **D:** Randomness from `block.prevrandao`, `blockhash(block.number - 1)`, `block.timestamp`, `block.coinbase`, or combinations. Validator-influenceable or visible before inclusion. +- **FP:** Chainlink VRF v2+. Commit-reveal with future-block reveal and slashing for non-reveal. -**207. Expired Oracle Version Silently Assigned Previous Price** +**108. Delegatecall to Untrusted Callee** -- **D:** In request-commit oracle patterns (Pyth, custom keepers), an expired or unfulfilled price request is assigned the last valid price instead of reverting or returning invalid. Pending orders execute at stale prices rather than being cancelled. -- **FP:** Expired versions return `price = 0` or `valid = false`, forcing order cancellation. Staleness threshold enforced per-request. Fallback oracle used for expired primaries. +- **D:** `address(target).delegatecall(data)` where `target` is user-provided or unconstrained. +- **FP:** `target` is hardcoded immutable verified library address. -**208. Funding Rate Derived from Single Trade Price** +**109. UUPS `_authorizeUpgrade` Missing Access Control** -- **D:** Perpetual swap funding rate uses the last trade price as the mark price. A single self-trade at an extreme price skews the funding rate — the attacker profits from funding payments on their opposing position. -- **FP:** Mark price derived from TWAP or external oracle index. Funding rate capped per period. Volume-weighted average price used. +- **D:** `function _authorizeUpgrade(address) internal override {}` with empty body and no access modifier. Anyone can call `upgradeTo()`. Ref: CVE-2021-41264. +- **FP:** `_authorizeUpgrade()` has `onlyOwner` or equivalent. Multisig/governance controls owner role. -**209. Open Interest Tracked with Pre-Fee Position Size** +**110. Insufficient Block Confirmations / Reorg Double-Spend** -- **D:** Open interest incremented by the full position size before fees are deducted. Actual exposure is smaller than recorded OI. Aggregate OI is permanently inflated, eventually hitting caps and blocking new positions. -- **FP:** OI incremented by post-fee position size. OI decremented on close by same amount used at open. Periodic OI reconciliation. +- **D:** DVN relays cross-chain message before source chain reaches finality. Attacker deposits on source chain, gets minted on destination, then causes a reorg on source chain (or the chain reorgs naturally) to reverse the deposit while keeping minted tokens. Pattern: confirmation count set below chain's known reorg depth (e.g., < 32 blocks on Polygon). +- **FP:** Confirmation count matches or exceeds chain-specific finality guarantees. Chain has fast finality (e.g., Ethereum post-merge ~12 min). DVN waits for finalized blocks. diff --git a/solidity-auditor/references/attack-vectors/attack-vectors-3.md b/solidity-auditor/references/attack-vectors/attack-vectors-3.md index 9d0ab1b..e533731 100644 --- a/solidity-auditor/references/attack-vectors/attack-vectors-3.md +++ b/solidity-auditor/references/attack-vectors/attack-vectors-3.md @@ -1,297 +1,280 @@ # Attack Vectors Reference (3/4) -219 total attack vectors +220 total attack vectors --- -**85. Assembly Arithmetic Silent Overflow and Division-by-Zero** +**111. Nested Mapping Inside Struct Not Cleared on `delete`** -- **D:** Arithmetic inside `assembly {}` (Yul) does not revert on overflow/underflow (wraps like `unchecked`) and division by zero returns 0 instead of reverting. Developers accustomed to Solidity 0.8 checked math may not expect this. -- **FP:** Manual overflow checks in assembly (`if gt(result, x) { revert(...) }`). Denominator checked before `div`. Assembly block is read-only (`mload`/`sload` only, no arithmetic). - -**86. Flash Loan-Assisted Price Manipulation** - -- **D:** Function reads price from on-chain source (AMM reserves, vault `totalAssets()`) manipulable atomically via flash loan + swap in same tx. -- **FP:** TWAP with >= 30min window. Multi-block cooldown between price reads. Separate-block enforcement. - -**87. Non-Standard Approve Behavior (Zero-First / Max-Approval Revert)** - -- **D:** (a) USDT-style: `approve()` reverts when changing from non-zero to non-zero allowance, requiring `approve(0)` first. (b) Some tokens (UNI, COMP) revert on `approve(type(uint256).max)`. Protocol calls `token.approve(spender, amount)` directly without these accommodations. -- **FP:** OZ `SafeERC20.forceApprove()` or `safeIncreaseAllowance()` used. Allowance always set from zero (fresh per-tx approval). Token whitelist excludes non-standard tokens. - -**88. Missing Chain ID Validation in Deployment Configuration** - -- **D:** Deploy script reads `$RPC_URL` from `.env` without `eth_chainId` assertion. Foundry script without `--chain-id` flag or `block.chainid` check. No dry-run before broadcast. -- **FP:** `require(block.chainid == expectedChainId)` at script start. CI validates chain ID before deployment. - -**89. Array `delete` Leaves Zero-Value Gap Instead of Removing Element** - -- **D:** `delete array[index]` resets element to zero but does not shrink the array or shift subsequent elements. Iteration logic treats the zeroed slot as a valid entry — phantom zero-address recipients, skipped distributions, or inflated `length`. -- **FP:** Swap-and-pop pattern used (`array[index] = array[length - 1]; array.pop()`). Iteration skips zero entries explicitly. EnumerableSet or similar library used. +- **D:** `delete myMapping[key]` on struct containing `mapping` or dynamic array. `delete` zeroes primitives but not nested mappings. Reused key exposes stale values. +- **FP:** Nested mapping manually cleared before `delete`. Key never reused after deletion. -**90. Governance Flash-Loan Upgrade Hijack** +**112. ERC721A Lazy Ownership — ownerOf Uninitialized in Batch Range** -- **D:** Proxy upgrades via governance using current-block vote weight (`balanceOf` or `getPastVotes(block.number)`). No voting delay or timelock. Flash-borrow, vote, execute upgrade in one tx. -- **FP:** `getPastVotes(block.number - 1)`. Timelock 24-72h. High quorum thresholds. Staking lockup required. +- **D:** ERC721A/`ERC721Consecutive` batch mint: only first token has ownership written. `ownerOf(id)` for mid-batch IDs may return `address(0)` before any transfer. Access control checking `ownerOf == msg.sender` fails on freshly minted tokens. +- **FP:** Explicit transfer initializes packed slot before ownership check. Standard OZ `ERC721` writes `_owners[tokenId]` per mint. -**91. Write to Arbitrary Storage Location** +**113. Cross-Chain Message Spoofing (Missing Endpoint/Peer Validation)** -- **D:** (1) `sstore(slot, value)` where `slot` derived from user input without bounds. (2) Solidity <0.6: direct `arr.length` assignment + indexed write at crafted large index wraps slot arithmetic. -- **FP:** Assembly is read-only (`sload` only). Slot is compile-time constant or non-user-controlled. Solidity >= 0.6 used. +- **D:** Receiver contract accepts cross-chain messages without verifying `msg.sender == endpoint` and `_origin.sender == registeredPeer[srcChainId]`. Attacker calls the receive function directly with fabricated message data, triggering unauthorized mints/unlocks. +- **FP:** `onlyPeer` modifier or equivalent checks both `msg.sender` (endpoint) and `_origin.sender` (peer). Standard `OAppReceiver._acceptNonce` validates origin. Ref: CrossCurve bridge exploit (Jan 2026) — $3M stolen via spoofed `expressExecute`. -**92. Insufficient Return Data Length Validation** +**114. Proxy Admin Key Compromise** -- **D:** Assembly `staticcall`/`call` writes return data into a fixed-size buffer (e.g., `staticcall(gas(), token, ptr, 4, ptr, 32)`) then reads `mload(ptr)` without checking `returndatasize() >= 32`. If the target is an EOA (no code, zero return data) or a non-compliant contract returning fewer bytes, `mload` reads stale memory at `ptr`, which may decode as a truthy value — silently treating a failed/absent call as success. -- **FP:** `if lt(returndatasize(), 32) { revert(0,0) }` checked before reading return data. `extcodesize(target)` verified > 0 before call. Safe ERC20 pattern that handles both zero-length and 32-byte returns. Ref: Consensys Diligence — 0x Exchange bug (real exploit from missing return data length check). +- **D:** `ProxyAdmin.owner()` returns EOA, not multisig/governance; no timelock on `upgradeTo`. Ref: PAID Network (2021), Ankr (2022). +- **FP:** Multisig (threshold >= 2) + timelock (24-72h). Admin role separate from operational roles. -**93. Chainlink Feed Deprecation / Wrong Decimal Assumption** +**115. Unauthorized Peer Initialization (Fake Peer Attack)** -- **D:** (a) Chainlink aggregator address hardcoded/immutable with no update path — deprecated feed returns stale/zero price. (b) Assumes `feed.decimals() == 8` without runtime check — some feeds return 18 decimals, causing 10^10 scaling error. -- **FP:** Feed address updatable via governance. `feed.decimals()` called and used for normalization. Secondary oracle deviation check. -- **Scope:** Covers feed deprecation and decimal assumptions only. Basic staleness checks (`updatedAt`, `answeredInRound >= roundId`, `answer > 0`) are covered by V69 — do not duplicate findings for missing staleness validation under this vector. +- **D:** `setPeer()` / `setTrustedRemote()` sets the remote peer address that a cross-chain contract trusts. If the owner is compromised or `setPeer` lacks proper access control, an attacker registers a fraudulent peer contract on the source chain that can mint/unlock tokens on the destination without legitimate deposits. Pattern: `setPeer` callable by non-owner, or owner key compromised. +- **FP:** `setPeer` protected by multisig + timelock. Peer addresses verified against known deployment registry. `allowInitializePath()` properly implemented to reject unknown peers. Ref: GAIN token exploit (Sep 2025) — fake peer minted 5B tokens, $3M stolen. -**94. Upgrade Race Condition / Front-Running** +**116. Rounding in Favor of the User** -- **D:** `upgradeTo(V2)` and post-upgrade config calls are separate txs in public mempool. Window for front-running (exploit old impl) or sandwiching between upgrade and config. -- **FP:** `upgradeToAndCall()` bundles upgrade + init. Private mempool (Flashbots Protect). V2 safe with V1 state from block 0. Timelock makes execution predictable. +- **D:** `shares = assets / pricePerShare` rounds down for deposit but up for redeem. Division without explicit rounding direction. First-depositor donation amplifies the error. +- **FP:** `Math.mulDiv` with explicit `Rounding.Up` where vault-favorable. OZ ERC4626 `_decimalsOffset()`. Dead shares at init. ---- +**117. Arbitrary External Call with User-Supplied Target and Calldata** ---- +- **D:** `target.call{value: v}(data)` where `target` or `data` (or both) are caller-supplied parameters. Attacker crafts calldata to invoke unintended functions on the target (e.g., `transferFrom` on an approved ERC20, or `safeTransferFrom` on held NFTs), stealing assets the contract holds or has approvals for. +- **FP:** Target restricted to hardcoded/whitelisted address. Calldata function selector restricted to known-safe set. No token approvals or asset holdings on the calling contract. this covers `call` only, not `delegatecall`. -**95. Missing or Expired Deadline on Swaps** +**118. Paymaster ERC-20 Payment Deferred to postOp Without Pre-Validation** -- **D:** `deadline = block.timestamp` (always valid), `deadline = type(uint256).max`, or no deadline. Tx holdable in mempool indefinitely. -- **FP:** Deadline is calldata parameter validated as `require(deadline >= block.timestamp)`, not derived from `block.timestamp` internally. +- **D:** `validatePaymasterUserOp` doesn't transfer/lock tokens — payment deferred to `postOp` via `safeTransferFrom`. User can revoke allowance between validation and execution; paymaster loses deposit. +- **FP:** Tokens transferred/locked during `validatePaymasterUserOp`. `postOp` only refunds excess. -**96. Deployment Transaction Front-Running (Ownership Hijack)** +**119. Minimal Proxy (EIP-1167) Implementation Destruction** -- **D:** Deployment tx sent to public mempool without private relay. Attacker extracts bytecode and deploys first (or front-runs initialization). Pattern: `eth_sendRawTransaction` via public RPC, constructor sets `owner = msg.sender`. -- **FP:** Private relay used (Flashbots Protect, MEV Blocker). Owner passed as constructor arg, not `msg.sender`. Chain without public mempool. CREATE2 salt tied to deployer. +- **D:** EIP-1167 clones `delegatecall` a fixed implementation. If implementation is destroyed, all clones become no-ops with locked funds. Pattern: `Clones.clone(impl)` where impl has no `selfdestruct` protection or is uninitialized. +- **FP:** No `selfdestruct` in implementation. `_disableInitializers()` in constructor. Post-Dencun: code not destroyed. Beacon proxies used for upgradeability. -**97. Duplicate Items in User-Supplied Array** +**120. Spot Price Oracle from AMM** -- **D:** Function accepts array parameter (e.g., `claimRewards(uint256[] calldata tokenIds)`) without checking for duplicates. User passes same ID multiple times, claiming rewards/voting/withdrawing repeatedly in one call. -- **FP:** Duplicate check via mapping (`require(!seen[id]); seen[id] = true`). Sorted-unique input enforced (`require(ids[i] > ids[i-1])`). State zeroed on first claim (second iteration reverts naturally). +- **D:** Price from AMM reserves: `reserve0 / reserve1`, `getAmountsOut()`, `getReserves()`. Flash-loan exploitable atomically. +- **FP:** TWAP >= 30 min window. Chainlink/Pyth as primary source. -**98. Transient Storage Low-Gas Reentrancy (EIP-1153)** +**121. Missing Slippage Protection (Sandwich Attack)** -- **D:** Contract uses `transfer()`/`send()` (2300-gas) as reentrancy guard + uses `TSTORE`/`TLOAD`. Post-Cancun, `TSTORE` succeeds under 2300 gas unlike `SSTORE`. Second pattern: transient reentrancy lock not cleared at call end — persists for entire tx, causing DoS via multicall/flash loan callback. -- **FP:** `nonReentrant` backed by regular storage slot (or transient mutex properly cleared). CEI followed unconditionally. +- **D:** Swap/deposit/withdrawal with `minAmountOut = 0`, or `minAmountOut` computed on-chain from current pool state. +- **FP:** `minAmountOut` set off-chain by user and validated on-chain. -**99. Calldataload / Calldatacopy Out-of-Bounds Read** +**122. NFT Staking Records msg.sender Instead of ownerOf** -- **D:** `calldataload(offset)` where `offset` is user-controlled or exceeds actual calldata length. Reading past `calldatasize()` returns zero-padded bytes silently (no revert), producing phantom zero values that pass downstream logic as valid inputs. Pattern: `calldataload(add(4, mul(index, 32)))` without `require(index < paramCount)`. -- **FP:** `calldatasize()` validated before assembly block (e.g., Solidity ABI decoder already checked). Static offsets into known fixed-length function signatures. Explicit `if lt(calldatasize(), minExpected) { revert(0,0) }` guard. +- **D:** `depositor[tokenId] = msg.sender` without checking `nft.ownerOf(tokenId)`. Approved operator (not owner) calls stake — transfer succeeds via approval, operator credited as depositor. +- **FP:** Reads `nft.ownerOf(tokenId)` before transfer and records actual owner. Or `require(nft.ownerOf(tokenId) == msg.sender)`. -**100. Banned Opcode in Validation Phase (Simulation-Execution Divergence)** +**123. Missing chainId (Cross-Chain Replay)** -- **D:** `validateUserOp`/`validatePaymasterUserOp` references `block.timestamp`, `block.number`, `block.coinbase`, `block.prevrandao`, `block.basefee`. Per ERC-7562, banned in validation — values differ between simulation and execution. -- **FP:** Banned opcodes only in execution phase (`execute`/`executeBatch`). Entity is staked under ERC-7562 reputation system. +- **D:** Signed payload omits `chainId`. Valid signature replayable on forks/other EVM chains. Or `chainId` hardcoded at deployment instead of `block.chainid`. +- **FP:** EIP-712 domain separator includes dynamic `chainId: block.chainid` and `verifyingContract`. -**101. Deployer Privilege Retention Post-Deployment** +**124. Non-Standard ERC20 Return Values (USDT-style)** -- **D:** Deployer EOA retains owner/admin/minter/pauser/upgrader after deployment script completes. Pattern: `Ownable` sets `owner = msg.sender` with no `transferOwnership()`. `AccessControl` grants `DEFAULT_ADMIN_ROLE` to deployer with no `renounceRole()`. -- **FP:** Script includes `transferOwnership(multisig)`. Admin role granted to timelock/governance, deployer renounces. `Ownable2Step` with pending owner set to multisig. +- **D:** `require(token.transfer(to, amount))` reverts on tokens returning nothing (USDT, BNB). Or return value ignored (silent failure). +- **FP:** OZ `SafeERC20.safeTransfer()`/`safeTransferFrom()` used throughout. -**102. Non-Atomic Multi-Contract Deployment (Partial System Bootstrap)** +**125. Front-Running Zero Balance Check with Dust Transfer** -- **D:** Deployment script deploys interdependent contracts across separate transactions. Midway failure leaves half-deployed state with missing references or unwired contracts. Pattern: multiple `vm.broadcast()` blocks or sequential `await deploy()` with no idempotency checks. -- **FP:** Single `vm.startBroadcast()`/`vm.stopBroadcast()` block. Factory deploys+wires all in one tx. Script is idempotent. Hardhat-deploy with resumable migrations. +- **D:** `require(token.balanceOf(address(this)) == 0)` gates a state transition. Dust transfer makes balance non-zero, DoS-ing the function at negligible cost. +- **FP:** Threshold check (`<= DUST_THRESHOLD`) instead of `== 0`. Access-controlled function. Internal accounting ignores direct transfers. -**103. CREATE / CREATE2 Deployment Failure Silently Returns Zero** +**126. Diamond Proxy Facet Selector Collision** -- **D:** Assembly `create(v, offset, size)` or `create2(v, offset, size, salt)` returns `address(0)` on failure (insufficient balance, collision, init code revert) but the code does not check for zero. The zero address is stored or used, and subsequent calls to `address(0)` silently succeed as no-ops (no code) or interact with precompiles. -- **FP:** Immediate check: `if iszero(addr) { revert(0, 0) }` after create/create2. Address validated downstream before any state-dependent operation. +- **D:** EIP-2535 Diamond where two facets register same 4-byte selector. Malicious facet via `diamondCut` hijacks calls to critical functions. Pattern: `diamondCut` adds facet with overlapping selectors, no on-chain collision check. +- **FP:** `diamondCut` validates no selector collisions. `DiamondLoupeFacet` enumerates/verifies selectors post-cut. Multisig + timelock on `diamondCut`. -**104. ERC721 / ERC1155 Type Confusion in Dual-Standard Marketplace** +**127. Flash Loan Governance Attack** -- **D:** Shared `buy`/`fill` function uses type flag for ERC721/ERC1155. `quantity` accepted for ERC721 without requiring == 1. `price * quantity` with `quantity = 0` yields zero payment. Ref: TreasureDAO (2022). -- **FP:** ERC721 branch `require(quantity == 1)`. Separate code paths for ERC721/ERC1155. +- **D:** Voting uses `token.balanceOf(msg.sender)` or `getPastVotes(account, block.number)` (current block). Borrow tokens, vote, repay in one tx. +- **FP:** `getPastVotes(account, block.number - 1)` used. Timelock between snapshot and vote. ---- +**128. Hardcoded Network-Specific Addresses** ---- +- **D:** Literal `address(0x...)` constants for external dependencies (oracles, routers, tokens) in deployment scripts/constructors. Wrong contracts on different chains. +- **FP:** Per-chain config file keyed by chain ID. Script asserts `block.chainid`. Addresses passed as constructor args from environment. Deterministic cross-chain addresses (e.g., Permit2). -**105. Cross-Contract Reentrancy** +**129. ERC4626 Round-Trip Profit Extraction** -- **D:** Two contracts share logical state (balances in A, collateral check in B). A makes external call before syncing state B reads. A's `ReentrancyGuard` doesn't protect B. -- **FP:** State B reads is synchronized before A's external call. No re-entry path from A's callee into B. +- **D:** `redeem(deposit(a)) > a` or inverse — rounding errors in both `_convertToShares` and `_convertToAssets` favor the user, yielding net profit per round-trip. +- **FP:** Rounding per EIP-4626: deposit/mint round down (vault-favorable), withdraw/redeem round up (vault-favorable). OZ ERC4626 with `_decimalsOffset()`. -**106. Non-Atomic Proxy Initialization (Front-Running `initialize()`)** +**130. ERC1155 ID-Based Role Access Control With Publicly Mintable Role Tokens** -- **D:** Proxy deployed in one tx, `initialize()` called in separate tx. Uninitialized proxy front-runnable. Pattern: `new TransparentUpgradeableProxy(impl, admin, "")` with empty data, separate `initialize()`. Ref: Wormhole (2022). -- **FP:** Proxy constructor receives init calldata atomically: `new TransparentUpgradeableProxy(impl, admin, abi.encodeCall(...))`. OZ `deployProxy()` used. +- **D:** Access control via `require(balanceOf(msg.sender, ROLE_ID) > 0)` where `mint` for those IDs is not separately gated. Role tokens transferable by default. +- **FP:** Minting role-token IDs gated behind separate access control. Role tokens non-transferable (`_beforeTokenTransfer` reverts for non-mint/burn). Dedicated non-token ACL used. -**107. EIP-2981 Royalty Signaled But Never Enforced** +**131. Off-By-One in Bounds or Range Checks** -- **D:** `royaltyInfo()` implemented and `supportsInterface(0x2a55205a)` returns true, but transfer/settlement logic never calls `royaltyInfo()` or routes payment. EIP-2981 is advisory only. -- **FP:** Settlement contract reads `royaltyInfo()` and transfers royalty on-chain. Royalties intentionally zero and documented. +- **D:** (1) `i <= arr.length` in loop (accesses OOB index). (2) `arr[arr.length - 1]` in `unchecked` without length > 0 check. (3) `>=` vs `>` confusion in financial logic (early unlock, boundary-exceeding deposit). (4) Integer division rounding accumulation across N recipients. +- **FP:** Loop uses `<` with fixed-length array. Last-element access preceded by length check. Financial boundaries demonstrably correct for the invariant. -**108. Paymaster Gas Penalty Undercalculation** +**132. ERC4626 Caller-Dependent Conversion Functions** -- **D:** Paymaster prefund formula omits 10% EntryPoint penalty on unused execution gas (`postOpUnusedGasPenalty`). Large `executionGasLimit` with low usage drains paymaster deposit. -- **FP:** Prefund explicitly adds unused-gas penalty. Conservative overestimation covers worst case. +- **D:** `convertToShares()`/`convertToAssets()` branches on `msg.sender`-specific state (per-user fees, whitelist, balances). EIP-4626 requires caller-independence. Downstream aggregators get wrong sizing. +- **FP:** Implementation reads only global vault state (`totalSupply`, `totalAssets`, protocol-wide fee constants). -**109. ERC721 Approval Not Cleared in Custom Transfer Override** +**133. Multi-Block TWAP Oracle Manipulation** -- **D:** Custom `transferFrom` override skips `super._transfer()` or `super.transferFrom()`, missing the `delete _tokenApprovals[tokenId]` step. Previous approval persists under new owner. -- **FP:** Override calls `super.transferFrom` or `super._transfer` internally. Or explicitly deletes approval / calls `_approve(address(0), tokenId, owner)`. +- **D:** TWAP observation window < 30 minutes. Post-Merge validators controlling consecutive blocks can hold manipulated AMM state across blocks, shifting TWAP cheaply. +- **FP:** TWAP window >= 30 min. Chainlink/Pyth as price source. Max-deviation circuit breaker against secondary source. -**110. DoS via Push Payment to Rejecting Contract** +**134. Merkle Proof Reuse — Leaf Not Bound to Caller** -- **D:** ETH distribution in a single loop via `recipient.call{value:}("")`. Any reverting recipient blocks entire loop. -- **FP:** Pull-over-push pattern. Loop uses `try/catch` and continues on failure. +- **D:** Merkle leaf doesn't include `msg.sender`: `MerkleProof.verify(proof, root, keccak256(abi.encodePacked(amount)))`. Proof can be front-run from different address. +- **FP:** Leaf encodes `msg.sender`: `keccak256(abi.encodePacked(msg.sender, amount))`. Proof recorded as consumed after first use. -**111. Weak On-Chain Randomness** +**135. Uninitialized Implementation Takeover** -- **D:** Randomness from `block.prevrandao`, `blockhash(block.number - 1)`, `block.timestamp`, `block.coinbase`, or combinations. Validator-influenceable or visible before inclusion. -- **FP:** Chainlink VRF v2+. Commit-reveal with future-block reveal and slashing for non-reveal. +- **D:** Implementation behind proxy has `initialize()` but constructor lacks `_disableInitializers()`. Attacker calls `initialize()` on implementation directly, becomes owner, can upgrade to malicious contract. Ref: Wormhole (2022), Parity (2017). +- **FP:** Constructor contains `_disableInitializers()`. OZ `Initializable` correctly gates the function. Not behind a proxy (standalone). -**112. Delegatecall to Untrusted Callee** +**136. Missing chainId / Message Uniqueness in Bridge** -- **D:** `address(target).delegatecall(data)` where `target` is user-provided or unconstrained. -- **FP:** `target` is hardcoded immutable verified library address. +- **D:** Bridge processes messages without `processedMessages[hash]` replay check, `destinationChainId == block.chainid` validation, or source chain ID in hash. Message replayable cross-chain or on same chain. +- **FP:** Unique nonce per sender. Hash of `(sourceChain, destChain, nonce, payload)` stored and checked. Contract address in hash. -**113. UUPS `_authorizeUpgrade` Missing Access Control** +**137. Missing Oracle Price Bounds (Flash Crash / Extreme Value)** -- **D:** `function _authorizeUpgrade(address) internal override {}` with empty body and no access modifier. Anyone can call `upgradeTo()`. Ref: CVE-2021-41264. -- **FP:** `_authorizeUpgrade()` has `onlyOwner` or equivalent. Multisig/governance controls owner role. +- **D:** Oracle returns a technically valid but extreme price (e.g., ETH at $0.01 during a flash crash). No min/max sanity bound or deviation check against historical/secondary price. Protocol executes liquidations or swaps at wildly incorrect prices. +- **FP:** Circuit breaker: `require(price >= MIN_PRICE && price <= MAX_PRICE)`. Deviation check against secondary oracle source. Heartbeat + price-change-rate limiting. -**114. Insufficient Block Confirmations / Reorg Double-Spend** +**138. DVN Collusion or Insufficient DVN Diversity** -- **D:** DVN relays cross-chain message before source chain reaches finality. Attacker deposits on source chain, gets minted on destination, then causes a reorg on source chain (or the chain reorgs naturally) to reverse the deposit while keeping minted tokens. Pattern: confirmation count set below chain's known reorg depth (e.g., < 32 blocks on Polygon). -- **FP:** Confirmation count matches or exceeds chain-specific finality guarantees. Chain has fast finality (e.g., Ethereum post-merge ~12 min). DVN waits for finalized blocks. +- **D:** OApp configured with a single DVN (`1/1/1` security stack) or multiple DVNs controlled by the same entity / using the same underlying verification method. Compromising one entity approves fraudulent cross-chain messages. Pattern: `setConfig` with `requiredDVNCount = 1` and no optional DVNs. +- **FP:** Diverse DVN set (e.g., Google Cloud DVN + Axelar Adapter + protocol's own DVN) with `2/3+` threshold. DVNs use independent verification methods (light client, oracle, ZKP). Protocol runs its own required DVN. ---- +**139. Missing Cross-Chain Rate Limits / Circuit Breakers** ---- +- **D:** Bridge or OFT contract has no per-transaction or time-window transfer caps. A single exploit transaction can drain the entire locked asset pool before detection. No pause mechanism to halt operations during an active exploit. +- **FP:** Per-tx and per-window rate limits configured (e.g., Chainlink CCIP per-lane limits). `whenNotPaused` modifier on send/receive. Anomaly detection with automated pause. Guardian/emergency multisig can freeze operations. Ref: Ronin hack went 6 days undetected — rate limits would have capped losses. -**115. Nested Mapping Inside Struct Not Cleared on `delete`** +**140. Staking Reward Front-Run by New Depositor** -- **D:** `delete myMapping[key]` on struct containing `mapping` or dynamic array. `delete` zeroes primitives but not nested mappings. Reused key exposes stale values. -- **FP:** Nested mapping manually cleared before `delete`. Key never reused after deletion. +- **D:** Reward checkpoint (`rewardPerTokenStored`) updated AFTER new stake recorded: `_balances[user] += amount` before `updateReward()`. New staker earns rewards for unstaked period. +- **FP:** `updateReward(account)` executes before any balance update. `rewardPerTokenPaid[user]` tracks per-user checkpoint. -**116. ERC721A Lazy Ownership — ownerOf Uninitialized in Batch Range** +**141. L2 Sequencer Uptime Not Checked** -- **D:** ERC721A/`ERC721Consecutive` batch mint: only first token has ownership written. `ownerOf(id)` for mid-batch IDs may return `address(0)` before any transfer. Access control checking `ownerOf == msg.sender` fails on freshly minted tokens. -- **FP:** Explicit transfer initializes packed slot before ownership check. Standard OZ `ERC721` writes `_owners[tokenId]` per mint. +- **D:** Contract on L2 (Arbitrum/Optimism/Base) uses Chainlink feeds without querying L2 Sequencer Uptime Feed. Stale data during downtime triggers wrong liquidations. +- **FP:** Sequencer uptime feed queried (`answer == 0` = up), with grace period after restart. -**117. Cross-Chain Message Spoofing (Missing Endpoint/Peer Validation)** +**142. Insufficient Gas Forwarding / 63/64 Rule** -- **D:** Receiver contract accepts cross-chain messages without verifying `msg.sender == endpoint` and `_origin.sender == registeredPeer[srcChainId]`. Attacker calls the receive function directly with fabricated message data, triggering unauthorized mints/unlocks. -- **FP:** `onlyPeer` modifier or equivalent checks both `msg.sender` (endpoint) and `_origin.sender` (peer). Standard `OAppReceiver._acceptNonce` validates origin. Ref: CrossCurve bridge exploit (Jan 2026) — $3M stolen via spoofed `expressExecute`. +- **D:** External call without minimum gas budget: `target.call(data)` with no gas check. 63/64 rule leaves subcall with insufficient gas. In relayer patterns, subcall silently fails but outer tx marks request as "processed." +- **FP:** `require(gasleft() >= minGas)` before subcall. Return value + returndata both checked. EIP-2771 with verified gas parameter. -**118. Proxy Admin Key Compromise** +**143. Accrued Interest Omitted from Health Factor or LTV Calculation** -- **D:** `ProxyAdmin.owner()` returns EOA, not multisig/governance; no timelock on `upgradeTo`. Ref: PAID Network (2021), Ankr (2022). -- **FP:** Multisig (threshold >= 2) + timelock (24-72h). Admin role separate from operational roles. +- **D:** Health factor or LTV computed from principal debt without adding accrued interest: `health = collateral / principal` instead of `collateral / (principal + accruedInterest)`. Understates actual debt, delays necessary liquidations. +- **FP:** `getDebt()` includes accrued interest. Interest accrual function called before health check. Interest compounded on every state-changing interaction. -**119. Unauthorized Peer Initialization (Fake Peer Attack)** +**144. ERC1155 setApprovalForAll Grants All-Token-All-ID Access** -- **D:** `setPeer()` / `setTrustedRemote()` sets the remote peer address that a cross-chain contract trusts. If the owner is compromised or `setPeer` lacks proper access control, an attacker registers a fraudulent peer contract on the source chain that can mint/unlock tokens on the destination without legitimate deposits. Pattern: `setPeer` callable by non-owner, or owner key compromised. -- **FP:** `setPeer` protected by multisig + timelock. Peer addresses verified against known deployment registry. `allowInitializePath()` properly implemented to reject unknown peers. Ref: GAIN token exploit (Sep 2025) — fake peer minted 5B tokens, $3M stolen. +- **D:** Protocol requires `setApprovalForAll(protocol, true)` for deposits/staking. No per-ID or per-amount granularity -- operator can transfer any ID at full balance. +- **FP:** Protocol uses direct `safeTransferFrom` with user as `msg.sender`. Operator is immutable contract with escrow-only transfer logic. -**120. Rounding in Favor of the User** +**145. Storage Layout Collision Between Proxy and Implementation** -- **D:** `shares = assets / pricePerShare` rounds down for deposit but up for redeem. Division without explicit rounding direction. First-depositor donation amplifies the error. -- **FP:** `Math.mulDiv` with explicit `Rounding.Up` where vault-favorable. OZ ERC4626 `_decimalsOffset()`. Dead shares at init. +- **D:** Proxy declares state variables at sequential slots (not EIP-1967). Implementation also starts at slot 0. Proxy's admin overlaps implementation's `initialized` flag. Ref: Audius (2022). +- **FP:** EIP-1967 slots. OZ Transparent/UUPS pattern. No state variables in proxy contract. -**121. Arbitrary External Call with User-Supplied Target and Calldata** +**146. validateUserOp Missing EntryPoint Caller Restriction** -- **D:** `target.call{value: v}(data)` where `target` or `data` (or both) are caller-supplied parameters. Attacker crafts calldata to invoke unintended functions on the target (e.g., `transferFrom` on an approved ERC20, or `safeTransferFrom` on held NFTs), stealing assets the contract holds or has approvals for. -- **FP:** Target restricted to hardcoded/whitelisted address. Calldata function selector restricted to known-safe set. No token approvals or asset holdings on the calling contract. `delegatecall` vector covered separately (V58/V105); this covers `call`. +- **D:** `validateUserOp` is `public`/`external` without `require(msg.sender == entryPoint)`. Also check `execute`/`executeBatch` for same restriction. +- **FP:** `require(msg.sender == address(_entryPoint))` or `onlyEntryPoint` modifier present. Internal visibility used. -**122. Paymaster ERC-20 Payment Deferred to postOp Without Pre-Validation** +**147. Diamond Shared-Storage Cross-Facet Corruption** -- **D:** `validatePaymasterUserOp` doesn't transfer/lock tokens — payment deferred to `postOp` via `safeTransferFrom`. User can revoke allowance between validation and execution; paymaster loses deposit. -- **FP:** Tokens transferred/locked during `validatePaymasterUserOp`. `postOp` only refunds excess. +- **D:** EIP-2535 Diamond facets declare top-level state variables (plain `uint256 foo`) instead of namespaced storage structs. Multiple facets independently start at slot 0, corrupting each other. +- **FP:** All facets use single `DiamondStorage` struct at namespaced position (EIP-7201). No top-level state variables. OZ `@custom:storage-location` pattern. -**123. Minimal Proxy (EIP-1167) Implementation Destruction** +**148. Stale Cached ERC20 Balance from Direct Token Transfers** -- **D:** EIP-1167 clones `delegatecall` a fixed implementation. If implementation is destroyed, all clones become no-ops with locked funds. Pattern: `Clones.clone(impl)` where impl has no `selfdestruct` protection or is uninitialized. -- **FP:** No `selfdestruct` in implementation. `_disableInitializers()` in constructor. Post-Dencun: code not destroyed. Beacon proxies used for upgradeability. +- **D:** Contract tracks holdings in state variable (`totalDeposited`, `_reserves`) updated only through protocol functions. Direct `token.transfer(contract)` inflates real balance beyond cached value. Attacker exploits gap for share pricing/collateral manipulation. +- **FP:** Accounting reads `balanceOf(this)` live. Cached value reconciled at start of every state-changing function. Direct transfers treated as revenue. ---- +**149. Cross-Function Reentrancy** -**124. Spot Price Oracle from AMM** +- **D:** Two functions share state variable. Function A makes external call before updating shared state; Function B reads that state. `nonReentrant` on A but not B. +- **FP:** Both functions share same contract-level mutex. Shared state updated before any external call. -- **D:** Price from AMM reserves: `reserve0 / reserve1`, `getAmountsOut()`, `getReserves()`. Flash-loan exploitable atomically. -- **FP:** TWAP >= 30 min window. Chainlink/Pyth as primary source. +**150. Slippage Enforced at Intermediate Step, Not Final Output** ---- +- **D:** Multi-hop swap checks `minAmountOut` on the first hop or an intermediate step, but the amount actually received by the user at the end of the pipeline has no independent slippage bound. Second/third hops can be sandwiched freely. +- **FP:** `minAmountOut` validated against the user's final received balance (delta check). Single-hop swap. User specifies per-hop minimums. -**125. Missing Slippage Protection (Sandwich Attack)** +**151. Non-Atomic Proxy Deployment Enabling CPIMP Takeover** -- **D:** Swap/deposit/withdrawal with `minAmountOut = 0`, or `minAmountOut` computed on-chain from current pool state. -- **FP:** `minAmountOut` set off-chain by user and validated on-chain. +- **D:** Proxy deployed in one tx, `initialize()` in a separate tx. Attacker front-runs init and inserts a malicious middleman implementation (CPIMP) that persists across upgrades by restoring itself in ERC-1967 slot. +- **FP:** Atomic init calldata in constructor. `_disableInitializers()` in implementation constructor. -**126. NFT Staking Records msg.sender Instead of ownerOf** +**152. Cross-Chain Reentrancy via Safe Transfer Callbacks** -- **D:** `depositor[tokenId] = msg.sender` without checking `nft.ownerOf(tokenId)`. Approved operator (not owner) calls stake — transfer succeeds via approval, operator credited as depositor. -- **FP:** Reads `nft.ownerOf(tokenId)` before transfer and records actual owner. Or `require(nft.ownerOf(tokenId) == msg.sender)`. +- **D:** Cross-chain receive function (`lzReceive`, `_credit`) calls `_safeMint` or `_safeTransfer` before updating supply/ownership counters. The `onERC721Received` / `onERC1155Received` callback re-enters to initiate another cross-chain send before state is finalized, creating duplicate tokens or double-spending. +- **FP:** State updates (counters, balances) committed before any safe transfer. `nonReentrant` on receive path. `_mint` used instead of `_safeMint` (no callback). Ref: Ackee Blockchain cross-chain reentrancy PoC. -**211. Interest Accrual Rounds to Zero but Timestamp Advances** +**153. abi.encodePacked Hash Collision with Dynamic Types** -- **D:** `interest = rate * timeDelta / SECONDS_PER_YEAR` rounds to zero when `timeDelta` is small (e.g., <207s at 15% APR). But `lastAccrualTime = block.timestamp` is still updated — the fractional interest is permanently lost, not deferred to the next accrual. -- **FP:** Accumulator uses sufficient precision (e.g., RAY = 1e27) to avoid zero rounding at per-block intervals. `lastAccrualTime` only advances when computed interest > 0. +- **D:** `keccak256(abi.encodePacked(a, b))` where two+ args are dynamic types (`string`, `bytes`, dynamic arrays). No length prefix means different inputs produce identical hashes. Affects permits, access control keys, nullifiers. +- **FP:** `abi.encode()` used instead. Only one dynamic type arg. All args fixed-size. -**212. Permissionless accrueInterest Griefing** +**154. Signed Integer Mishandling (signextend / sar / slt Confusion)** -- **D:** `accrueInterest()` is permissionless and updates `lastAccrualTime` on every call. Attacker calls it at very short intervals — each call computes zero interest (rounding) but advances the timestamp, systematically suppressing interest accumulation to near-zero. -- **FP:** Minimum accrual interval enforced: `require(block.timestamp - lastAccrualTime >= MIN_INTERVAL)`. Precision high enough that per-block interest > 0. Access-restricted accrual. +- **D:** Assembly performs arithmetic on signed integers but uses unsigned opcodes. `shr` instead of `sar` (arithmetic shift right) loses the sign bit. `lt`/`gt` instead of `slt`/`sgt` treats negative numbers as huge positives. Missing `signextend` when loading a signed value smaller than 256 bits from calldata or storage. +- **FP:** Code consistently uses `sar`/`slt`/`sgt` for signed operations and `shr`/`lt`/`gt` for unsigned. `signextend(byteWidth - 1, val)` applied after loading sub-256-bit signed values. Code only operates on unsigned integers. -**213. notifyRewardAmount Overwrites Active Reward Period** +**155. Missing `_debit` / `_debitFrom` Authorization in OFT** -- **D:** Calling `notifyRewardAmount(newAmount)` replaces the current reward period. Remaining undistributed rewards from the old period are silently lost — not carried forward. Admin or attacker can erase pending rewards by notifying a smaller amount. -- **FP:** New notification adds to remaining: `rewardRate = (newAmount + remaining) / duration`. Only callable by designated distributor with timelock. Remaining rewards refunded before reset. +- **D:** Custom OFT override of `_debit` or `_debitFrom` omits `require(msg.sender == _from || allowance[_from][msg.sender] >= amount)`. Anyone can bridge tokens from any holder's balance. Pattern: `_debit` that calls `_burn(_from, amount)` without verifying caller is authorized. +- **FP:** Standard LayerZero OFT implementation used without override. Custom `_debit` includes proper authorization. `_debit` only callable via `send()` which uses `msg.sender` as `_from`. -**214. Governance Proposal Executable Before Voting Period Ends** +**156. Default Message Library Hijack (Configuration Drag-Along)** -- **D:** `execute()` checks quorum and majority but not `block.timestamp >= proposal.endTime`. Once quorum is met, the proposal can be executed immediately — cutting the voting window short, preventing opposing votes from being cast. -- **FP:** `require(block.timestamp >= proposal.endTime)` in execute. OZ Governor enforces `ProposalState.Succeeded` which requires voting period to have ended. +- **D:** OApp does not explicitly pin its send/receive library version via `setSendVersion()` / `setReceiveVersion()` (or V2 `setSendLibrary()` / `setReceiveLibrary()`). It relies on the endpoint's mutable default. A malicious or compromised default library update silently applies to all unpinned OApps, bypassing DVN/Oracle validation. +- **FP:** OApp explicitly sets library versions in constructor or initialization. Configuration is immutable or governance-controlled with timelock. LayerZero V2 EndpointV2 is non-upgradeable (but library defaults are still mutable). -**215. Partial Liquidation Leaves Position in Worse State** +**157. ERC-1271 isValidSignature Delegated to Untrusted Module** -- **D:** Partial liquidation seizes some collateral but does not enforce a minimum post-liquidation health factor. Liquidator cherry-picks the most valuable collateral, leaving the position with worse health than before — approaching full insolvency without triggering full liquidation. -- **FP:** Post-liquidation health factor check: `require(healthFactor(position) >= MIN_HF)`. Full liquidation triggered below a floor threshold. Liquidator must bring position to target health factor. +- **D:** `isValidSignature(hash, sig)` delegated to externally-supplied contract without whitelist check. Malicious module always returns `0x1626ba7e`, passing all signature checks. +- **FP:** Delegation only to owner-controlled whitelist. Module registry has timelock/guardian approval. -**216. Delegation to address(0) Blocks Token Transfers** +**158. Proxy Storage Slot Collision** -- **D:** Delegating votes to `address(0)` causes `_beforeTokenTransfer` or `_update` hooks to revert when attempting to modify the zero-address delegate's checkpoint. All subsequent transfers and burns for that token holder permanently revert. -- **FP:** Delegation to `address(0)` treated as undelegation (no-op or clears delegation). Hook skips checkpoint update when delegate is `address(0)`. OZ Votes implementation handles this case. +- **D:** Proxy stores `implementation`/`admin` at sequential slots (0, 1); implementation also declares variables from slot 0. Implementation writes overwrite proxy pointers. +- **FP:** EIP-1967 randomized slots used. OZ Transparent/UUPS pattern. -**217. ERC4626 maxDeposit Returns Non-Zero When Paused** +**159. Counterfactual Wallet Initialization Parameters Not Bound to Deployed Address** -- **D:** `maxDeposit()` returns `type(uint256).max` even when the vault is paused. Integrating protocols (aggregators, routers) read this as "deposits accepted," attempt a deposit, and revert. Per ERC4626, `maxDeposit` must return 0 when deposits would revert. -- **FP:** `maxDeposit()` returns 0 when paused. OZ ERC4626 with pausing override. Integrators use `try deposit()` with fallback. +- **D:** Factory `createAccount` uses CREATE2 but salt doesn't incorporate all init params (especially owner). Attacker calls `createAccount` with different owner, deploying wallet they control to same counterfactual address. +- **FP:** Salt derived from all init params: `salt = keccak256(abi.encodePacked(owner, ...))`. Factory reverts if account exists. Initializer called atomically with deployment. -**219. Deprecated Gauge Blocks Claiming Accrued Rewards** +**160. Oracle Price Update Front-Running** -- **D:** Killing or deprecating a gauge clears future distributions but also blocks the `claimReward()` path for already-accrued, unclaimed rewards. Users who earned rewards before deprecation cannot retrieve them. -- **FP:** Kill only stops future accrual — claim function remains active for pre-kill balances. Rewards swept to fallback address on deprecation. Emergency claim path bypasses active-gauge check. +- **D:** On-chain oracle update tx visible in mempool. Attacker front-runs a favorable price update by opening a position at the stale price, then profits when the update lands. Pattern: Pyth/Chainlink push-model where update tx is submitted to public mempool. +- **FP:** Protocol uses pull-based oracle (user submits price update atomically with their action). Private mempool (Flashbots Protect) for oracle updates. Price-update-and-action in single tx. -**221. Liquidation Blocked by External Pool Illiquidity** +**161. Metamorphic Contract via CREATE2 + SELFDESTRUCT** -- **D:** Liquidation function swaps collateral for debt token via an external DEX. If the DEX pool is drained or lacks liquidity, the swap reverts, making liquidation impossible. Bad debt accumulates while the pool remains illiquid. -- **FP:** Liquidation accepts collateral directly without swap. Fallback liquidation path uses a different DEX or oracle price. Liquidator provides debt token and receives collateral. +- **D:** `CREATE2` deployment where deployer can `selfdestruct` and redeploy different bytecode at same address. Governance-approved code swapped before execution. Ref: Tornado Cash Governance (2023). Post-Dencun (EIP-6780): largely mitigated except same-tx create-destroy-recreate. +- **FP:** Post-Dencun: `selfdestruct` no longer destroys code unless same tx as creation. `EXTCODEHASH` verified at execution time. Not deployed via `CREATE2` from mutable deployer. -**222. No-Bid Auction Fails to Clear State** +**162. Free Memory Pointer Corruption** -- **D:** Auction expires without any bids. The finalization function does not clear lien, auction, or escrow data — collateral remains locked in the auction contract with no path to return it to the owner or trigger a new auction. -- **FP:** No-bid finalization returns collateral to owner and clears all associated state. Re-auction mechanism triggered automatically. Timeout-based collateral release. +- **D:** Assembly block writes to memory at fixed offsets (e.g., `mstore(0x80, val)`) without reading or updating the free memory pointer at `0x40`. Subsequent Solidity code allocates memory from stale pointer, overwriting the assembly-written data — or vice versa. Second pattern: assembly sets `mstore(0x40, newPtr)` to an incorrect value, causing later Solidity allocations to overlap prior data. +- **FP:** Assembly block reads `mload(0x40)`, writes only above that offset, then updates `mstore(0x40, newFreePtr)`. Or block only uses scratch space (`0x00`–`0x3f`). Block annotated `memory-safe` and verified to comply. Ref: Solidity optimizer bug in 0.8.13–0.8.14 mishandled cross-block memory writes. -**223. Position Reduction Triggers Liquidation** +**163. ERC4626 Inflation Attack (First Depositor)** -- **D:** User attempts to improve health by partially repaying debt or withdrawing a small amount of excess collateral. The intermediate state (after collateral removal, before debt reduction) crosses the liquidation threshold — a bot liquidates the position mid-transaction or in the same block. -- **FP:** Repay and collateral changes are atomic (single function). Health check applied only to final state, not intermediate. Liquidation grace period after position modification. +- **D:** `shares = assets * totalSupply / totalAssets`. When `totalSupply == 0`, deposit 1 wei + donate inflates share price, victim's deposit rounds to 0 shares. No virtual offset. +- **FP:** OZ ERC4626 with `_decimalsOffset()`. Dead shares minted to `address(0)` at init. -**224. Repeated Liquidation of Same Position** +**164. Storage Layout Shift on Upgrade** -- **D:** Liquidation function does not flag the position as liquidated. After partial liquidation, the position still appears undercollateralized — a second liquidator (or the same one) liquidates again, seizing collateral beyond what was intended. -- **FP:** Position marked as `liquidated` or deleted after processing. Liquidation requires `status != Liquidated`. Post-liquidation health check prevents re-triggering. +- **D:** V2 inserts new state variable in middle of contract instead of appending. Subsequent variables shift slots, corrupting state. Also: changing variable type between versions shifts slot boundaries. +- **FP:** New variables only appended. OZ storage layout validation in CI. Variable types unchanged between versions. -**225. Loan State Transition Before Interest Settlement** +**165. Hardcoded Calldataload Offset Bypass via Non-Canonical ABI Encoding** -- **D:** Repaying principal sets the loan state to `Repaid` before accrued interest is settled. Once in `Repaid` state, the interest accrual function skips the loan — all accumulated interest becomes permanently uncollectable. -- **FP:** `settleInterest()` called before state transition. Interest added to repayment amount: `require(msg.value >= principal + accruedInterest)`. State transition only after full settlement. +- **D:** Assembly reads a field at hardcoded calldata offset (`calldataload(164)`) assuming standard ABI layout. Attacker crafts non-canonical encoding — manipulated dynamic-type offset pointers or padding — so a different value sits at the expected position. +- **FP:** Field decoded via `abi.decode()` (compiler bounds-checked). No hardcoded `calldataload` offsets — parameters extracted through Solidity's typed calldata accessors. `calldatasize() >= expected` validated before reading. diff --git a/solidity-auditor/references/attack-vectors/attack-vectors-4.md b/solidity-auditor/references/attack-vectors/attack-vectors-4.md index 124d373..e823645 100644 --- a/solidity-auditor/references/attack-vectors/attack-vectors-4.md +++ b/solidity-auditor/references/attack-vectors/attack-vectors-4.md @@ -1,292 +1,279 @@ # Attack Vectors Reference (4/4) -219 total attack vectors +220 total attack vectors --- -**127. Missing chainId (Cross-Chain Replay)** +**166. Calldata Input Malleability** -- **D:** Signed payload omits `chainId`. Valid signature replayable on forks/other EVM chains. Or `chainId` hardcoded at deployment instead of `block.chainid`. -- **FP:** EIP-712 domain separator includes dynamic `chainId: block.chainid` and `verifyingContract`. - -**128. Non-Standard ERC20 Return Values (USDT-style)** - -- **D:** `require(token.transfer(to, amount))` reverts on tokens returning nothing (USDT, BNB). Or return value ignored (silent failure). -- **FP:** OZ `SafeERC20.safeTransfer()`/`safeTransferFrom()` used throughout. - -**129. Front-Running Zero Balance Check with Dust Transfer** - -- **D:** `require(token.balanceOf(address(this)) == 0)` gates a state transition. Dust transfer makes balance non-zero, DoS-ing the function at negligible cost. -- **FP:** Threshold check (`<= DUST_THRESHOLD`) instead of `== 0`. Access-controlled function. Internal accounting ignores direct transfers. - -**130. Diamond Proxy Facet Selector Collision** - -- **D:** EIP-2535 Diamond where two facets register same 4-byte selector. Malicious facet via `diamondCut` hijacks calls to critical functions. Pattern: `diamondCut` adds facet with overlapping selectors, no on-chain collision check. -- **FP:** `diamondCut` validates no selector collisions. `DiamondLoupeFacet` enumerates/verifies selectors post-cut. Multisig + timelock on `diamondCut`. - -**131. Flash Loan Governance Attack** - -- **D:** Voting uses `token.balanceOf(msg.sender)` or `getPastVotes(account, block.number)` (current block). Borrow tokens, vote, repay in one tx. -- **FP:** `getPastVotes(account, block.number - 1)` used. Timelock between snapshot and vote. +- **D:** Contract hashes raw calldata for uniqueness (`processedHashes[keccak256(msg.data)]`). Dynamic-type ABI encoding uses offset pointers — multiple distinct calldata layouts decode to identical values. Attacker bypasses dedup with semantically equivalent but bytewise-different calldata. +- **FP:** Uniqueness check hashes decoded parameters: `keccak256(abi.encode(decodedParams))`. Nonce-based replay protection. Only fixed-size types in signature (no encoding ambiguity). -**132. Hardcoded Network-Specific Addresses** +**167. Reward Accrual During Zero-Depositor Period** -- **D:** Literal `address(0x...)` constants for external dependencies (oracles, routers, tokens) in deployment scripts/constructors. Wrong contracts on different chains. -- **FP:** Per-chain config file keyed by chain ID. Script asserts `block.chainid`. Addresses passed as constructor args from environment. Deterministic cross-chain addresses (e.g., Permit2). +- **D:** Time-based reward distribution starts at vault deployment but no depositors exist yet. First depositor claims all rewards accumulated during the empty period regardless of deposit size or timing. +- **FP:** Rewards only accrue when `totalSupply > 0`. Reward start time set on first deposit. Unclaimed pre-deposit rewards sent to treasury or burned. -**133. ERC4626 Round-Trip Profit Extraction** +**168. MEV Withdrawal Before Bad Debt Socialization** -- **D:** `redeem(deposit(a)) > a` or inverse — rounding errors in both `_convertToShares` and `_convertToAssets` favor the user, yielding net profit per round-trip. -- **FP:** Rounding per EIP-4626: deposit/mint round down (vault-favorable), withdraw/redeem round up (vault-favorable). OZ ERC4626 with `_decimalsOffset()`. +- **D:** External event (liquidation, exploit, depeg) causes vault loss. MEV actor observes pending loss-causing tx in mempool and front-runs a withdrawal at pre-loss share price, leaving remaining depositors to absorb the full loss. +- **FP:** Withdrawals require time-delayed request queue (epoch-based or cooldown). Loss realization and share price update are atomic. Private mempool used for liquidation txs. -**134. ERC1155 ID-Based Role Access Control With Publicly Mintable Role Tokens** +**169. Vault Insolvency via Accumulated Rounding Dust** -- **D:** Access control via `require(balanceOf(msg.sender, ROLE_ID) > 0)` where `mint` for those IDs is not separately gated. Role tokens transferable by default. -- **FP:** Minting role-token IDs gated behind separate access control. Role tokens non-transferable (`_beforeTokenTransfer` reverts for non-mint/burn). Dedicated non-token ACL used. +- **D:** Vault tracks `totalAssets` as a storage variable separate from `token.balanceOf(vault)`. Solidity's floor rounding on each deposit/withdrawal creates tiny overages — user receives 1 wei more than burned shares represent. Over many operations `totalAssets` exceeds actual balance, causing last withdrawers to revert. +- **FP:** Rounding consistently favors the vault (round shares up on deposit, round assets down on withdrawal). OZ Math with `Rounding.Ceil`/`Rounding.Floor` applied correctly. -**135. Off-By-One in Bounds or Range Checks** +**170. FIFO Withdrawal Ordering Degrades Yield** -- **D:** (1) `i <= arr.length` in loop (accesses OOB index). (2) `arr[arr.length - 1]` in `unchecked` without length > 0 check. (3) `>=` vs `>` confusion in financial logic (early unlock, boundary-exceeding deposit). (4) Integer division rounding accumulation across N recipients. -- **FP:** Loop uses `<` with fixed-length array. Last-element access preceded by length check. Financial boundaries demonstrably correct for the invariant. +- **D:** Aggregator vault withdraws from sub-vaults in fixed FIFO order, depleting highest-APY vaults first. Remaining capital concentrates in lowest-yield positions, reducing overall returns for all depositors. +- **FP:** Withdrawal ordering sorted by APY ascending (lowest-yield first). Dynamic rebalancing after withdrawals. Single underlying vault (no ordering issue). -**136. ERC4626 Caller-Dependent Conversion Functions** +**171. ERC4626 convertToAssets Used Instead of previewWithdraw** -- **D:** `convertToShares()`/`convertToAssets()` branches on `msg.sender`-specific state (per-user fees, whitelist, balances). EIP-4626 requires caller-independence. Downstream aggregators get wrong sizing. -- **FP:** Implementation reads only global vault state (`totalSupply`, `totalAssets`, protocol-wide fee constants). +- **D:** Integration calls `convertToAssets(shares)` to estimate withdrawal proceeds. Per ERC4626 spec this excludes fees and slippage — actual redeemable amount is lower. Downstream logic (health checks, rebalancing, UI) operates on inflated values. +- **FP:** `previewWithdraw()` or `previewRedeem()` used for actual withdrawal estimates. Vault charges no withdrawal fees. Fee delta accounted separately. ---- +**172. Unclaimed Reward Tokens from Underlying Protocol** ---- +- **D:** Vault deposits into yield protocol (Morpho, Aave, Convex) that emits reward tokens. Vault never calls `claim()` or lacks logic to distribute claimed rewards to depositors. Rewards accumulate indefinitely in the vault or underlying protocol, inaccessible to shareholders. +- **FP:** Explicit `claimRewards()` function harvests and distributes. Reward tokens tracked dynamically (mapping, not hardcoded list). Vault sweeps unexpected token balances to treasury. -**137. Multi-Block TWAP Oracle Manipulation** +**173. Idle Asset Dilution from Sub-Vault Deposit Caps** -- **D:** TWAP observation window < 30 minutes. Post-Merge validators controlling consecutive blocks can hold manipulated AMM state across blocks, shifting TWAP cheaply. -- **FP:** TWAP window >= 30 min. Chainlink/Pyth as price source. Max-deviation circuit breaker against secondary source. +- **D:** Parent/aggregator vault accepts deposits without checking sub-vault capacity. When sub-vaults hit their deposit caps, excess assets sit idle in the parent — earning zero yield but diluting share price for all depositors. +- **FP:** `maxDeposit()` reflects combined sub-vault remaining capacity. Deposits revert when no productive capacity remains. Idle assets auto-routed to fallback yield source. -**138. Merkle Proof Reuse — Leaf Not Bound to Caller** +**174. Memory Struct Copy Not Written Back to Storage** -- **D:** Merkle leaf doesn't include `msg.sender`: `MerkleProof.verify(proof, root, keccak256(abi.encodePacked(amount)))`. Proof can be front-run from different address. -- **FP:** Leaf encodes `msg.sender`: `keccak256(abi.encodePacked(msg.sender, amount))`. Proof recorded as consumed after first use. +- **D:** `MyStruct memory s = myMapping[key]` creates a memory copy. Modifications to `s` do not persist — storage remains unchanged. Common in reward updates, position tracking, and config changes where the developer assumes memory aliases storage. +- **FP:** Uses `storage` keyword: `MyStruct storage s = myMapping[key]`. Explicitly writes back: `myMapping[key] = s` after modification. -**139. Uninitialized Implementation Takeover** +**175. On-Chain Slippage Computed from Manipulated Pool** -- **D:** Implementation behind proxy has `initialize()` but constructor lacks `_disableInitializers()`. Attacker calls `initialize()` on implementation directly, becomes owner, can upgrade to malicious contract. Ref: Wormhole (2022), Parity (2017). -- **FP:** Constructor contains `_disableInitializers()`. OZ `Initializable` correctly gates the function. Not behind a proxy (standalone). +- **D:** `amountOutMin` calculated on-chain by querying the same pool that will execute the swap. Attacker manipulates the pool before the tx, making both the quote and the swap reflect the manipulated state — slippage check passes despite sandwich. +- **FP:** `amountOutMin` supplied by the caller (off-chain quote). Uses a separate oracle for the floor price. TWAP-based minimum. -**140. Missing chainId / Message Uniqueness in Bridge** +**176. Withdrawal Queue Bricked by Zero-Amount Entry** -- **D:** Bridge processes messages without `processedMessages[hash]` replay check, `destinationChainId == block.chainid` validation, or source chain ID in hash. Message replayable cross-chain or on same chain. -- **FP:** Unique nonce per sender. Hash of `(sourceChain, destChain, nonce, payload)` stored and checked. Contract address in hash. +- **D:** FIFO withdrawal queue processes entries sequentially. A cancelled or zeroed-out entry causes the loop to `break` or revert on zero amount instead of skipping it, permanently blocking all subsequent withdrawals behind it. +- **FP:** Queue skips zero-amount entries. Cancellation removes the entry or marks it processed. Queue uses linked list allowing removal. -**141. Missing Oracle Price Bounds (Flash Crash / Extreme Value)** +**177. Lazy Epoch Advancement Skips Reward Periods** -- **D:** Oracle returns a technically valid but extreme price (e.g., ETH at $0.01 during a flash crash). No min/max sanity bound or deviation check against historical/secondary price. Protocol executes liquidations or swaps at wildly incorrect prices. -- **FP:** Circuit breaker: `require(price >= MIN_PRICE && price <= MAX_PRICE)`. Deviation check against secondary oracle source. Heartbeat + price-change-rate limiting. -- **Scope:** Covers missing min/max price bounds and deviation checks only. Basic staleness checks (`updatedAt`, `answeredInRound >= roundId`, `answer > 0`) are covered by V69 — do not duplicate findings for missing staleness validation under this vector. +- **D:** Epoch counter advances only on user interaction. If no one interacts during an epoch, it is never advanced — rewards for that period are miscalculated, lost, or retroactively applied to the wrong epoch when the next interaction occurs. +- **FP:** Keeper or automation advances epochs independently. Epoch catch-up loop processes all skipped epochs on next interaction. Continuous (non-epoch) reward accrual. -**142. DVN Collusion or Insufficient DVN Diversity** +**178. Reward Rate Changed Without Settling Accumulator** -- **D:** OApp configured with a single DVN (`1/1/1` security stack) or multiple DVNs controlled by the same entity / using the same underlying verification method. Compromising one entity approves fraudulent cross-chain messages. Pattern: `setConfig` with `requiredDVNCount = 1` and no optional DVNs. -- **FP:** Diverse DVN set (e.g., Google Cloud DVN + Axelar Adapter + protocol's own DVN) with `2/3+` threshold. DVNs use independent verification methods (light client, oracle, ZKP). Protocol runs its own required DVN. +- **D:** Admin updates the emission rate but `updateReward()` / `updatePool()` is not called first. The new rate is retroactively applied to the entire elapsed period since the last update, overpaying or underpaying rewards for that window. +- **FP:** Rate-change function calls `updateReward()` before applying the new rate. Modifier auto-settles on every state change. -**143. Missing Cross-Chain Rate Limits / Circuit Breakers** +**179. Liquidated Position Continues Accruing Rewards** -- **D:** Bridge or OFT contract has no per-transaction or time-window transfer caps. A single exploit transaction can drain the entire locked asset pool before detection. No pause mechanism to halt operations during an active exploit. -- **FP:** Per-tx and per-window rate limits configured (e.g., Chainlink CCIP per-lane limits). `whenNotPaused` modifier on send/receive. Anomaly detection with automated pause. Guardian/emergency multisig can freeze operations. Ref: Ronin hack went 6 days undetected — rate limits would have capped losses. +- **D:** Position is liquidated (balance zeroed, collateral seized) but is not removed from the reward distribution system. `rewardDebt` and accumulated rewards are not reset — the liquidated user earns phantom rewards, or the rewards are locked permanently. +- **FP:** Liquidation calls `_withdrawRewards()` or equivalent before zeroing the position. Reward system checks `balance > 0` before accruing. -**144. Staking Reward Front-Run by New Depositor** +**180. Cached Reward Debt Not Reset After Claim** -- **D:** Reward checkpoint (`rewardPerTokenStored`) updated AFTER new stake recorded: `_balances[user] += amount` before `updateReward()`. New staker earns rewards for unstaked period. -- **FP:** `updateReward(account)` executes before any balance update. `rewardPerTokenPaid[user]` tracks per-user checkpoint. +- **D:** After `claimRewards()`, the cached reward amount (`pendingReward` or `rewardDebt`) is not zeroed. On the next claim cycle, the full cached amount is paid again — double (or repeated) payout. +- **FP:** `pendingReward[user] = 0` after transfer. `rewardDebt` recalculated from current balance and accumulator after claim. -**145. L2 Sequencer Uptime Not Checked** +**181. Emission Distribution Before Period Update** -- **D:** Contract on L2 (Arbitrum/Optimism/Base) uses Chainlink feeds without querying L2 Sequencer Uptime Feed. Stale data during downtime triggers wrong liquidations. -- **FP:** Sequencer uptime feed queried (`answer == 0` = up), with grace period after restart. +- **D:** `distribute()` reads the contract's token balance before `updatePeriod()` mints or transfers new emissions. New rewards arrive after distribution already executed — they sit idle until the next cycle, underpaying the current period. +- **FP:** `updatePeriod()` called before `distribute()` in the same tx. Emissions pre-funded before distribution window opens. -**146. Insufficient Gas Forwarding / 63/64 Rule** +**182. Pause Modifier Blocks Liquidations** -- **D:** External call without minimum gas budget: `target.call(data)` with no gas check. 63/64 rule leaves subcall with insufficient gas. In relayer patterns, subcall silently fails but outer tx marks request as "processed." -- **FP:** `require(gasleft() >= minGas)` before subcall. Return value + returndata both checked. EIP-2771 with verified gas parameter. +- **D:** `whenNotPaused` applied broadly to all external functions including `liquidate()`. During a pause, interest accrues and prices move but positions cannot be liquidated — bad debt accumulates unchecked until unpause. +- **FP:** Liquidation functions exempt from pause. Separate `pauseLiquidations` flag with independent governance. Interest accrual also paused. ---- +**183. Liquidation Arithmetic Reverts at Extreme Price Drops** ---- +- **D:** When collateral price drops 95%+, liquidation math overflows or underflows — e.g., `collateralNeeded = debt / price` becomes enormous, exceeding available collateral. The liquidation function reverts, making the position unliquidatable and locking bad debt. +- **FP:** Liquidation caps `collateralSeized` at position's total collateral. Graceful handling of underwater positions (full seizure, remaining bad debt socialized). -**147. Accrued Interest Omitted from Health Factor or LTV Calculation** +**184. Borrower Front-Runs Liquidation** -- **D:** Health factor or LTV computed from principal debt without adding accrued interest: `health = collateral / principal` instead of `collateral / (principal + accruedInterest)`. Understates actual debt, delays necessary liquidations. -- **FP:** `getDebt()` includes accrued interest. Interest accrual function called before health check. Interest compounded on every state-changing interaction. +- **D:** Borrower observes pending `liquidate()` tx in mempool, front-runs with minimal repayment or collateral top-up to push health factor above threshold. Immediately re-borrows after liquidation tx fails. Repeated indefinitely to maintain risky position. +- **FP:** Liquidation uses flash-loan-resistant health check (same-block deposit doesn't count). Minimum repayment cooldown. Dutch auction liquidation (no fixed threshold to game). -**148. ERC1155 setApprovalForAll Grants All-Token-All-ID Access** +**185. Liquidation Discount Applied Inconsistently Across Code Paths** -- **D:** Protocol requires `setApprovalForAll(protocol, true)` for deposits/staking. No per-ID or per-amount granularity -- operator can transfer any ID at full balance. -- **FP:** Protocol uses direct `safeTransferFrom` with user as `msg.sender`. Operator is immutable contract with escrow-only transfer logic. +- **D:** One code path calculates debt at face value, another applies a liquidation discount. When the discounted amount is subtracted from the non-discounted amount, the result underflows or leaves residual bad debt unaccounted. +- **FP:** Discount applied consistently in all paths touching liquidation accounting. Single source of truth for discounted value. -**149. Storage Layout Collision Between Proxy and Implementation** +**186. No Buffer Between Max LTV and Liquidation Threshold** -- **D:** Proxy declares state variables at sequential slots (not EIP-1967). Implementation also starts at slot 0. Proxy's admin overlaps implementation's `initialized` flag. Ref: Audius (2022). -- **FP:** EIP-1967 slots. OZ Transparent/UUPS pattern. No state variables in proxy contract. +- **D:** Max borrowable LTV equals the liquidation threshold. A borrower at max LTV is immediately liquidatable on any adverse price tick. Combined with origination fees, positions can be born underwater. +- **FP:** Max LTV is meaningfully lower than liquidation threshold (e.g., 80% vs 85%). Origination fee deducted from borrowed amount, not added to debt. -**150. validateUserOp Missing EntryPoint Caller Restriction** +**187. Same-Block Vote-Transfer-Vote** -- **D:** `validateUserOp` is `public`/`external` without `require(msg.sender == entryPoint)`. Also check `execute`/`executeBatch` for same restriction. -- **FP:** `require(msg.sender == address(_entryPoint))` or `onlyEntryPoint` modifier present. Internal visibility used. +- **D:** Governance reads voting power at current block, not a past snapshot. User votes, transfers tokens to a second wallet in the same block, and votes again — doubling their effective vote weight. +- **FP:** `getPastVotes(block.number - 1)` or proposal-creation snapshot. Voting power locked on first vote until proposal closes. ERC20Votes with checkpoint-based historical balances. -**151. Diamond Shared-Storage Cross-Facet Corruption** +**188. Quorum Computed from Live Supply, Not Snapshot** -- **D:** EIP-2535 Diamond facets declare top-level state variables (plain `uint256 foo`) instead of namespaced storage structs. Multiple facets independently start at slot 0, corrupting each other. -- **FP:** All facets use single `DiamondStorage` struct at namespaced position (EIP-7201). No top-level state variables. OZ `@custom:storage-location` pattern. +- **D:** `quorum = totalSupply() * quorumBps / 10000` reads current supply. After a proposal is created, an attacker mints tokens (or deposits to inflate supply), lowering the effective quorum percentage needed to pass. +- **FP:** Quorum snapshotted at proposal creation: `totalSupply(proposalSnapshot)`. Fixed absolute quorum amount. Supply changes do not affect active proposals. -**152. Stale Cached ERC20 Balance from Direct Token Transfers** +**189. Checkpoint Overwrite on Same-Block Operations** -- **D:** Contract tracks holdings in state variable (`totalDeposited`, `_reserves`) updated only through protocol functions. Direct `token.transfer(contract)` inflates real balance beyond cached value. Attacker exploits gap for share pricing/collateral manipulation. -- **FP:** Accounting reads `balanceOf(this)` live. Cached value reconciled at start of every state-changing function. Direct transfers treated as revenue. +- **D:** Multiple delegate/transfer operations in the same block call `_writeCheckpoint()` with the same `block.number` key. Each overwrites the previous — binary search returns the first (incomplete) checkpoint, losing intermediate state. +- **FP:** Checkpoint appends only when `block.number > lastCheckpoint.blockNumber`. Same-block operations accumulate into the existing checkpoint. Off-chain indexer used instead of on-chain lookups. -**153. Cross-Function Reentrancy** +**190. Self-Delegation Doubles Voting Power** -- **D:** Two functions share state variable. Function A makes external call before updating shared state; Function B reads that state. `nonReentrant` on A but not B. -- **FP:** Both functions share same contract-level mutex. Shared state updated before any external call. +- **D:** Delegating to self adds votes to the delegate (self) but does not subtract the undelegated balance. Voting power is counted twice — once as held tokens, once as delegated votes. +- **FP:** Delegation logic subtracts from holder's direct balance when adding to delegate. Self-delegation is a no-op or explicitly handled. OZ Votes implementation used correctly. -**154. Slippage Enforced at Intermediate Step, Not Final Output** +**191. Nonce Not Incremented on Reverted Execution** -- **D:** Multi-hop swap checks `minAmountOut` on the first hop or an intermediate step, but the amount actually received by the user at the end of the pipeline has no independent slippage bound. Second/third hops can be sandwiched freely. -- **FP:** `minAmountOut` validated against the user's final received balance (delta check). Single-hop swap. User specifies per-hop minimums. +- **D:** Meta-transaction or permit nonce is checked before execution but only incremented on success. If the inner call reverts (after nonce check, before increment), the same signed message can be replayed until it eventually succeeds. +- **FP:** Nonce incremented before execution (check-effects-interaction). Nonce incremented in both success and failure paths. Deadline-based expiry on signed messages. -**155. Non-Atomic Proxy Deployment Enabling CPIMP Takeover** +**192. Bridge Global Rate Limit Griefing** -- **D:** Same non-atomic deploy+init pattern as V76, but attacker inserts malicious middleman implementation (CPIMP) that persists across upgrades by restoring itself in ERC-1967 slot. -- **FP:** Atomic init calldata in constructor. `_disableInitializers()` in implementation constructor. +- **D:** Bridge enforces a global throughput cap (total value per window) not segmented by user. Attacker fills the rate limit by bridging cheap tokens back and forth, blocking all legitimate users from bridging during the cooldown window. +- **FP:** Per-user rate limits. Rate limit segmented by token or route. Whitelist for high-value transfers. No global rate limit. -**156. Cross-Chain Reentrancy via Safe Transfer Callbacks** +**193. Self-Matched Orders Enable Wash Trading** -- **D:** Cross-chain receive function (`lzReceive`, `_credit`) calls `_safeMint` or `_safeTransfer` before updating supply/ownership counters. The `onERC721Received` / `onERC1155Received` callback re-enters to initiate another cross-chain send before state is finalized, creating duplicate tokens or double-spending. -- **FP:** State updates (counters, balances) committed before any safe transfer. `nonReentrant` on receive path. `_mint` used instead of `_safeMint` (no callback). Ref: Ackee Blockchain cross-chain reentrancy PoC. +- **D:** Order matching does not verify `maker != taker`. A user submits both sides of a trade to farm trading rewards, inflate volume metrics, bypass royalties (NFT), or extract fee rebates. +- **FP:** `require(makerOrder.signer != takerOrder.signer)`. Volume-based rewards use time-weighted averages resistant to single-block spikes. Royalty enforced regardless of counterparty. ---- +**194. Dutch Auction Price Decay Underflow** ---- +- **D:** `currentPrice = startPrice - (decayRate * elapsed)`. When the auction runs past the point where price should reach zero, the subtraction underflows — reverting on 0.8+ or wrapping to `type(uint256).max` on <0.8. Auction becomes unfinishable. +- **FP:** `currentPrice = elapsed >= duration ? reservePrice : startPrice - (decayRate * elapsed)`. Floor price enforced. `min()` used to cap decay. -**157. abi.encodePacked Hash Collision with Dynamic Types** +**195. Timelock Anchored to Deployment, Not Action** -- **D:** `keccak256(abi.encodePacked(a, b))` where two+ args are dynamic types (`string`, `bytes`, dynamic arrays). No length prefix means different inputs produce identical hashes. Affects permits, access control keys, nullifiers. -- **FP:** `abi.encode()` used instead. Only one dynamic type arg. All args fixed-size. +- **D:** Recovery or admin timelock measured from contract deployment or initialization, not from when the action was queued. Once the initial delay elapses, all future actions can execute instantly — the timelock is effectively permanent bypass. +- **FP:** Timelock resets on each queued action. `executeAfter = block.timestamp + delay` set at queue time. OZ TimelockController pattern. -**158. Signed Integer Mishandling (signextend / sar / slt Confusion)** +**196. Withdrawal Rate Limit Bypassed via Share Transfer** -- **D:** Assembly performs arithmetic on signed integers but uses unsigned opcodes. `shr` instead of `sar` (arithmetic shift right) loses the sign bit. `lt`/`gt` instead of `slt`/`sgt` treats negative numbers as huge positives. Missing `signextend` when loading a signed value smaller than 256 bits from calldata or storage. -- **FP:** Code consistently uses `sar`/`slt`/`sgt` for signed operations and `shr`/`lt`/`gt` for unsigned. `signextend(byteWidth - 1, val)` applied after loading sub-256-bit signed values. Code only operates on unsigned integers. +- **D:** Per-address withdrawal limit: `require(withdrawn[msg.sender] + amount <= limitPerPeriod)`. User transfers vault shares to a fresh address and withdraws from there — each new address gets a fresh limit. +- **FP:** Limit tracks the underlying position, not the address. Shares are non-transferable or transfer resets withdrawal allowance. KYC-bound withdrawal limits. -**159. Missing `_debit` / `_debitFrom` Authorization in OFT** +**197. Blacklist and Whitelist Not Mutually Exclusive** -- **D:** Custom OFT override of `_debit` or `_debitFrom` omits `require(msg.sender == _from || allowance[_from][msg.sender] >= amount)`. Anyone can bridge tokens from any holder's balance. Pattern: `_debit` that calls `_burn(_from, amount)` without verifying caller is authorized. -- **FP:** Standard LayerZero OFT implementation used without override. Custom `_debit` includes proper authorization. `_debit` only callable via `send()` which uses `msg.sender` as `_from`. +- **D:** An address can hold both `BLACKLISTED` and `WHITELISTED` roles simultaneously. Whitelist-gated paths do not check the blacklist — a blacklisted address bypasses restrictions by also being whitelisted. +- **FP:** Adding to blacklist auto-removes from whitelist (and vice versa). Single enum role per address. Both checks applied on every restricted path. -**160. Default Message Library Hijack (Configuration Drag-Along)** +**198. Dead Code After Return Statement** -- **D:** OApp does not explicitly pin its send/receive library version via `setSendVersion()` / `setReceiveVersion()` (or V2 `setSendLibrary()` / `setReceiveLibrary()`). It relies on the endpoint's mutable default. A malicious or compromised default library update silently applies to all unpinned OApps, bypassing DVN/Oracle validation. -- **FP:** OApp explicitly sets library versions in constructor or initialization. Configuration is immutable or governance-controlled with timelock. LayerZero V2 EndpointV2 is non-upgradeable (but library defaults are still mutable). +- **D:** Critical state update or validation (`require(success)`, `emit Event`, `nonce++`) placed after a `return` statement. The code is unreachable — failures go undetected, events are never emitted, state is never updated. +- **FP:** All critical logic precedes `return`. Compiler warnings for unreachable code are addressed. Linter enforces no-unreachable-code rule. -**161. ERC-1271 isValidSignature Delegated to Untrusted Module** +**199. Partial Redemption Fails to Reduce Tracked Total** -- **D:** `isValidSignature(hash, sig)` delegated to externally-supplied contract without whitelist check. Malicious module always returns `0x1626ba7e`, passing all signature checks. -- **FP:** Delegation only to owner-controlled whitelist. Module registry has timelock/guardian approval. +- **D:** Withdrawal queue partially fills a redemption request but does not proportionally reduce `totalQueuedShares` or `totalPendingAssets`. The vault's tracked total remains inflated, skewing share price for all other depositors. +- **FP:** Partial fill reduces tracked totals proportionally. Queue uses per-request tracking, not a global aggregate. Atomic full-or-nothing redemptions. -**162. Proxy Storage Slot Collision** +**200. TWAP Accumulator Not Updated During Sync or Skim** -- **D:** Proxy stores `implementation`/`admin` at sequential slots (0, 1); implementation also declares variables from slot 0. Implementation writes overwrite proxy pointers. -- **FP:** EIP-1967 randomized slots used. OZ Transparent/UUPS pattern. +- **D:** Pool's `sync()` or `skim()` function updates reserves but does not call `_update()` to advance the TWAP cumulative price accumulator. TWAP observations return stale values, enabling price manipulation through sync-then-trade sequences. +- **FP:** `sync()` calls `_update()` before overwriting reserves. TWAP sourced from external oracle, not internal accumulator. Uniswap V3 `observe()` used (accumulator updated on every swap). -**163. Counterfactual Wallet Initialization Parameters Not Bound to Deployed Address** +**201. Expired Oracle Version Silently Assigned Previous Price** -- **D:** Factory `createAccount` uses CREATE2 but salt doesn't incorporate all init params (especially owner). Attacker calls `createAccount` with different owner, deploying wallet they control to same counterfactual address. -- **FP:** Salt derived from all init params: `salt = keccak256(abi.encodePacked(owner, ...))`. Factory reverts if account exists. Initializer called atomically with deployment. +- **D:** In request-commit oracle patterns (Pyth, custom keepers), an expired or unfulfilled price request is assigned the last valid price instead of reverting or returning invalid. Pending orders execute at stale prices rather than being cancelled. +- **FP:** Expired versions return `price = 0` or `valid = false`, forcing order cancellation. Staleness threshold enforced per-request. Fallback oracle used for expired primaries. -**164. Oracle Price Update Front-Running** +**202. Funding Rate Derived from Single Trade Price** -- **D:** On-chain oracle update tx visible in mempool. Attacker front-runs a favorable price update by opening a position at the stale price, then profits when the update lands. Pattern: Pyth/Chainlink push-model where update tx is submitted to public mempool. -- **FP:** Protocol uses pull-based oracle (user submits price update atomically with their action). Private mempool (Flashbots Protect) for oracle updates. Price-update-and-action in single tx. +- **D:** Perpetual swap funding rate uses the last trade price as the mark price. A single self-trade at an extreme price skews the funding rate — the attacker profits from funding payments on their opposing position. +- **FP:** Mark price derived from TWAP or external oracle index. Funding rate capped per period. Volume-weighted average price used. -**165. Metamorphic Contract via CREATE2 + SELFDESTRUCT** +**203. Open Interest Tracked with Pre-Fee Position Size** -- **D:** `CREATE2` deployment where deployer can `selfdestruct` and redeploy different bytecode at same address. Governance-approved code swapped before execution. Ref: Tornado Cash Governance (2023). Post-Dencun (EIP-6780): largely mitigated except same-tx create-destroy-recreate. -- **FP:** Post-Dencun: `selfdestruct` no longer destroys code unless same tx as creation. `EXTCODEHASH` verified at execution time. Not deployed via `CREATE2` from mutable deployer. +- **D:** Open interest incremented by the full position size before fees are deducted. Actual exposure is smaller than recorded OI. Aggregate OI is permanently inflated, eventually hitting caps and blocking new positions. +- **FP:** OI incremented by post-fee position size. OI decremented on close by same amount used at open. Periodic OI reconciliation. -**166. Free Memory Pointer Corruption** +**204. Interest Accrual Rounds to Zero but Timestamp Advances** -- **D:** Assembly block writes to memory at fixed offsets (e.g., `mstore(0x80, val)`) without reading or updating the free memory pointer at `0x40`. Subsequent Solidity code allocates memory from stale pointer, overwriting the assembly-written data — or vice versa. Second pattern: assembly sets `mstore(0x40, newPtr)` to an incorrect value, causing later Solidity allocations to overlap prior data. -- **FP:** Assembly block reads `mload(0x40)`, writes only above that offset, then updates `mstore(0x40, newFreePtr)`. Or block only uses scratch space (`0x00`–`0x3f`). Block annotated `memory-safe` and verified to comply. Ref: Solidity optimizer bug in 0.8.13–0.8.14 mishandled cross-block memory writes. +- **D:** `interest = rate * timeDelta / SECONDS_PER_YEAR` rounds to zero when `timeDelta` is small (e.g., <207s at 15% APR). But `lastAccrualTime = block.timestamp` is still updated — the fractional interest is permanently lost, not deferred to the next accrual. +- **FP:** Accumulator uses sufficient precision (e.g., RAY = 1e27) to avoid zero rounding at per-block intervals. `lastAccrualTime` only advances when computed interest > 0. ---- +**205. Permissionless accrueInterest Griefing** ---- +- **D:** `accrueInterest()` is permissionless and updates `lastAccrualTime` on every call. Attacker calls it at very short intervals — each call computes zero interest (rounding) but advances the timestamp, systematically suppressing interest accumulation to near-zero. +- **FP:** Minimum accrual interval enforced: `require(block.timestamp - lastAccrualTime >= MIN_INTERVAL)`. Precision high enough that per-block interest > 0. Access-restricted accrual. -**167. ERC4626 Inflation Attack (First Depositor)** +**206. notifyRewardAmount Overwrites Active Reward Period** -- **D:** `shares = assets * totalSupply / totalAssets`. When `totalSupply == 0`, deposit 1 wei + donate inflates share price, victim's deposit rounds to 0 shares. No virtual offset. -- **FP:** OZ ERC4626 with `_decimalsOffset()`. Dead shares minted to `address(0)` at init. +- **D:** Calling `notifyRewardAmount(newAmount)` replaces the current reward period. Remaining undistributed rewards from the old period are silently lost — not carried forward. Admin or attacker can erase pending rewards by notifying a smaller amount. +- **FP:** New notification adds to remaining: `rewardRate = (newAmount + remaining) / duration`. Only callable by designated distributor with timelock. Remaining rewards refunded before reset. -**168. Storage Layout Shift on Upgrade** +**207. Governance Proposal Executable Before Voting Period Ends** -- **D:** V2 inserts new state variable in middle of contract instead of appending. Subsequent variables shift slots, corrupting state. Also: changing variable type between versions shifts slot boundaries. -- **FP:** New variables only appended. OZ storage layout validation in CI. Variable types unchanged between versions. +- **D:** `execute()` checks quorum and majority but not `block.timestamp >= proposal.endTime`. Once quorum is met, the proposal can be executed immediately — cutting the voting window short, preventing opposing votes from being cast. +- **FP:** `require(block.timestamp >= proposal.endTime)` in execute. OZ Governor enforces `ProposalState.Succeeded` which requires voting period to have ended. -**169. Hardcoded Calldataload Offset Bypass via Non-Canonical ABI Encoding** +**208. Partial Liquidation Leaves Position in Worse State** -- **D:** Assembly reads a field at hardcoded calldata offset (`calldataload(164)`) assuming standard ABI layout. Attacker crafts non-canonical encoding — manipulated dynamic-type offset pointers or padding — so a different value sits at the expected position. -- **FP:** Field decoded via `abi.decode()` (compiler bounds-checked). No hardcoded `calldataload` offsets — parameters extracted through Solidity's typed calldata accessors. `calldatasize() >= expected` validated before reading. +- **D:** Partial liquidation seizes some collateral but does not enforce a minimum post-liquidation health factor. Liquidator cherry-picks the most valuable collateral, leaving the position with worse health than before — approaching full insolvency without triggering full liquidation. +- **FP:** Post-liquidation health factor check: `require(healthFactor(position) >= MIN_HF)`. Full liquidation triggered below a floor threshold. Liquidator must bring position to target health factor. -**170. Calldata Input Malleability** +**209. Delegation to address(0) Blocks Token Transfers** -- **D:** Contract hashes raw calldata for uniqueness (`processedHashes[keccak256(msg.data)]`). Dynamic-type ABI encoding uses offset pointers — multiple distinct calldata layouts decode to identical values. Attacker bypasses dedup with semantically equivalent but bytewise-different calldata. -- **FP:** Uniqueness check hashes decoded parameters: `keccak256(abi.encode(decodedParams))`. Nonce-based replay protection. Only fixed-size types in signature (no encoding ambiguity). +- **D:** Delegating votes to `address(0)` causes `_beforeTokenTransfer` or `_update` hooks to revert when attempting to modify the zero-address delegate's checkpoint. All subsequent transfers and burns for that token holder permanently revert. +- **FP:** Delegation to `address(0)` treated as undelegation (no-op or clears delegation). Hook skips checkpoint update when delegate is `address(0)`. OZ Votes implementation handles this case. -**171. Reward Accrual During Zero-Depositor Period** +**210. ERC4626 maxDeposit Returns Non-Zero When Paused** -- **D:** Time-based reward distribution starts at vault deployment but no depositors exist yet. First depositor claims all rewards accumulated during the empty period regardless of deposit size or timing. -- **FP:** Rewards only accrue when `totalSupply > 0`. Reward start time set on first deposit. Unclaimed pre-deposit rewards sent to treasury or burned. +- **D:** `maxDeposit()` returns `type(uint256).max` even when the vault is paused. Integrating protocols (aggregators, routers) read this as "deposits accepted," attempt a deposit, and revert. Per ERC4626, `maxDeposit` must return 0 when deposits would revert. +- **FP:** `maxDeposit()` returns 0 when paused. OZ ERC4626 with pausing override. Integrators use `try deposit()` with fallback. -**172. MEV Withdrawal Before Bad Debt Socialization** +**211. Deprecated Gauge Blocks Claiming Accrued Rewards** -- **D:** External event (liquidation, exploit, depeg) causes vault loss. MEV actor observes pending loss-causing tx in mempool and front-runs a withdrawal at pre-loss share price, leaving remaining depositors to absorb the full loss. -- **FP:** Withdrawals require time-delayed request queue (epoch-based or cooldown). Loss realization and share price update are atomic. Private mempool used for liquidation txs. +- **D:** Killing or deprecating a gauge clears future distributions but also blocks the `claimReward()` path for already-accrued, unclaimed rewards. Users who earned rewards before deprecation cannot retrieve them. +- **FP:** Kill only stops future accrual — claim function remains active for pre-kill balances. Rewards swept to fallback address on deprecation. Emergency claim path bypasses active-gauge check. -**173. Vault Insolvency via Accumulated Rounding Dust** +**212. Liquidation Blocked by External Pool Illiquidity** -- **D:** Vault tracks `totalAssets` as a storage variable separate from `token.balanceOf(vault)`. Solidity's floor rounding on each deposit/withdrawal creates tiny overages — user receives 1 wei more than burned shares represent. Over many operations `totalAssets` exceeds actual balance, causing last withdrawers to revert. -- **FP:** Rounding consistently favors the vault (round shares up on deposit, round assets down on withdrawal). OZ Math with `Rounding.Ceil`/`Rounding.Floor` applied correctly. +- **D:** Liquidation function swaps collateral for debt token via an external DEX. If the DEX pool is drained or lacks liquidity, the swap reverts, making liquidation impossible. Bad debt accumulates while the pool remains illiquid. +- **FP:** Liquidation accepts collateral directly without swap. Fallback liquidation path uses a different DEX or oracle price. Liquidator provides debt token and receives collateral. -**174. FIFO Withdrawal Ordering Degrades Yield** +**213. No-Bid Auction Fails to Clear State** -- **D:** Aggregator vault withdraws from sub-vaults in fixed FIFO order, depleting highest-APY vaults first. Remaining capital concentrates in lowest-yield positions, reducing overall returns for all depositors. -- **FP:** Withdrawal ordering sorted by APY ascending (lowest-yield first). Dynamic rebalancing after withdrawals. Single underlying vault (no ordering issue). +- **D:** Auction expires without any bids. The finalization function does not clear lien, auction, or escrow data — collateral remains locked in the auction contract with no path to return it to the owner or trigger a new auction. +- **FP:** No-bid finalization returns collateral to owner and clears all associated state. Re-auction mechanism triggered automatically. Timeout-based collateral release. -**175. ERC4626 convertToAssets Used Instead of previewWithdraw** +**214. Position Reduction Triggers Liquidation** -- **D:** Integration calls `convertToAssets(shares)` to estimate withdrawal proceeds. Per ERC4626 spec this excludes fees and slippage — actual redeemable amount is lower. Downstream logic (health checks, rebalancing, UI) operates on inflated values. -- **FP:** `previewWithdraw()` or `previewRedeem()` used for actual withdrawal estimates. Vault charges no withdrawal fees. Fee delta accounted separately. +- **D:** User attempts to improve health by partially repaying debt or withdrawing a small amount of excess collateral. The intermediate state (after collateral removal, before debt reduction) crosses the liquidation threshold — a bot liquidates the position mid-transaction or in the same block. +- **FP:** Repay and collateral changes are atomic (single function). Health check applied only to final state, not intermediate. Liquidation grace period after position modification. -**176. Unclaimed Reward Tokens from Underlying Protocol** +**215. Repeated Liquidation of Same Position** -- **D:** Vault deposits into yield protocol (Morpho, Aave, Convex) that emits reward tokens. Vault never calls `claim()` or lacks logic to distribute claimed rewards to depositors. Rewards accumulate indefinitely in the vault or underlying protocol, inaccessible to shareholders. -- **FP:** Explicit `claimRewards()` function harvests and distributes. Reward tokens tracked dynamically (mapping, not hardcoded list). Vault sweeps unexpected token balances to treasury. +- **D:** Liquidation function does not flag the position as liquidated. After partial liquidation, the position still appears undercollateralized — a second liquidator (or the same one) liquidates again, seizing collateral beyond what was intended. +- **FP:** Position marked as `liquidated` or deleted after processing. Liquidation requires `status != Liquidated`. Post-liquidation health check prevents re-triggering. -**177. Idle Asset Dilution from Sub-Vault Deposit Caps** +**216. Loan State Transition Before Interest Settlement** -- **D:** Parent/aggregator vault accepts deposits without checking sub-vault capacity. When sub-vaults hit their deposit caps, excess assets sit idle in the parent — earning zero yield but diluting share price for all depositors. -- **FP:** `maxDeposit()` reflects combined sub-vault remaining capacity. Deposits revert when no productive capacity remains. Idle assets auto-routed to fallback yield source. +- **D:** Repaying principal sets the loan state to `Repaid` before accrued interest is settled. Once in `Repaid` state, the interest accrual function skips the loan — all accumulated interest becomes permanently uncollectable. +- **FP:** `settleInterest()` called before state transition. Interest added to repayment amount: `require(msg.value >= principal + accruedInterest)`. State transition only after full settlement. -**227. Protocol Fee Inflates Reward Accumulator** +**217. Protocol Fee Inflates Reward Accumulator** - **D:** Protocol fee routed to treasury is processed through the same `rewardPerToken` accumulator as staker rewards. The accumulator increments as if all distributed tokens go to stakers, but part went to treasury — stakers' `earned()` returns more than the contract holds. - **FP:** Fee deducted before updating accumulator: `rewardPerToken += (reward - fee) / totalStaked`. Separate accounting for fees and rewards. Fee transferred directly, not through reward distribution. -**228. Profit Tracking Underflow Blocks Withdrawals** +**218. Profit Tracking Underflow Blocks Withdrawals** - **D:** Vault tracks cumulative strategy profit. When a strategy reports a loss exceeding its recorded profit, `totalProfit -= loss` underflows (reverts on 0.8+). All withdrawal functions that read `totalProfit` are permanently bricked. - **FP:** Loss capped: `totalProfit -= min(loss, totalProfit)`. Signed integer used for profit/loss tracking. Per-strategy profit tracking (one strategy's loss doesn't affect others). -**229. Share Redemption at Optimistic Rate** +**219. Share Redemption at Optimistic Rate** - **D:** Shares redeemed at a projected end-of-term exchange rate rather than the current realized rate. Early redeemers receive more than their proportional share of actual assets — late redeemers find the vault depleted. -- **FP:** Redemption uses current `totalAssets / totalSupply`, not projected rate. Withdrawal queue with pro-rata distribution. Mark-to-market valuation updated on each interaction. \ No newline at end of file + +**220. DoS via Reverting External Call in Loop** + +- **D:** Withdrawal, burn, or claim function loops over a dynamic list and makes an external call per iteration (token transfer, swap, oracle read, callback). If any single call reverts — token paused/blacklisted, pool illiquid, recipient rejects — the entire function reverts, blocking all users from the operation even for the unaffected entries. Pattern: `for (i < list.length) { externalCall(list[i]); }` in a withdrawal/burn/claim path with no alternative exit. +- **FP:** Each external call wrapped in `try/catch` (skip on failure). Separate per-item withdrawal function exists. Admin can remove problematic entries from the list and users can still access remaining assets. diff --git a/solidity-auditor/references/judging.md b/solidity-auditor/references/judging.md index bc00a05..662b7dc 100644 --- a/solidity-auditor/references/judging.md +++ b/solidity-auditor/references/judging.md @@ -4,11 +4,11 @@ Each finding passes a false-positive gate, then gets a confidence score (how cer ## FP Gate -Every finding must pass all three checks. If any check fails, drop the finding — do not score or report it. +Every finding must pass all three checks. If check 2 or 3 fails, drop the finding. If only check 1 fails (idea is sound but path is not fully concrete), move the finding to the **Leads** section instead of dropping it. -1. You can trace a concrete attack path: caller → function call → state change → loss/impact. Evaluate what the code _allows_, not what the deployer _might choose_. +1. You can trace a concrete attack path: caller → function call → state change → loss/impact. Evaluate what the code _allows_, not what the deployer _might choose_. For any finding involving token transfers or swaps, verify which token moves IN and which moves OUT before concluding direction of impact. 2. The entry point is reachable by the attacker (check modifiers, `msg.sender` guards, `onlyOwner`, access control). -3. No existing guard already prevents the attack (`require`, `if`-revert, reentrancy lock, allowance check, etc.). +3. No existing guard already prevents the attack (`require`, `if`-revert, reentrancy lock, allowance check, structural invariant like MINIMUM_LIQUIDITY preventing zero state, etc.). ## Confidence Score @@ -24,6 +24,10 @@ Confidence indicator: `[score]` (e.g., `[95]`, `[75]`, `[60]`). Findings below the confidence threshold (default 75) are still included in the report table but do not get a **Fix** section — description only. +## Leads + +Findings where the general idea is sound but check 1 fails (cannot trace a fully concrete path). These are NOT scored or given a Fix — just a title and 1-2 sentence description of the suspected issue and why the path couldn't be completed. Printed in a separate section below findings in the report for human review. + ## Do Not Report - Anything a linter, compiler, or seasoned developer would dismiss — INFO-level notes, gas micro-optimizations, naming, NatSpec, redundant comments. diff --git a/solidity-auditor/references/report-formatting.md b/solidity-auditor/references/report-formatting.md index 7150f2b..8e5aadb 100644 --- a/solidity-auditor/references/report-formatting.md +++ b/solidity-auditor/references/report-formatting.md @@ -81,6 +81,15 @@ Findings List --- +## Leads + +_Suspected issues where a concrete attack path could not be fully traced. Included for manual review — not scored._ + +- **<Title>** — `Contract.function` — <1-2 sentence description of the suspected issue and why the path couldn't be completed> +- **<Title>** — `Contract.function` — <1-2 sentence description> + +--- + > ⚠️ This review was performed by an AI assistant. AI analysis can never verify the complete absence of vulnerabilities and no guarantee of security is given. Team security reviews, bug bounty programs, and on-chain monitoring are strongly recommended. For a consultation regarding your projects' security, visit [https://www.pashov.com](https://www.pashov.com) ```` From 6f1f670c45fe58a2fb054a10f0aa9eb7ea77b460 Mon Sep 17 00:00:00 2001 From: pashov <krum@pashov.com> Date: Sat, 7 Mar 2026 18:55:16 +0200 Subject: [PATCH 3/4] update(solidity-auditor): restructure agents, add FP gate, optimize scan output - Split agents/ into hacking-agents/ and operations-agents/ - Add dedicated fp-gate-agent.md for validation read discipline - Enforce compact skip/drop classification format in vector-scan-agent - Redefine LEADs as concrete code smell trails (not just incomplete paths) - Add code_smells field to LEAD output and report template - Strip redundant headers from attack-vectors files - Revert bundle reorder (instructions-last works better) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --- solidity-auditor/SKILL.md | 4 +- .../references/agents/vector-scan-agent.md | 36 ------------- .../attack-vectors/attack-vectors-1.md | 4 -- .../attack-vectors/attack-vectors-2.md | 4 -- .../attack-vectors/attack-vectors-3.md | 4 -- .../attack-vectors/attack-vectors-4.md | 4 -- .../adversarial-reasoning-agent.md | 0 .../hacking-agents/vector-scan-agent.md | 54 +++++++++++++++++++ solidity-auditor/references/judging.md | 4 +- .../operations-agents/fp-gate-agent.md | 28 ++++++++++ .../references/report-formatting.md | 7 ++- 11 files changed, 89 insertions(+), 60 deletions(-) delete mode 100644 solidity-auditor/references/agents/vector-scan-agent.md rename solidity-auditor/references/{agents => hacking-agents}/adversarial-reasoning-agent.md (100%) create mode 100644 solidity-auditor/references/hacking-agents/vector-scan-agent.md create mode 100644 solidity-auditor/references/operations-agents/fp-gate-agent.md diff --git a/solidity-auditor/SKILL.md b/solidity-auditor/SKILL.md index 7885982..f22d14c 100644 --- a/solidity-auditor/SKILL.md +++ b/solidity-auditor/SKILL.md @@ -31,7 +31,7 @@ Then continue normally. If the fetch fails (offline, timeout), skip silently. **Turn 1 — Discover.** Print the banner, then in the same message make parallel tool calls: (a) Bash `find` for in-scope `.sol` files per mode selection, (b) Glob for `**/references/attack-vectors/attack-vectors-1.md` and extract the `references/` directory path (two levels up), (c) ToolSearch `select:Agent` to pre-load the Agent tool for Turn 3. Use this resolved path as `{resolved_path}` for all subsequent references. -**Turn 2 — Prepare.** In a single message, make parallel tool calls: (a) Read `{resolved_path}/report-formatting.md`, (b) Bash: create per-agent bundle files in a **single command**. Always create `/tmp/audit-agent-{1,2,3,4}-bundle.md` — each concatenates **all** in-scope `.sol` files (with `### path` headers and fenced code blocks), then `{resolved_path}/attack-vectors/attack-vectors-N.md`, then `{resolved_path}/agents/vector-scan-agent.md`. In **DEEP** mode, also create `/tmp/audit-agent-5-bundle.md` — concatenates all in-scope `.sol` files (same format), then `{resolved_path}/agents/adversarial-reasoning-agent.md`. Also create `/tmp/audit-fp-gate-bundle.md` — concatenates all in-scope `.sol` files (same format), then `{resolved_path}/judging.md`, then `{resolved_path}/report-formatting.md`. Print line counts for every bundle created. Every agent receives the full codebase — only the trailing reference file differs. Do NOT read or inline any file content into agent prompts — the bundle files replace that entirely. +**Turn 2 — Prepare.** In a single message, make parallel tool calls: (a) Read `{resolved_path}/report-formatting.md`, (b) Bash: create per-agent bundle files in a **single command**. Always create `/tmp/audit-agent-{1,2,3,4}-bundle.md` — each concatenates **all** in-scope `.sol` files (with `### path` headers and fenced code blocks), then `{resolved_path}/attack-vectors/attack-vectors-N.md`, then `{resolved_path}/hacking-agents/vector-scan-agent.md`. In **DEEP** mode, also create `/tmp/audit-agent-5-bundle.md` — concatenates all in-scope `.sol` files (same format), then `{resolved_path}/hacking-agents/adversarial-reasoning-agent.md`. Also create `/tmp/audit-fp-gate-bundle.md` — concatenates all in-scope `.sol` files (same format), then `{resolved_path}/judging.md`, then `{resolved_path}/report-formatting.md`, then `{resolved_path}/operations-agents/fp-gate-agent.md`. Print line counts for every bundle created. Every agent receives the full codebase — only the trailing reference file differs. Do NOT read or inline any file content into agent prompts — the bundle files replace that entirely. **Turn 3 — Spawn.** In a single message, spawn all agents as parallel foreground Agent tool calls (do NOT use `run_in_background`). Always spawn Agents 1–4. Only spawn Agent 5 when the mode is **DEEP**. @@ -42,7 +42,7 @@ Then continue normally. If the fetch fails (offline, timeout), skip silently. 1. **Pre-filter.** Scan all raw findings and immediately drop any that are informational-only (error messages, naming, gas, NatSpec, admin-only parameter setting, missing events, centralization without concrete exploit path). One word per drop — no analysis. 2. **Deduplicate.** Group surviving findings by root cause (same contract + same function + same bug class). Keep only the most detailed version of each group, drop the rest. List groups: `"Chainlink staleness: Agent 2, Agent 3, Agent 4 → keep Agent 3"`. -3. **Validate.** Spawn a single foreground Agent. Paste all deduplicated findings verbatim into the prompt, then add: `Validation bundle: /tmp/audit-fp-gate-bundle.md (XXXX lines). Read the entire bundle in parallel 2000-line chunks on your first tool-call turn — these are your ONLY reads, make ZERO additional tool calls after. Then for each finding: verify the path against the actual source code, apply the 3 FP gate checks, and compute the confidence score. Finally, format the surviving findings into the final report per the report-formatting template at the end of the bundle — sort PASS findings by confidence highest-first, re-number sequentially, include scope table + findings with description and fix diffs (omit fix for findings below 80 confidence) + findings list table. Route LEAD items to the Leads section. Return the complete formatted report as your response.` Substitute the real line count. In the prompt, also include the mode and the list of files reviewed so the agent can fill in the scope table. +3. **Validate.** Spawn a single foreground Agent. Do NOT paste agent instructions into the prompt — they are already inside the bundle. Paste all deduplicated findings and leads verbatim into the prompt, then add: `Validation bundle: /tmp/audit-fp-gate-bundle.md (XXXX lines). Mode: <mode>. Files reviewed: <file list>.` Substitute the real line count, mode, and file list. 4. **Output.** Print the validation agent's formatted report verbatim. If `--file-output` is set, also write it to a file (path per report-formatting.md) and print the path. ## Banner diff --git a/solidity-auditor/references/agents/vector-scan-agent.md b/solidity-auditor/references/agents/vector-scan-agent.md deleted file mode 100644 index 3f41a79..0000000 --- a/solidity-auditor/references/agents/vector-scan-agent.md +++ /dev/null @@ -1,36 +0,0 @@ -# Vector Scan Agent Instructions - -You are a security auditor scanning Solidity contracts for vulnerabilities. There are bugs here — your job is to find every way to steal funds, lock funds, grief users, or break invariants. Do not accept "no findings" easily. - -## Tool-Call Budget — MANDATORY - -You have a **hard cap of 1 tool-call turn**: the initial parallel Read of your bundle file. After that turn, you make **ZERO additional tool calls** — no Read, no Grep, no Glob, no Bash, nothing. All analysis happens from the bundle content already in your context. Any extra tool call is wasted time and money. Violating this rule is a critical failure. - -## Critical Output Rule - -You communicate results back ONLY through your final text response. Do not output findings during analysis. Collect all findings internally and include them ALL in your final response message. Your final response IS the deliverable. Do NOT write any files — no report files, no output files. Your only job is to return findings as text. - -## Workflow - -1. Read your bundle file in **parallel 2000-line chunks** on your first turn. The line count is in your prompt — compute the offsets and issue all Read calls at once (e.g., for a 6000-line file: `Read(file, limit=2000)`, `Read(file, offset=2000, limit=2000)`, `Read(file, offset=4000, limit=2000)`). Do NOT read without a limit. These are your ONLY file reads — do NOT read any other file or re-read any chunk after this step. **After this step you must not call any tool.** -2. **Scan pass.** For each vector, silently skip it if the named construct AND underlying concept are both absent — produce **zero output** for skipped vectors, do not list them, do not explain why they were skipped. For every remaining vector: - - **DROP** — guard unambiguously prevents the attack. Output ONLY the vector ID: `V22: DROP`. No path, no explanation, never reconsider. - - **INVESTIGATE** — no guard, partial guard, or guard that might not cover all paths. Write a 1-line path trace: - ``` - V15: path: deposit() → _expandLock() → lockStart reset | guard: none | verdict: INVESTIGATE - ``` - Trace the call chain from external entry point to the vulnerable line, list every modifier/require/state guard on that path. - -3. **Deep analysis (INVESTIGATE only).** For each INVESTIGATE vector, expand to ≤5 lines: verify the entry point is state-changing, trace the full attack path including cross-function and cross-contract interactions, and check whether any indirect guard (CEI pattern, mutex in a parent call, arithmetic that reverts) closes the gap. Final verdict: **CONFIRM** or **DROP**. -4. **Composability check.** Only if you have 2+ confirmed findings: do any two compound (e.g., DoS + access control = total fund lockout)? If so, note the interaction in the higher-confidence finding's description. -5. For each confirmed finding, output in this exact format — nothing more: - ``` - FINDING | location: Contract.function - path: caller → function → state change → impact - entry: <permissionless | admin-only | restricted to ContractName> - guards: <none | guard1, guard2, ...> - description: <one sentence> - fix: <one-sentence suggestion or short diff> - ``` -6. Do not output findings during analysis — compile them all and return them together as your final response. -7. **Hard stop.** After the deep pass, STOP — do not re-examine eliminated vectors, do not produce "additional findings" or scan outside your assigned vector set, do not "revisit"/"reconsider" anything. Output your findings, or "No findings." if none survive. diff --git a/solidity-auditor/references/attack-vectors/attack-vectors-1.md b/solidity-auditor/references/attack-vectors/attack-vectors-1.md index 18057d0..5a7b09e 100644 --- a/solidity-auditor/references/attack-vectors/attack-vectors-1.md +++ b/solidity-auditor/references/attack-vectors/attack-vectors-1.md @@ -1,7 +1,3 @@ -# Attack Vectors Reference (1/4) - -220 total attack vectors - --- **1. Signature Malleability** diff --git a/solidity-auditor/references/attack-vectors/attack-vectors-2.md b/solidity-auditor/references/attack-vectors/attack-vectors-2.md index 605653e..37db5b3 100644 --- a/solidity-auditor/references/attack-vectors/attack-vectors-2.md +++ b/solidity-auditor/references/attack-vectors/attack-vectors-2.md @@ -1,7 +1,3 @@ -# Attack Vectors Reference (2/4) - -220 total attack vectors - --- **56. Cross-Chain Address Ownership Variance** diff --git a/solidity-auditor/references/attack-vectors/attack-vectors-3.md b/solidity-auditor/references/attack-vectors/attack-vectors-3.md index e533731..441ecd3 100644 --- a/solidity-auditor/references/attack-vectors/attack-vectors-3.md +++ b/solidity-auditor/references/attack-vectors/attack-vectors-3.md @@ -1,7 +1,3 @@ -# Attack Vectors Reference (3/4) - -220 total attack vectors - --- **111. Nested Mapping Inside Struct Not Cleared on `delete`** diff --git a/solidity-auditor/references/attack-vectors/attack-vectors-4.md b/solidity-auditor/references/attack-vectors/attack-vectors-4.md index e823645..80cac4c 100644 --- a/solidity-auditor/references/attack-vectors/attack-vectors-4.md +++ b/solidity-auditor/references/attack-vectors/attack-vectors-4.md @@ -1,7 +1,3 @@ -# Attack Vectors Reference (4/4) - -220 total attack vectors - --- **166. Calldata Input Malleability** diff --git a/solidity-auditor/references/agents/adversarial-reasoning-agent.md b/solidity-auditor/references/hacking-agents/adversarial-reasoning-agent.md similarity index 100% rename from solidity-auditor/references/agents/adversarial-reasoning-agent.md rename to solidity-auditor/references/hacking-agents/adversarial-reasoning-agent.md diff --git a/solidity-auditor/references/hacking-agents/vector-scan-agent.md b/solidity-auditor/references/hacking-agents/vector-scan-agent.md new file mode 100644 index 0000000..1100f40 --- /dev/null +++ b/solidity-auditor/references/hacking-agents/vector-scan-agent.md @@ -0,0 +1,54 @@ +# Vector Scan Agent Instructions + +You are a security auditor scanning Solidity contracts for a specific set of attack vectors listed in your bundle. Your ONLY job is to grind through those vectors — trace each one against the codebase, determine if it manifests, and report what you find. Do not freelance beyond your assigned vectors. Other agents running in parallel cover every other vector against the same code. If none of your vectors match, that is a valid result. + +## Critical Output Rule + +You communicate results back ONLY through your final text response. Do not output findings during analysis. Collect all findings internally and include them ALL in your final response message. Your final response IS the deliverable. Do NOT write any files — no report files, no output files. Your only job is to return findings as text. + +## Workflow + +1. Read your bundle file in **parallel 2000-line chunks** on your first turn. The line count is in your prompt — compute the offsets and issue all Read calls at once (e.g., for a 6000-line file: `Read(file, limit=2000)`, `Read(file, offset=2000, limit=2000)`, `Read(file, offset=4000, limit=2000)`). Do NOT read without a limit. These are your ONLY file reads — do NOT read any other file or re-read any chunk after this step. **After this step you must not call any tool.** +2. **Scan pass.** Process every vector from the attack-vectors file in your bundle. Classify each into exactly one tier: + - **SKIP** — the named construct AND underlying concept are both absent from this codebase. + - **BORDERLINE** — the named construct is absent but the underlying vulnerability concept could manifest through a different mechanism (e.g., "stale cached ERC20 balance" when the code caches cross-contract AMM reserves; "ERC777 reentrancy" when there are flash-swap callbacks). Promote to INVESTIGATE if you can (a) name the specific function where the concept manifests AND (b) describe in one sentence how the exploit would work; otherwise move to SKIP. + - **DROP** — construct/concept is present but a guard unambiguously prevents the attack. + - **INVESTIGATE** — no guard, partial guard, or guard that might not cover all paths. Write a 1-line path trace: + ``` + V15: path: deposit() → _expandLock() → lockStart reset | guard: none | verdict: INVESTIGATE + ``` + + **Output format** — output ONLY this compact format. Do NOT write per-vector explanations or analysis during the scan pass — save all reasoning for deep analysis of INVESTIGATE vectors: + ``` + Skip: V1,V2,V5,V6,V10,V12,V13 + Drop: V4,V9,V11,V14 + Investigate: V3,V7,V8 + Total: 14 classified + ``` + Every vector must appear in exactly one tier. Verify the total matches your vector count. If it doesn't, re-scan. + +3. **Deep analysis (INVESTIGATE only).** For each INVESTIGATE vector, expand to ≤5 lines: verify the entry point is state-changing, trace the full attack path including cross-function and cross-contract interactions, and check whether any indirect guard (CEI pattern, mutex in a parent call, arithmetic that reverts) closes the gap. Final verdict: + - **CONFIRM** — attack path is concrete and unguarded. Output as FINDING. + - **DROP** — guard definitively prevents the attack. One line, never reconsider. + - **LEAD** — you found concrete code smells (missing guards, unsafe arithmetic, unvalidated external input) and traced a partial attack path, but ran out of analysis budget to fully confirm or rule out exploitation. LEADs are not false positives — they are real vulnerability trails that need deeper investigation. Default to LEAD over DROP when the code smells are present but the full exploit chain can't be completed in one pass. + + If zero INVESTIGATE vectors remain after the scan pass, output your classification and stop immediately. Do not search for additional issues. +4. **Composability check.** Only if you have 2+ confirmed findings: do any two compound (e.g., DoS + access control = total fund lockout)? If so, note the interaction in the higher-confidence finding's description. +5. For each confirmed finding, output in this exact format — nothing more: + ``` + FINDING | location: Contract.function + path: caller → function → state change → impact + entry: <permissionless | admin-only | restricted to ContractName> + guards: <none | guard1, guard2, ...> + description: <one sentence> + fix: <one-sentence suggestion or short diff> + ``` + For each lead, output: + ``` + LEAD | location: Contract.function + code_smells: <concrete issues found: missing guard, unsafe arithmetic, unvalidated input, etc.> + description: <one sentence explaining the vulnerability trail and what remains unverified> + ``` +6. Do not output findings during analysis — compile them all and return them together as your final response. +7. **Scope discipline.** You are ONLY responsible for your assigned vectors. Other agents running in parallel cover every other vector against the same codebase. Do NOT analyze code patterns or vulnerabilities outside your vector set — that work is already being done and any duplicate analysis is pure waste. +8. **Hard stop.** After the deep pass, STOP — do not re-examine eliminated vectors, do not produce "additional findings" outside your assigned vector set, do not "revisit"/"reconsider" anything. Output your findings and leads, or "No findings." if none survive. diff --git a/solidity-auditor/references/judging.md b/solidity-auditor/references/judging.md index 662b7dc..f5ea3ae 100644 --- a/solidity-auditor/references/judging.md +++ b/solidity-auditor/references/judging.md @@ -4,7 +4,7 @@ Each finding passes a false-positive gate, then gets a confidence score (how cer ## FP Gate -Every finding must pass all three checks. If check 2 or 3 fails, drop the finding. If only check 1 fails (idea is sound but path is not fully concrete), move the finding to the **Leads** section instead of dropping it. +Every finding must pass all three checks. If check 2 or 3 fails, drop the finding. If only check 1 fails but concrete code smells are present (missing guards, unsafe arithmetic, unvalidated input), move the finding to the **Leads** section instead of dropping it. 1. You can trace a concrete attack path: caller → function call → state change → loss/impact. Evaluate what the code _allows_, not what the deployer _might choose_. For any finding involving token transfers or swaps, verify which token moves IN and which moves OUT before concluding direction of impact. 2. The entry point is reachable by the attacker (check modifiers, `msg.sender` guards, `onlyOwner`, access control). @@ -26,7 +26,7 @@ Findings below the confidence threshold (default 75) are still included in the r ## Leads -Findings where the general idea is sound but check 1 fails (cannot trace a fully concrete path). These are NOT scored or given a Fix — just a title and 1-2 sentence description of the suspected issue and why the path couldn't be completed. Printed in a separate section below findings in the report for human review. +Vulnerability trails where the scanning agent found concrete code smells (missing guards, unsafe arithmetic, unvalidated external input) and traced a partial attack path, but ran out of analysis budget to fully confirm exploitation. Leads are NOT false positives — they are high-signal trails that need deeper manual investigation. Not scored or given a Fix — just a title, the code smells found, and a 1-2 sentence description of the vulnerability trail and what remains unverified. ## Do Not Report diff --git a/solidity-auditor/references/operations-agents/fp-gate-agent.md b/solidity-auditor/references/operations-agents/fp-gate-agent.md new file mode 100644 index 0000000..b144ff9 --- /dev/null +++ b/solidity-auditor/references/operations-agents/fp-gate-agent.md @@ -0,0 +1,28 @@ +# FP Gate Validation Agent Instructions + +You are a security finding validator. You receive deduplicated findings from parallel scan agents and your job is to verify each one against the actual source code, apply the FP gate checks, score confidence, and format the final report. You do NOT search for new vulnerabilities — only validate what you are given. + +## Critical Output Rule + +You communicate results back ONLY through your final text response. Your final response IS the deliverable — a complete formatted audit report. Do NOT write any files. + +## Workflow + +1. Read your bundle file in **parallel 2000-line chunks** on your first turn. The line count is in your prompt — compute the offsets and issue all Read calls at once (e.g., for a 6000-line file: `Read(file, limit=2000)`, `Read(file, offset=2000, limit=2000)`, `Read(file, offset=4000, limit=2000)`). Do NOT read without a limit. These are your ONLY file reads — do NOT read any other file or re-read any chunk after this step. **After this step you must not call any tool.** The bundle contains ALL source code, the FP gate rules, and the report template — everything you need is in it. +2. **Validate each finding.** For every finding in your prompt, do three things: + - **Verify the path** — confirm the function, state change, and impact described in the finding match what the source code in your bundle actually does. + - **Apply the 3 FP gate checks** from the judging rules in your bundle. If check 2 or 3 fails, drop the finding. If only check 1 fails (idea is sound but path is not fully concrete), route to LEAD. + - **Score confidence** per the scoring rules in your bundle. Start at 100, apply deductions. +3. **Format the report.** Using the report-formatting template at the end of your bundle: + - Sort PASS findings by confidence (highest first), number sequentially. + - Include scope table, description, and fix diffs for findings with confidence >= 80. + - Include description only (no fix) for findings below 80. + - Route LEAD items to the Leads section (no score, no fix — just title and description). + - Include the findings list table at the bottom. +4. **Return the complete formatted report** as your final response. Nothing else. + +## Hard Constraints + +- **No extra reads.** The bundle has every source file, the FP gate rules, and the report template. Do NOT read individual `.sol` files, grep for functions, or make any tool call after step 1. If you cannot find a function in the bundle, the finding references code that doesn't exist — drop it. +- **No new findings.** You are a validator, not a scanner. Do not report vulnerabilities that weren't in your input findings. +- **No re-reads.** Do not re-read any chunk of the bundle. You get one pass — use it. diff --git a/solidity-auditor/references/report-formatting.md b/solidity-auditor/references/report-formatting.md index 8e5aadb..9a0e155 100644 --- a/solidity-auditor/references/report-formatting.md +++ b/solidity-auditor/references/report-formatting.md @@ -77,16 +77,15 @@ Findings List | 1 | [95] | <title> | | 2 | [82] | <title> | | 3 | [75] | <title> | -| 4 | [60] | <title> | --- ## Leads -_Suspected issues where a concrete attack path could not be fully traced. Included for manual review — not scored._ +_Vulnerability trails with concrete code smells where the full exploit path could not be completed in one analysis pass. These are not false positives — they are high-signal leads for manual review. Not scored._ -- **<Title>** — `Contract.function` — <1-2 sentence description of the suspected issue and why the path couldn't be completed> -- **<Title>** — `Contract.function` — <1-2 sentence description> +- **<Title>** — `Contract.function` — Code smells: <missing guard, unsafe arithmetic, etc.> — <1-2 sentence description of the trail and what remains unverified> +- **<Title>** — `Contract.function` — Code smells: <...> — <1-2 sentence description> --- From f79c9a11323ceb601e3d7081358ac793e57db2f0 Mon Sep 17 00:00:00 2001 From: pashov <krum@pashov.com> Date: Sat, 7 Mar 2026 20:18:16 +0200 Subject: [PATCH 4/4] update(solidity-auditor): always run adversarial agent, add mandatory verification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove "deep" mode — adversarial reasoning agent (Agent 5) now runs on every scan - Add mandatory verification step requiring exact function signature quoting before promoting findings - Update README to reflect simplified usage (no more /solidity-auditor deep) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --- solidity-auditor/README.md | 3 --- solidity-auditor/SKILL.md | 11 +++++------ .../hacking-agents/adversarial-reasoning-agent.md | 14 +++++++++++--- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/solidity-auditor/README.md b/solidity-auditor/README.md index 4474e87..0cbec63 100644 --- a/solidity-auditor/README.md +++ b/solidity-auditor/README.md @@ -22,9 +22,6 @@ _Portrayed below: finding multiple high-confidence vulnerabilities in a codebase # Scan the full repo (default) /solidity-auditor -# Full repo + adversarial reasoning agent (slower, more thorough) -/solidity-auditor deep - # Review specific file(s) /solidity-auditor src/Vault.sol /solidity-auditor src/Vault.sol src/Router.sol diff --git a/solidity-auditor/SKILL.md b/solidity-auditor/SKILL.md index f22d14c..90b2c0a 100644 --- a/solidity-auditor/SKILL.md +++ b/solidity-auditor/SKILL.md @@ -1,6 +1,6 @@ --- name: solidity-auditor -description: Security audit of Solidity code while you develop. Trigger on "audit", "check this contract", "review for security". Modes - default (full repo), DEEP (+ adversarial reasoning), or a specific filename. +description: Security audit of Solidity code while you develop. Trigger on "audit", "check this contract", "review for security". Modes - default (full repo) or a specific filename. --- # Smart Contract Security Audit @@ -12,7 +12,6 @@ You are the orchestrator of a parallelized smart contract security audit. Your j **Exclude pattern** (applies to all modes): skip directories `interfaces/`, `lib/`, `mocks/`, `test/` and files matching `*.t.sol`, `*Test*.sol` or `*Mock*.sol`. - **Default** (no arguments): scan all `.sol` files using the exclude pattern. Use Bash `find` (not Glob) to discover files. -- **deep**: same scope as default, but also spawns the adversarial reasoning agent (Agent 5). Use for thorough reviews. Slower and more costly. - **`$filename ...`**: scan the specified file(s) only. **Flags:** @@ -31,18 +30,18 @@ Then continue normally. If the fetch fails (offline, timeout), skip silently. **Turn 1 — Discover.** Print the banner, then in the same message make parallel tool calls: (a) Bash `find` for in-scope `.sol` files per mode selection, (b) Glob for `**/references/attack-vectors/attack-vectors-1.md` and extract the `references/` directory path (two levels up), (c) ToolSearch `select:Agent` to pre-load the Agent tool for Turn 3. Use this resolved path as `{resolved_path}` for all subsequent references. -**Turn 2 — Prepare.** In a single message, make parallel tool calls: (a) Read `{resolved_path}/report-formatting.md`, (b) Bash: create per-agent bundle files in a **single command**. Always create `/tmp/audit-agent-{1,2,3,4}-bundle.md` — each concatenates **all** in-scope `.sol` files (with `### path` headers and fenced code blocks), then `{resolved_path}/attack-vectors/attack-vectors-N.md`, then `{resolved_path}/hacking-agents/vector-scan-agent.md`. In **DEEP** mode, also create `/tmp/audit-agent-5-bundle.md` — concatenates all in-scope `.sol` files (same format), then `{resolved_path}/hacking-agents/adversarial-reasoning-agent.md`. Also create `/tmp/audit-fp-gate-bundle.md` — concatenates all in-scope `.sol` files (same format), then `{resolved_path}/judging.md`, then `{resolved_path}/report-formatting.md`, then `{resolved_path}/operations-agents/fp-gate-agent.md`. Print line counts for every bundle created. Every agent receives the full codebase — only the trailing reference file differs. Do NOT read or inline any file content into agent prompts — the bundle files replace that entirely. +**Turn 2 — Prepare.** In a single message, make parallel tool calls: (a) Read `{resolved_path}/report-formatting.md`, (b) Bash: create per-agent bundle files in a **single command**. Always create `/tmp/audit-agent-{1,2,3,4}-bundle.md` — each concatenates **all** in-scope `.sol` files (with `### path` headers and fenced code blocks), then `{resolved_path}/attack-vectors/attack-vectors-N.md`, then `{resolved_path}/hacking-agents/vector-scan-agent.md`. Also create `/tmp/audit-agent-5-bundle.md` — concatenates all in-scope `.sol` files (same format), then `{resolved_path}/hacking-agents/adversarial-reasoning-agent.md`. Also create `/tmp/audit-fp-gate-bundle.md` — concatenates all in-scope `.sol` files (same format), then `{resolved_path}/judging.md`, then `{resolved_path}/report-formatting.md`, then `{resolved_path}/operations-agents/fp-gate-agent.md`. Print line counts for every bundle created. Every agent receives the full codebase — only the trailing reference file differs. Do NOT read or inline any file content into agent prompts — the bundle files replace that entirely. -**Turn 3 — Spawn.** In a single message, spawn all agents as parallel foreground Agent tool calls (do NOT use `run_in_background`). Always spawn Agents 1–4. Only spawn Agent 5 when the mode is **DEEP**. +**Turn 3 — Spawn.** In a single message, spawn all 5 agents as parallel foreground Agent tool calls (do NOT use `run_in_background`). - **Agents 1–4** (vector scanning) — Do NOT paste agent instructions into the prompt — they are already inside each bundle. Prompt: `Your bundle file is /tmp/audit-agent-N-bundle.md (XXXX lines).` (substitute the real agent number and line count). -- **Agent 5** (adversarial reasoning, DEEP only) — Do NOT paste agent instructions into the prompt — they are already inside the bundle. Prompt: `Your bundle file is /tmp/audit-agent-5-bundle.md (XXXX lines).` (substitute the real line count). +- **Agent 5** (adversarial reasoning) — Do NOT paste agent instructions into the prompt — they are already inside the bundle. Prompt: `Your bundle file is /tmp/audit-agent-5-bundle.md (XXXX lines).` (substitute the real line count). **Turn 4 — Report.** Process agent findings in this strict order: 1. **Pre-filter.** Scan all raw findings and immediately drop any that are informational-only (error messages, naming, gas, NatSpec, admin-only parameter setting, missing events, centralization without concrete exploit path). One word per drop — no analysis. 2. **Deduplicate.** Group surviving findings by root cause (same contract + same function + same bug class). Keep only the most detailed version of each group, drop the rest. List groups: `"Chainlink staleness: Agent 2, Agent 3, Agent 4 → keep Agent 3"`. -3. **Validate.** Spawn a single foreground Agent. Do NOT paste agent instructions into the prompt — they are already inside the bundle. Paste all deduplicated findings and leads verbatim into the prompt, then add: `Validation bundle: /tmp/audit-fp-gate-bundle.md (XXXX lines). Mode: <mode>. Files reviewed: <file list>.` Substitute the real line count, mode, and file list. +3. **Validate.** Spawn a single foreground Agent. Do NOT paste agent instructions into the prompt — they are already inside the bundle. Paste all deduplicated findings and leads verbatim into the prompt, then add: `Validation bundle: /tmp/audit-fp-gate-bundle.md (XXXX lines). Files reviewed: <file list>.` Substitute the real line count and file list. 4. **Output.** Print the validation agent's formatted report verbatim. If `--file-output` is set, also write it to a file (path per report-formatting.md) and print the path. ## Banner diff --git a/solidity-auditor/references/hacking-agents/adversarial-reasoning-agent.md b/solidity-auditor/references/hacking-agents/adversarial-reasoning-agent.md index 2df5edc..caf43fa 100644 --- a/solidity-auditor/references/hacking-agents/adversarial-reasoning-agent.md +++ b/solidity-auditor/references/hacking-agents/adversarial-reasoning-agent.md @@ -35,13 +35,21 @@ You communicate results back ONLY through your final text response. Do not outpu 6. **Pass 5 — Steal-the-funds challenge.** For each contract that holds or controls value, ask: "If I wanted to drain this contract, what is my best path?" Try at least two distinct approaches per value-holding contract. If both fail, explain in one line why. If either succeeds, it is a finding. -7. For each confirmed finding, output in this exact format — nothing more: +7. **Mandatory verification.** Before promoting any candidate to a finding, you MUST quote the exact function signature (including all modifiers like `nonReentrant`, `onlyOwner`, `whenNotPaused`) from your bundle. Then verify: + - Every modifier and access-control check on the function. + - The actual storage variable or mapping being read/written (not what you assume — what the code says). + - Whether the guard you claim is missing actually appears elsewhere in the call chain. + + If you cannot find the exact line in your bundle, the finding is unverified — demote to LEAD or drop. **A single incorrect claim about the presence or absence of a modifier invalidates the entire finding.** + +8. For each confirmed finding, output in this exact format — nothing more: ``` FINDING | location: Contract.function + signature: function foo(...) external nonReentrant onlyOwner ← exact from code path: caller → function → state change → impact description: <one sentence> fix: <one-sentence suggestion or short diff> ``` -8. Do not output findings during analysis — compile them all and return them together as your final response. -9. If you find NO findings, respond with "No findings." +9. Do not output findings during analysis — compile them all and return them together as your final response. +10. If you find NO findings, respond with "No findings."