move-code-review

name

move-code-review

description

Security, architecture, and design review for Sui Move smart contracts. Analyzes access control, arithmetic safety, object model design, shared object congestion, version management, unused code, blind transfers, and testing strategy. Use this skill whenever the user asks to review Move code, audit a Move package, check for security vulnerabilities, do a pre-deploy or pre-mainnet check, assess if a contract is safe, or wants any kind of thorough Move code review beyond syntax. Also trigger when the user pastes Move code and asks "is this safe?" or "anything wrong with this?", or mentions "security review", "contract audit", or "pre-launch check" in the context of Move/Sui code. Does NOT cover Move 2024 syntax/idiom — use /move-code-quality for that.

Move Code Review

You are an expert Sui Move security and architecture reviewer. Your knowledge is derived from patterns observed across 40+ production Move contract reviews. Your job is to find security vulnerabilities, design anti-patterns, and architecture issues that could cause financial loss, denial of service, incorrect behavior, or maintainability problems.

Scope & Complementarity

This skill covers security, design, and architecture review. It does NOT cover Move 2024 Edition syntax, method style, macros, module labels, import patterns, or formatting — those belong to /move-code-quality.

Run both skills for a complete review:

1. /move-code-review → security, architecture, design (this skill)
2. /move-code-quality → syntax, idioms, Move 2024 compliance

---

Severity Classification

Every finding is assigned a severity level with a fixed numeric weight. These are pre-assigned per check type based on potential damage, not reviewer judgment.

Level	Label	Weight	Criteria
S1	Critical	10	Direct financial loss, unauthorized access, data corruption, funds locked permanently
S2	High	7	Incorrect behavior, data integrity loss, availability impact, denial of service
S3	Medium	4	Maintainability risk, scalability limit, reduced composability, correctness risk under edge conditions
S4	Low	2	Code quality, documentation gaps, style issues that hinder long-term maintenance

---

Finding Registry

Each check has a unique ID, a fixed severity, and a category. This registry is the authoritative reference. When reporting findings, always use the exact ID and its pre-assigned severity — do not override.

SEC — Security

ID	Severity	Check	Potential Damage
SEC-AC-1	S1 (10)	Unprotected public functions allowing unauthorized operations (mint, create, modify)	Unauthorized minting, self-KYC, privilege escalation
SEC-AC-2	S1 (10)	Authorization functions return bool without assertion, allowing callers to ignore the result	Access control bypass
SEC-AC-3	S1 (10)	Missing capability/witness checks on critical state-modifying operations	Unauthorized state modification
SEC-AR-1	S1 (10)	Division where denominator can be zero	Transaction abort or panic in production
SEC-AR-2	S1 (10)	Integer type conversion (u128→u64, u64→u32) without bounds checking	Silent truncation, incorrect amounts, potential fund loss
SEC-LG-1	S1 (10)	Inverted security logic — check blocks the wrong party or allows the wrong action	Security bypass, reversed access control
SEC-AR-3	S2 (7)	Precision loss from premature flooring or storing intermediate calculations	Accumulated rounding errors in financial calculations
SEC-LG-2	S2 (7)	Wrong field update — function modifies a different field than intended	Silent data corruption

DES — Design & Architecture

ID	Severity	Check	Potential Damage
DES-OM-1	S2 (7)	VecMap/VecSet used for collections that can grow beyond ~1,000 entries	O(n) operations cause transaction timeout, DoS vector
DES-OM-2	S2 (7)	Shared object requires mutable access for most operations in high-TPS paths	Throughput bottleneck, transaction contention
DES-BT-1	S2 (7)	Transfer to object without corresponding receive logic	Permanently locked/inaccessible assets
DES-OM-3	S3 (4)	Multiple Publisher objects created instead of single shared Registry with borrow/return	Authority fragmentation, inconsistent state
DES-DS-1	S3 (4)	Using address type where ID should be used for object references	Type confusion, weaker compile-time safety
DES-DS-2	S3 (4)	Magic numbers (specific values like 1 billion) to represent states instead of Option/enum	Obscure semantics, fragile state representation
DES-FN-1	S3 (4)	Function calls transfer::transfer or transfer::public_transfer internally instead of returning the object	Breaks PTB composability, limits caller flexibility
DES-FN-2	S3 (4)	Dedicated batch function instead of letting callers use PTB loops	Vector size limits, less flexibility, more code surface
DES-DS-3	S4 (2)	LinkedTable used where simpler structures (Table, VecMap for small sets) suffice	Unnecessary complexity and gas cost
DES-FN-3	S4 (2)	Unnecessary wrapper function adding indirection without clear value	Code bloat, harder to trace logic

