0x-shashi/skills/fix-review

Fix Review Skill

Purpose

Verify that bug fixes correctly address reported vulnerabilities without introducing new issues. A fix review is not a rubber stamp — it requires the same rigor as the original audit, focused on the change boundary and its ripple effects.

When to Trigger

• After a protocol team submits fixes for audit findings
• During re-audit engagement for upgraded contracts
• When reviewing a PR that addresses a security issue
• Contest mitigation review phase (Sherlock, Code4rena)
• Any code change to a previously-audited contract

Fix Review Framework

Phase 1: Understand the Finding

Before reviewing the fix, re-read the original finding completely:

## Pre-Review Checklist
- [ ] Finding ID and severity confirmed
- [ ] Root cause fully understood (not just the symptom)
- [ ] Impact clearly documented
- [ ] Original PoC reviewed (if available)
- [ ] All variant instances listed

Phase 2: Analyze the Fix

Apply the 5-Point Fix Validation:

1. Root Cause Resolution

Q: Does the fix address the ROOT CAUSE, or just the symptom?

GOOD FIX (root cause):
  Finding: CEI violation in withdraw()
  Fix: Added ReentrancyGuard + nonReentrant modifier
  → Prevents ALL reentrancy, not just the specific path

BAD FIX (symptom only):
  Finding: CEI violation in withdraw()
  Fix: Moved one state update before one external call
  → May still be vulnerable via a different code path

2. Completeness Check

Q: Does the fix cover ALL instances of the vulnerability?

GOOD: Applied nonReentrant to withdraw(), claim(), AND liquidate()
BAD:  Applied nonReentrant to withdraw() only (ignored variants)

Check against the variant analysis from the original finding.

3. No New Vulnerabilities Introduced

Common patterns where fixes introduce new bugs:

| Original Fix | New Vulnerability Introduced | |---|---| | Added require(amount > 0) | Now users cannot withdraw dust amounts, funds permanently locked | | Changed transfer to safeTransfer | ERC777 tokensReceived hook now enables reentrancy | | Added nonReentrant modifier | Cross-contract reentrancy still possible if guard is per-contract | | Added access control to function | Legitimate users now blocked from valid operations | | Changed rounding direction | Opposite rounding error now created for different user type | | Added deadline check | Deadline is in wrong units (milliseconds vs seconds) | | Moved state update before call | Read-only reentrancy still returns stale state in view functions |

4. No Regressions

Q: Does the fix break any existing legitimate functionality?

