Building Secure Contracts Skill

Overview

This skill applies systematic security analysis to smart contracts and secure API contracts. The core principle: every state mutation must be proven safe through invariant verification before an external call executes. It covers both EVM (Solidity) and Solana (Rust) ecosystems with platform-specific vulnerability patterns.

Vulnerability taxonomy: OpenSCV (94 classified security issues) Critical patterns: CEI, reentrancy guards, access modifiers, SafeMath equivalents Risk landscape: $1.8B+ in DeFi exploits Q3 2025 (access control: $953M, reentrancy: $420M)

When to Use

Before deploying any smart contract to mainnet
When auditing existing contracts for security vulnerabilities
When reviewing API contracts for invariant violations
When adding new entry points or external calls to existing contracts
When upgrading proxy contracts (storage slot collision risk)
When integrating oracles, flash loans, or third-party DeFi protocols

Iron Laws

NEVER make external calls before updating state — Checks-Effects-Interactions (CEI) is non-negotiable; any external call before state update is a reentrancy vector regardless of perceived safety.
NEVER assume access control is correct without reading every modifier — access control failures account for ~53% of 2024 DeFi losses; verify every onlyOwner, onlyRole, and custom guard.
NEVER trust integer arithmetic without explicit bounds checking — Solidity 0.8+ has native overflow protection but custom assembly, unchecked blocks, and Rust/Solana code require explicit verification.
ALWAYS enumerate all contract invariants before analysis — invariants are the ground truth for correctness; a violation is always a bug; document them in NatSpec before reviewing the implementation.
ALWAYS test reentrancy across full call chains, not just single functions — cross-function reentrancy (withdraw + transfer sharing state) is as dangerous as direct reentrancy.

Phase 1: Contract Reconnaissance

Goal: Map the attack surface before deep analysis.

Steps

Enumerate entry points: All external/public functions, fallback, receive
Identify state-mutating functions: Functions that modify storage
Map access control boundaries: Roles, modifiers, ownership checks
Catalog external calls: call(), transfer(), ERC20 hooks, interface calls
Identify trust boundaries: User input, oracle feeds, cross-contract calls

Output Format

## Contract Reconnaissance

### Entry Points

- [ ] `withdraw(uint256 amount)` — external, state-mutating, calls msg.sender
- [ ] `deposit()` — payable, updates balances mapping

### Access Control Map

- [ ] `onlyOwner`: [list of functions]
- [ ] `onlyRole(ADMIN_ROLE)`: [list of functions]
- [ ] No modifier (verify intent): [list of functions]

### External Calls

- [ ] `msg.sender.call{value: amount}("")` at withdraw():L45
- [ ] `token.transferFrom(...)` at deposit():L23

### Trust Boundaries

- [ ] User-supplied amount at withdraw():L40
- [ ] Oracle price feed at getPrice():L67 — manipulation risk

Phase 2: Reentrancy Analysis

Goal: Identify all reentrancy vectors (direct, cross-function, read-only).

Checks-Effects-Interactions Verification

For each function with external calls:

### Function: withdraw(uint256 amount)

#### CEI Order Analysis

- L40: CHECK — require(balances[msg.sender] >= amount) ✓
- L45: EXTERNAL CALL — msg.sender.call{value: amount}("") ← VIOLATION
- L48: EFFECT — balances[msg.sender] -= amount ← STATE AFTER CALL

**FINDING**: Classic reentrancy — balance updated after external call.
**Fix**: Move L48 before L45 (CEI pattern)
**Severity**: Critical

#### Fixed Pattern

```solidity
require(balances[msg.sender] >= amount);
balances[msg.sender] -= amount;  // Effect BEFORE external call
(bool success, ) = msg.sender.call{value: amount}("");
require(success);
```

Cross-Function Reentrancy Check

Identify shared state between functions that both make external calls:

### Shared State: balances mapping

- withdraw() reads + writes balances + makes external call
- emergencyWithdraw() reads + writes balances + makes external call
  **RISK**: Reentrancy from withdraw() into emergencyWithdraw() bypasses checks

Phase 3: Access Control Audit

Goal: Verify every state-mutating function has appropriate guards.

Access Control Checklist

For each function:

### Function Audit: updateTreasury(address newTreasury)

- [ ] Has access modifier? → NO ← FINDING: Missing onlyOwner
- [ ] Modifier verified in contract? → N/A (not present)
- [ ] Owner transferable safely? → N/A
- [ ] Time lock for critical changes? → NO

**Severity**: Critical — anyone can redirect protocol treasury
**Fix**: Add `onlyOwner` modifier and time-lock for parameter changes

Role Confusion Patterns

### Role Check: PAUSER_ROLE vs ADMIN_ROLE