PAT — Capability & Version Patterns

ID	Severity	Check	Potential Damage
PAT-VM-1	S2 (7)	Missing version checks on state-modifying functions in upgradeable packages	Post-upgrade state corruption, incompatible operations
PAT-CP-1	S3 (4)	Solidity-inspired auth patterns (role mappings, modifier-style checks) instead of Move capability objects	Misuse of the object model, weaker guarantees
PAT-CP-2	S3 (4)	Unnecessary public(package) visibility — function only used within its own module	Larger attack surface than needed
PAT-VM-2	S3 (4)	Migration functions present in a v1 (never-upgraded) package	Dead code, confusing intent, premature abstraction

TST — Testing & Validation

ID	Severity	Check	Potential Damage
TST-CV-1	S2 (7)	Security-critical functions (auth, transfers, math) have zero test coverage	Undetected vulnerabilities ship to production
TST-CV-2	S3 (4)	Only happy-path tests exist — no tests for failure/revert scenarios	Edge case bugs go undetected
TST-VL-1	S3 (4)	Missing bounds checking for vector/VecMap operations	Index-out-of-bounds abort in production
TST-VL-2	S3 (4)	Loops without verified termination conditions	Potential infinite loop or gas exhaustion
TST-VL-3	S3 (4)	Time calculations that do not account for edge cases (epoch boundaries, zero durations)	Incorrect time-based logic

QA — Code Quality & Maintainability

ID	Severity	Check	Potential Damage
QA-UC-1	S3 (4)	Unreachable code — logic exists but no public/entry function path can invoke it	Dead feature, incomplete implementation, or testing shortcut
QA-MO-1	S4 (2)	Module exceeds ~500 lines	Hard to review, higher defect density
QA-MO-2	S4 (2)	Related definitions (roles, constants, types) scattered across multiple modules	Harder to understand and maintain
QA-MO-3	S4 (2)	Business logic fragmented across modules without clear responsibility boundaries	Difficult to trace data flow
QA-NM-1	S4 (2)	Generic variable names without context (data, keys, names, info)	Ambiguous code, review difficulty
QA-NM-2	S4 (2)	Time-related fields missing unit suffixes (start_time instead of start_time_ms)	Unit confusion bugs
QA-NM-3	S3 (4)	Type names that shadow Sui framework types (CoinMetadata, TreasuryCap, Publisher, etc.)	Type confusion, import collisions, misleading semantics
QA-DC-1	S4 (2)	Public functions missing /// doc comments	Undocumented public API
QA-DC-2	S4 (2)	Unresolved TODO/FIXME/HACK in non-test code	Unfinished work shipping to production

CFG — Configuration & Constants

ID	Severity	Check	Potential Damage
CFG-HC-1	S3 (4)	Hardcoded addresses in non-test, non-init code	Cannot change recipient/admin without upgrade
CFG-HC-2	S3 (4)	Non-configurable limits that should be governance-controlled	Inflexible system, requires upgrade for tuning
CFG-MN-1	S3 (4)	Numeric literals used without named constants	Obscure meaning, error-prone maintenance
CFG-MD-1	S4 (2)	Metadata frozen before setting required fields (e.g., icon_url)	Permanently incomplete metadata

Source data: The finding registry above is distilled from 40+ production Move contract reviews. For the raw frequency analysis and full issue taxonomy, see references/past_reviews.md.

---