Check:
- [ ] All existing tests still pass
- [ ] New tests added specifically for the vulnerability
- [ ] Edge cases still handled correctly (0 amount, max amount, empty array)
- [ ] Gas impact is acceptable (new modifier doesn't make function too expensive)
- [ ] Compatibility with existing integrations preserved

5. Minimality

Q: Does the fix change only what is necessary?

RED FLAGS:
- Large refactoring alongside the fix (hides changes)
- Unrelated changes bundled in the same PR
- Storage layout changes in upgradeable contracts
- New dependencies added (expanded attack surface)
- Function signatures changed (breaks composability)

Phase 3: Test the Fix

Manual Verification

// BEFORE (vulnerable):
function withdraw(uint256 amount) external {
    require(balances[msg.sender] >= amount, "Insufficient");
    token.safeTransfer(msg.sender, amount); // external call first
    balances[msg.sender] -= amount;          // state update second
}

// AFTER (fixed):
function withdraw(uint256 amount) external nonReentrant {
    require(balances[msg.sender] >= amount, "Insufficient");
    balances[msg.sender] -= amount;          // state update first
    token.safeTransfer(msg.sender, amount);  // external call second
}

// VERIFICATION:
// ✅ nonReentrant modifier added — prevents re-entry
// ✅ CEI pattern applied — state before interaction
// ✅ Both mitigations applied — defense in depth
// ⚠️ Check: Are claim() and liquidate() also fixed?

Reproduce Original PoC Against Fix

// If original PoC was provided, run it against the fixed code
// Expected: PoC should FAIL (revert) after the fix

function test_reentrancy_fix() public {
    // Setup attacker contract with reentrancy callback
    AttackContract attacker = new AttackContract(vault);
    vault.deposit{value: 10 ether}();
    
    // Try the original attack
    vm.expectRevert(); // Should revert now
    attacker.attack();
    
    // Verify vault funds are intact
    assertEq(address(vault).balance, 10 ether);
}

Write Regression Tests

// Test that legitimate functionality still works
function test_withdraw_still_works() public {
    vault.deposit{value: 5 ether}();
    vault.withdraw(3 ether);
    assertEq(vault.balances(address(this)), 2 ether);
}

// Test edge cases
function test_withdraw_zero() public {
    // Should this revert or succeed? Check spec.
    vault.deposit{value: 5 ether}();
    vault.withdraw(0);
}

function test_withdraw_full_balance() public {
    vault.deposit{value: 5 ether}();
    vault.withdraw(5 ether);
    assertEq(vault.balances(address(this)), 0);
}

Phase 4: Severity Re-Assessment

After reviewing the fix, update the finding status:

| Status | Meaning | |---|---| | Fixed | Root cause fully addressed, all instances covered, no regressions | | Partially Fixed | Some instances fixed, others remain; or fix is incomplete | | Not Fixed | Fix does not address the root cause at all | | Acknowledged | Team accepts the risk, chose not to fix (document reasoning) | | Disputed | Team disagrees with finding validity (document both positions) | | New Issue | Fix introduces a new, different vulnerability |

Phase 5: Report Fix Review Results

Template for each finding:

## Finding [ID]: [Title]

**Original Severity**: High
**Fix Status**: Fixed / Partially Fixed / Not Fixed / Acknowledged / Disputed

### Fix Summary
[One-sentence description of what the fix does]

### Fix Analysis
- Root cause addressed: Yes/No
- All instances covered: Yes/No (list missing instances if partial)
- New vulnerabilities: None found / [describe new issue]
- Regressions: None found / [describe regression]
- Tests added: Yes/No

### Verification
[How the fix was verified — PoC test results, code review notes]

### Recommendation (if partially/not fixed)
[What additional changes are needed]

Common Fix Patterns and Their Risks

Reentrancy Fixes

| Fix Pattern | Risk Level | Notes | |---|---|---| | Add nonReentrant modifier | Low risk | Best approach — prevents all reentrancy in that function | | Reorder to CEI | Low risk | Good but may miss cross-function reentrancy | | Both CEI + nonReentrant | Lowest risk | Defense in depth — recommended | | Add mutex lock | Medium risk | Custom implementation may have bugs | | Use transfer() (2300 gas) | High risk | Breaks with EIP-1884 and future gas changes |

Access Control Fixes

| Fix Pattern | Risk Level | Notes | |---|---|---| | Add onlyOwner modifier | Low risk | Standard, but check if owner is multisig | | Add role-based access | Low risk | Use OpenZeppelin AccessControl | | Add initializer modifier | Low risk | Also call _disableInitializers() in constructor | | Add require(msg.sender == x) | Medium risk | Inline checks are error-prone vs modifiers |

Oracle Fixes

| Fix Pattern | Risk Level | Notes | |---|---|---| | Switch to Chainlink TWAP | Low risk | Ensure heartbeat and staleness checks added | | Add staleness check | Low risk | Use updatedAt from latestRoundData() | | Add deviation threshold | Medium risk | Must be tuned per asset — too tight = DoS, too loose = manipulation | | Switch from spot to TWAP | Low risk | Verify TWAP window is sufficient (30 min minimum) |

Arithmetic Fixes

| Fix Pattern | Risk Level | Notes | |---|---|---| | Use mulDivUp/mulDivDown | Low risk | Explicit rounding direction | | Add dead shares / virtual offset | Low risk | For ERC4626 first depositor — check amount is sufficient | | Add minimum deposit | Medium risk | May block legitimate small deposits | | Switch to SafeCast | Low risk | Reverts on overflow instead of silent truncation |

Upgrade-Specific Fix Review

When the fix is deployed via proxy upgrade:

## Upgrade Safety Checklist
- [ ] Storage layout is backward-compatible (no variable reordering)
- [ ] New state variables added ONLY at the end
- [ ] No removed state variables (use `__gap` slots)
- [ ] Initializer version incremented (reinitializer(2), etc.)
- [ ] Old storage slots not reinterpreted as different types
- [ ] Immutable variables consistent between old and new impl
- [ ] Constructor calls `_disableInitializers()`

Integration Points

| Skill | How It's Used | |---|---| | variant-analysis/ | Check if fix covers all variant instances | | differential-review/ | Compare old vs new code systematically | | severity/ | Re-assess severity after fix | | methodology/ | Apply standard verification methodology | | checklists/ | Use protocol-specific checklist for regression |