- pause() requires: PAUSER_ROLE
- unpause() requires: PAUSER_ROLE (RISK: pauser can also unpause)
- grantRole() requires: ADMIN_ROLE

**Issue**: Pauser can unilaterally pause and unpause — should require separate roles
**Severity**: Medium

Phase 4: Integer Arithmetic Analysis

Goal: Identify overflow, underflow, precision loss, and rounding direction bugs.

Arithmetic Boundary Analysis

### Function: calculateReward(uint256 principal, uint256 rate)

- L88: `uint256 reward = principal * rate / 1e18`
  - Multiplication before division: OK (avoids precision loss)
  - Overflow check: principal \* rate could overflow if both > sqrt(uint256.max)
  - Rounding: truncates toward zero — check if favors protocol or user
  - `unchecked` block? → NO → Solidity 0.8+ protects this

### Unchecked Block Analysis

- L102-108: `unchecked { ... }`
  - Why unchecked? Check comment and verify mathematician's claim
  - Is the claimed impossibility of overflow actually proven?
  - [UNVERIFIED] claim: "amount < balance guarantees no underflow"

Phase 5: Invariant Verification

Goal: Identify and verify all contract-level invariants.

### Contract Invariants: LiquidityPool

1. **Solvency**: sum(balances) == address(this).balance — [VERIFIED L90]
2. **Total supply**: totalSupply == sum(all user shares) — [UNVERIFIED]
3. **Fee bound**: fee <= MAX_FEE (1000 bps) — [VERIFIED by require at L45]
4. **Non-zero denominator**: totalSupply > 0 before share calculation — [VIOLATED at L67, division-by-zero risk on first deposit]

### Invariant Violation Findings

**FINDING**: Invariant 4 violated — first depositor can cause division by zero

- Location: L67 `shares = amount * totalSupply / totalAssets`
- When: totalSupply == 0 on first deposit
- Impact: DoS attack on first deposit; protocol initialization blocked
- Fix: Handle zero totalSupply case separately with initial share ratio

Output: Security Report

# Security Report: [Contract Name]

## Summary

- Functions analyzed: N
- Findings: N (Critical: X, High: Y, Medium: Z, Low: W)
- Invariants verified: N of M
- CEI violations: N

## Critical Findings

### [F-01] Reentrancy in withdraw()

- Location: `src/Pool.sol:L45`
- Pattern: External call before state update (CEI violation)
- Impact: Complete fund drainage
- Fix: Apply CEI pattern — update state before external call
- 5 Whys: [root cause chain]

## Invariant Status

| Invariant                  | Status     | Evidence            |
| -------------------------- | ---------- | ------------------- |
| sum(balances) == balance   | VERIFIED   | L90 invariant check |
| totalSupply == sum(shares) | UNVERIFIED | No test coverage    |

## Recommendations

1. [Critical] Fix reentrancy in withdraw() before deployment
2. [High] Add reentrancy guard as defense-in-depth
3. [Medium] Add formal invariant tests via Foundry invariant suite

Integration with Agent-Studio

Recommended Workflow

Invoke audit-context-building for initial code reconnaissance
Invoke building-secure-contracts for contract-specific analysis
Feed findings into security-architect for threat modeling
Use static-analysis (Semgrep/CodeQL) for automated confirmation
Use medusa-security for fuzzing-based invariant testing

Complementary Skills

Skill	Relationship
`audit-context-building`	Builds initial mental model before contract analysis
`security-architect`	Consumes findings for threat modeling and STRIDE
`static-analysis`	Automated SAST confirmation of manual findings
`medusa-security`	Fuzzing and property-based testing for invariants
`variant-analysis`	Finds similar vulnerability patterns across codebase
`web3-expert`	Solidity/Ethereum ecosystem expertise

Anti-Patterns

Anti-Pattern	Why It Fails	Correct Approach
Auditing only the happy path	Reentrancy and access control bugs are invisible in happy path	Explicitly trace every error path and external call
Trusting function name for access control	`onlyAdmin()` might not check the actual admin role	Read the modifier implementation, not just its name
Assuming Solidity 0.8 prevents all integer bugs	`unchecked` blocks, assembly, and casting bypass protection	Audit all `unchecked` blocks and type casts explicitly
Skipping cross-function reentrancy	Cross-function reentrancy bypasses single-function guards	Map shared state across ALL functions making external calls
Leaving invariants implicit	Unwritten invariants are unverified risks	Document every invariant in NatSpec before analysis

Memory Protocol

Before starting: Check .claude/context/memory/learnings.md for prior contract audits of the same protocol or token standard.

During analysis: Write incremental findings to context report as discovered. Do not wait until the end.

After completion: Record key findings and patterns to .claude/context/memory/learnings.md. Record architecture decisions (CEI enforcement patterns, invariant frameworks) to decisions.md.

building-secure-contracts