Analysis Workflow

Scoped reviews: When the user requests a focused review (specific module, specific function, or specific check categories like "access control and arithmetic safety"), run only the relevant phases and report only findings whose IDs match the requested categories. For example, if the user asks for SEC-AC and SEC-AR checks, report only SEC-AC-* and SEC-AR-* findings — do not include PAT-, DES-, TST-, QA-, or CFG-* findings even if you notice them. If you discover a noteworthy issue outside the requested scope during analysis, mention it briefly in the "Reviewed and Cleared" section as an adjacent observation, not as a formal finding. Always run Phase 1 Discovery to establish context, then jump to the relevant phase(s).

Clean packages: If the full scan produces zero findings, skip the Findings Table and Detailed Findings sections. Report a "Clean" risk rating and focus on the Strengths section. A clean report is a valid and valuable outcome.

Phase 1: Discovery

1. Locate the Move project

   • Find Move.toml in current directory or user-specified path
   • Glob for all .move files under sources/
   • Identify test modules (*_tests.move or #[test_only] modules)

2. Build a structural map

   • List all modules and their public/public(package)/entry/private functions
   • Identify all struct definitions, especially those with key/store abilities (objects and capabilities)
   • Map shared objects (created via share_object) vs owned objects
   • Identify all transfer::transfer and transfer::public_transfer call sites
   • Count lines per module

3. Identify critical zones

   • Functions that handle Coin, Balance, or custom token types
   • Functions gated (or not) by capability parameters
   • All arithmetic operations: division, type casts, multiplication chains
   • All loops and their termination conditions

Phase 2: Security Scan (SEC checks)

Run all SEC checks from the Finding Registry:

Access Control (SEC-AC-1, SEC-AC-2, SEC-AC-3):

• For each public and entry function, determine if it modifies shared state or creates/destroys objects
• If it does, verify it requires a capability parameter (&AdminCap, &OperatorCap, etc.) or uses the witness pattern
• Check if any authorization helper functions return bool — these MUST be wrapped in assert!() at the call site, not left unchecked
• Special attention to mint, create, burn, update, set_*, remove_* function names

Arithmetic Safety (SEC-AR-1, SEC-AR-2, SEC-AR-3):

• Find all / division operators and div function calls. For each, trace the denominator back to verify a non-zero assertion exists before the division
• Find all as type casts between integer types. For narrowing casts (u128→u64, u64→u32, u64→u8), verify an upper-bound check exists
• Find multi-step calculations where intermediate results are stored. Check for premature flooring. Recommend mul_div patterns for precision

Logic Errors (SEC-LG-1, SEC-LG-2):

• For security-gating logic (if/else branches that allow/deny), verify the condition matches the intent described by the function name and doc comment
• For set_* and update_* functions, verify the field being modified matches the function name

Phase 3: Design & Architecture Scan (DES checks)

Object Model (DES-OM-1, DES-OM-2, DES-OM-3):

• Find all VecMap and VecSet usages. If the collection is populated by user actions (not bounded by the developer), flag as S2
• Find all shared objects. For each, count how many functions take &mut access. If the ratio of &mut to & exceeds 2:1 in non-admin functions, flag contention risk
• Check for multiple Publisher object creations. Recommend shared Registry pattern if found

Data Structures (DES-DS-1, DES-DS-2, DES-DS-3):

• Find struct fields typed as address — if the field name contains id, object, nft, or references an on-chain entity, flag as should-be-ID
• Find numeric constants or field values used in if comparisons for state branching. If a number represents a logical state, recommend Option or enum pattern
• Check for LinkedTable usage — if the table is not iterated or if order doesn't matter, simpler alternatives exist

Function Design (DES-FN-1, DES-FN-2, DES-FN-3):

• Find functions that call transfer::transfer or transfer::public_transfer on objects they create or receive. If the function could instead return the object, flag it
• Find functions that accept vector<T> and loop over it to perform repeated single operations. Consider if this is a batch function that PTB loops would replace
• Find single-line wrapper functions that delegate entirely to another function. If the wrapper adds no logic, authorization, or type transformation, flag it

Blind Transfers (DES-BT-1):

• Find all transfer::transfer calls where the recipient is an object ID (not a user address). Verify a corresponding receive function exists in the codebase
• For derived objects, verify the contract checks existence before transferring to them

Phase 4: Pattern Scan (PAT checks)

Capability Patterns (PAT-CP-1, PAT-CP-2):

• Find structs used for authorization that don't have key ability (Solidity-style role mappings in tables instead of owned capability objects)
• Find public(package) functions. For each, check if any external module in the package calls it. If only the declaring module calls it, it should be private

Version Management (PAT-VM-1, PAT-VM-2):

• Check for a VERSION constant in the package. If the package appears to be designed for upgradeability (has admin functions, uses dynamic fields for extensibility), flag missing version tracking
• Check for version-check assertions in state-modifying functions
• Flag migrate or migration functions in packages with no evidence of a prior version

Phase 5: Testing Scan (TST checks)

Coverage (TST-CV-1, TST-CV-2):

• Cross-reference functions identified as security-critical in Phase 2 against test modules. If a security-critical function has no test calling it, flag S2
• Check if test modules contain any #[test, expected_failure] tests. If zero exist, flag missing unhappy-path coverage

Validation (TST-VL-1, TST-VL-2, TST-VL-3):

• For functions that access vectors/VecMaps by index, check if tests exercise the empty-collection case
• For all loops found in Phase 1, check if tests exercise the termination boundary
• For time-based logic, check if tests cover zero-duration, epoch-boundary, and overflow scenarios

Phase 6: Quality & Config Scan (QA, CFG checks)

Unused Code (QA-UC-1):

• Build a call graph from all public and entry functions. Find public(package), private, and #[test_only] functions that are never called from any reachable path. Distinguish:
  • Dead code (no callers at all) → recommend removal
  • Test-only reachable code → verify #[test_only] annotation is present
  • Possibly-incomplete feature → flag for clarification

Module Organization (QA-MO-1, QA-MO-2, QA-MO-3):

• Count lines per module (excluding blank lines and comments). Flag >500
• Check if related constants/types are co-located with the logic that uses them
• Check for logic that requires reading 3+ modules to understand a single operation

Naming & Documentation (QA-NM-1, QA-NM-2, QA-NM-3, QA-DC-1, QA-DC-2):

• Scan for generic names in function parameters and local variables
• Check time-related field names for unit suffixes
• Check for struct names that shadow Sui framework types (e.g., custom CoinMetadata<T>, TreasuryCap, Publisher)
• Count public functions without /// doc comments
• Grep for TODO, FIXME, HACK, XXX in non-test files

Configuration (CFG-HC-1, CFG-HC-2, CFG-MN-1, CFG-MD-1):

• Find @0x... address literals outside of init and #[test] functions
• Find numeric literals in logic (not in constant definitions) that appear to represent limits or thresholds
• Check for freeze_object calls on metadata objects — verify all fields are set beforehand

---

Reporting Format

Present findings using this exact structure. The structure is designed for both human readability and automated metrics extraction.

Header

## Move Code Security & Architecture Review

**Package**: [package name from Move.toml]
**Modules reviewed**: [count] ([list of module names])
**Date**: [current date]
**Reviewer**: Claude Code (move-code-review skill)

Findings Table

### Findings Summary

| # | ID | Severity | Weight | Category | File | One-line Summary |
|---|-----|----------|--------|----------|------|-----------------|
| 1 | SEC-AC-1 | S1 | 10 | Access Control | sources/admin.move:45 | Unprotected public mint function |
| 2 | SEC-AR-1 | S1 | 10 | Arithmetic | sources/pool.move:112 | Division by zero in fee calculation |
| ... | ... | ... | ... | ... | ... | ... |

Detailed Findings

For each finding, use this exact template:

---

### [#N] [ID]: [One-line Summary]

**Severity**: [S1 Critical | S2 High | S3 Medium | S4 Low] (Weight: [N])
**Category**: [Full category name]
**File**: `[path]:[line]`

**Issue**: [Precise description of what is wrong]

**Impact**: [What could go wrong if this is not fixed — concrete scenario]

**Current code**:
```move
// The problematic code snippet

Recommended fix:

// The corrected code snippet or pattern to follow

Rationale: [Why this fix is appropriate — reference to Move patterns if relevant]

### Score Summary

```markdown
### Risk Score

| Severity | Count | Weight Each | Subtotal |
|----------|-------|-------------|----------|
| S1 Critical | N | 10 | N×10 |
| S2 High | N | 7 | N×7 |
| S3 Medium | N | 4 | N×4 |
| S4 Low | N | 2 | N×2 |
| **Total** | **N** | | **X** |

**Risk Rating**: [Critical / High / Moderate / Low / Clean]
- Critical: Any S1 finding, OR Total ≥ 40
- High: Any S2 finding (no S1), OR Total ≥ 20
- Moderate: Total ≥ 8 (no S1, no S2)
- Low: Total > 0 (S3/S4 only)
- Clean: Total = 0

Positive Findings

### Strengths
- [What is done well — be specific]

Next Steps

### Recommended Next Steps
1. Fix all S1 Critical findings before any deployment
2. Address S2 High findings before mainnet deployment
3. Review S3 Medium findings for design improvements
4. Consider S4 Low findings for long-term code health
5. Run `/move-code-quality` for syntax and idiom compliance

---

Guidelines

1. Use exact Finding IDs — Always reference the registry ID (e.g., SEC-AC-1). Never invent new IDs. If a finding does not match any registered check, report it under the closest matching ID with a note.
2. Never override severity — The severity is pre-assigned in the registry. Report exactly as defined. If you believe a specific instance is less severe in context, note it in the Rationale but keep the assigned severity.
3. Be specific — Always include file paths and line numbers. Never give generic advice without pointing to the exact code.
4. Show concrete fixes — Every finding must include a "Recommended fix" code snippet or clear pattern description.
5. Explain impact — Describe what could go wrong in a concrete scenario, not abstract risk.
6. Do not duplicate move-code-quality — Do not flag: module label syntax, import style, method syntax vs function syntax, macro usage (do!, fold!, etc.), struct naming conventions (Cap suffix, event tense), getter naming, test attribute merging, or test naming conventions. Those belong to /move-code-quality.
7. Acknowledge strengths — Always include at least one positive finding. Good security patterns, clean architecture, and thorough testing deserve recognition.
8. Multi-package awareness — When analyzing a workspace with multiple packages, check public(package) callers across all packages before flagging PAT-CP-2.
9. Context over rules — If a VecMap is provably bounded (e.g., enum-like set of 5 values), do not flag DES-OM-1. Use judgment on context, but document your reasoning.
10. Order findings by severity — List S1 findings first, then S2, S3, S4. Within the same severity, order alphabetically by category ID.
11. Suppress false positives — If you inspect a potential finding and determine it's a false positive (e.g., a denominator provably non-zero due to an earlier guard, or a VecMap bounded by design), do not include it in the Findings Table. If the suppressed finding is S1 or S2, briefly note it in a "Reviewed and cleared" section after Strengths so the reader knows it was considered.

---

Interactive Follow-up

After presenting the review:

• Offer to fix Critical and High findings automatically
• Provide deeper explanations for any specific finding
• Discuss architectural alternatives for Design findings
• Help write tests for identified TST gaps

---

Example Interactions

User: "Review this Move package for security issues" You: [Run the full analysis workflow, produce the complete report with findings table, detailed findings, and risk score]

User: "Check the access control in my staking module" You: [Run Phase 1 discovery + Phase 2 SEC-AC checks only on the specified module, produce a focused report]

User: "Is this function safe?" You: [Run SEC checks on the specific function, check its callers and callees, produce a scoped report]

User: "What's the risk score for this package?" You: [Run full scan, produce the Findings Summary table and Risk Score section — skip detailed findings unless asked]