Security review gate for completed Solidity contract implementations. Use after any contract implementation is complete and before marking work done, merging, or deploying. Triggers on: "review this contract", "security review", "is this secure?", "check for vulnerabilities", "audit this", "pen test this", or when any contract implementation is complete. Dispatches the reviewoor agent with full context: source code, interface, design doc, invariant list, and git diff if modifying an existing contract. All Critical and High severity findings must be resolved with regression tests before exiting this skill.
After completing any contract implementation, before marking work as done. Also when the user
says "review this", "is this secure?", "check for vulnerabilities", "security review", or
"audit this contract." Dispatches the reviewoor agent.
NO CONTRACT IS COMPLETE WITHOUT A REVIEWOOR SECURITY REVIEW WITH ALL CRITICAL AND HIGH FINDINGS RESOLVED
This skill dispatches the reviewoor agent with the necessary context (see below). After the
review, all Critical and High findings must be:
Medium findings must be documented in the audit prep findings log. Low/Informational must be logged. Nothing is ignored.
Before dispatching the reviewoor agent, prepare all of the following:
src/interfaces/I<ContractName>.sol)docs/designs/slither . --filter-paths "test,script,lib"If any of these are missing, generate them before dispatching. A review without context is a superficial scan, not a security review.
After the review:
docs/audit/findings-log.mddocs/reviews/YYYY-MM-DD-<ContractName>-security.mdDispatch reviewoor agent with:
1. Source code: [paste full content of each in-scope .sol file]
2. Interface: [paste src/interfaces/I<ContractName>.sol]
3. Design doc: [paste docs/designs/YYYY-MM-DD-<name>-design.md]
4. Invariants: [list all invariants from the design doc]
5. Git diff: [paste output of git diff main HEAD -- src/]
6. Slither: [paste output of slither . --filter-paths "test,script,lib"]
7. Coverage gaps: [list all functions/branches not covered by tests]
See security-checklist.md for full vulnerability descriptions, proof-of-concept exploits,
detection methods, mitigations, and test patterns.
[ ] All state changes occur BEFORE any external calls (CEI pattern)?
→ Check every function: state updates in EFFECTS, external calls in INTERACTIONS
[ ] ReentrancyGuard applied to functions that change state and make external calls?
→ As defense-in-depth even when CEI is followed
[ ] Cross-function reentrancy: can A call external contract which re-enters B?
→ Common: withdraw() calls external; attacker's fallback calls deposit()
→ Fix: shared reentrancy lock across both A and B, OR separate their locked state
[ ] Cross-contract reentrancy: can A call B which re-enters A?
→ Less common but catastrophic. Audit every external call target.
[ ] Read-only reentrancy: can a view function return stale data during reentrancy?
→ Price feeds and oracle views are most vulnerable
[ ] Every privileged function has an explicit access modifier or early revert?
→ Access check must be FIRST in the function (fail fast, save gas)
[ ] Ownable2Step used, not Ownable?
→ Single-step ownership transfer: one typo or wrong address loses ownership forever
→ Ownable2Step requires the new owner to explicitly accept — prevents accidents
[ ] No tx.origin for authorization?
→ tx.origin is the original EOA, not the immediate caller
→ Contracts calling your contract appear as trusted users — phishable
[ ] Role renouncement is explicit and protected?
→ Renouncing the last admin role permanently bricks the contract
→ Add a check: cannot renounce if you're the last holder of a critical role
[ ] Timelock on sensitive privileged operations?
→ setFee(), setOracle(), upgradeTo() should have a delay for user reaction
[ ] Two-factor upgrade authorization for UUPS contracts?
→ _authorizeUpgrade must have non-trivial access control
→ Empty _authorizeUpgrade = anyone can upgrade = total compromise
[ ] No precision loss from (a / b) * c pattern?
→ Division truncates. Always: (a * c) / b (multiply before dividing)
[ ] All unchecked blocks have a comment proving bounds?
→ "Safe: x was already checked < y above" must be present
→ Unchecked without proof is a bug waiting to trigger
[ ] Downcast safety verified?
→ uint256 → uint128: check the value fits before casting
→ Silent truncation: uint256(type(uint128).max + 1) == 0 after uint128 cast
→ Fix: use OpenZeppelin's SafeCast or explicit range checks
[ ] ERC-4626 rounding direction correct?
→ Deposits (previewDeposit, convertToShares): round DOWN (user gets fewer shares)
→ Withdrawals (previewWithdraw, convertToAssets): round UP (vault keeps more)
→ Wrong rounding direction enables dust theft over many transactions
[ ] No share price manipulation through donation?
→ First depositor attack: deposit 1 wei, donate large amount, inflate share price
→ Fix: use virtual shares (e.g., totalSupply + 1e18 offset) or minimum deposit
[ ] Return values of .call(), .delegatecall(), .staticcall() checked?
→ Low-level calls return (bool success, bytes memory data)
→ Ignoring success means failed calls are silently accepted
[ ] SafeERC20 used for all token transfers?
→ USDT transfer() returns void — raw transferFrom reverts on USDT
→ Non-standard tokens silently fail without SafeERC20
[ ] Non-standard ERC-20 behaviors handled?
→ Fee-on-transfer: measure balance delta, not the amount parameter
→ Rebasing: store shares, not amounts
→ Return false instead of revert: SafeERC20 handles this
[ ] User-supplied addresses treated as adversarial?
→ Any external contract can have malicious receive()/fallback()
→ Calls to user-provided addresses can re-enter or fail strategically
[ ] Pull payment pattern over push where possible?
→ Sending ETH in a loop: one recipient's failure blocks everyone else
→ Better: accumulators + claimable withdrawals (pull pattern)
[ ] No spot price from a single DEX source?
→ Uniswap spot price is manipulable within one block via flash loans
→ Use Uniswap TWAP (minimum 1 block, ideally 30+ minutes) or Chainlink
[ ] Chainlink staleness check present?
→ (, int256 price, , uint256 updatedAt, ) = feed.latestRoundData();
→ if (block.timestamp - updatedAt > STALENESS_THRESHOLD) revert StalePrice();
[ ] Chainlink negative/zero price check present?
→ if (price <= 0) revert InvalidPrice(price);
→ Negative prices indicate oracle circuit breaker activation
[ ] L2 sequencer uptime check present (for Arbitrum, Optimism, Base)?
→ On L2, the sequencer can go down; Chainlink still serves last-known price
→ Use sequencerUptimeFeed and add grace period after sequencer restart
[ ] Price decimal normalization correct?
→ Chainlink ETH/USD: 8 decimals. Most tokens: 18 decimals. Check each feed.
[ ] Atomically reversible operations analyzed for flash loan exploits?
→ If an operation can be: borrow → exploit → repay in one tx, it's vulnerable
→ Common targets: governance with current-block snapshot, price-sensitive logic
[ ] Significant privileged operations have a timelock?
→ Front-running oracle updates, governance proposals, fee changes
→ Timelock gives users time to react to adverse changes
[ ] Sandwich attack on price-sensitive operations mitigated?
→ Require minimum output (slippage tolerance) on any swap or price-sensitive tx
→ Or use a private RPC (Flashbots) to avoid the public mempool
[ ] Storage layout diff verified via forge inspect --pretty?
→ No existing slot changed position, removed, or retyped
[ ] _disableInitializers() in implementation constructor?
→ Without this, anyone can initialize the implementation contract directly
→ Initialized implementation can be used as a malicious upgrade target
[ ] initialize() protected by initializer modifier and not re-runnable?
→ Can the initialize function be called twice? (Should revert on second call)
[ ] _authorizeUpgrade has correct access control?
→ Empty override = anyone can upgrade = total protocol compromise
[ ] upgradeToAndCall used (not upgradeTo + separate initialize)?
→ Separation creates a window between upgrade and initialization
[ ] No unbounded loops over user-controlled storage?
→ for (uint i; i < users.length; ++i) where users grows unboundedly = DoS
→ Fix: pagination, or accumulator patterns instead of enumeration
[ ] No operations that can be made to fail by a single bad actor?
→ Push pattern with external calls: one failing recipient blocks all others
→ Fix: pull payment pattern; wrap external calls in try/catch
[ ] No grieving via small/dust deposits?
→ Many small deposits can fill the contract's user list, making loops expensive
→ Fix: minimum deposit thresholds
[ ] Governance proposals cannot be bricked by spam?
→ Proposal spam with low threshold fills the proposal queue
→ Fix: proposal threshold, proposal cooldown per address
[ ] Nonces used for all signature-based authorization?
→ Without nonces, a signed message can be replayed indefinitely
[ ] Domain separator includes chainId?
→ Without chainId, signatures from testnet work on mainnet (replay across chains)
[ ] Deadline parameter on all signed messages?
→ Without deadline, signed permissions are valid forever
[ ] permit() signature validation correct?
→ Incorrect ECDSA recovery produces wrong signer address (no revert, just wrong auth)
| Severity | Definition | Action | Blocker? |
|---|---|---|---|
| Critical | Direct loss of funds, unauthorized access to privileged functions, permanent DoS | Fix immediately. Write regression test. Run reviewoor again on the fix. | Yes — blocks everything |
| High | Indirect loss of funds, significant security degradation, breaks core invariant | Fix before any merge or deployment. Write regression test. | Yes — blocks merge and deploy |
| Medium | Partial loss, economically suboptimal, correct but dangerous pattern | Fix before deployment. Document in audit prep findings log with resolution. | Blocks deploy, not merge |
| Low | Informational, gas inefficiency, code quality, deviation from best practice | Log in audit prep findings doc. Address before external audit. | Not a blocker |
| Informational | Style, documentation, unclear naming | Log. Fix if time permits. | Not a blocker |
# Run Slither static analysis (include full output in reviewoor dispatch)
slither . --filter-paths "test,script,lib"
# Generate test coverage (include in reviewoor dispatch)
forge coverage
# Verify storage layout before/after changes
forge inspect <ContractName> storage-layout --pretty
# Run full test suite — must pass before security review
forge test
# Run with verbosity to see failing test details
forge test -vvv
# Check for console.log in source (must be zero)
grep -rn "console" src/
The reviewoor agent produces a security review report. Save to:
docs/reviews/YYYY-MM-DD-<ContractName>-security.md
Minimum report requirements:
After all Critical and High findings are resolved:
solidity-gas-optimizer (if gas review is pending)solidity-deployer (if ready to deploy)solidity-audit-prep (if preparing for external audit)| Rationalization | Counter |
|---|---|
| "The code is obviously correct" | The reviewoor finds things that "obviously correct" code misses. That's the point. |
| "I already reviewed it myself" | Self-review and agent review are not the same thing. One is implementation thinking; the other is security thinking. Both are required. |
| "The tests pass, so it's secure" | Tests verify known behavior. The reviewoor checks for unknown attack vectors. Passing tests and being secure are independent properties. |
| "It's a small contract" | Small contracts have small but critical attack surfaces. The Parity multisig was small. The reentrancy bug in The DAO was in a small function. |
| "We'll do a real audit later" | Internal review finds the obvious issues. External audit finds the subtle ones. Neither replaces the other, and both require this skill to pass. |