Smart Contract Security Review

Review Methodology

Run these skills in order, then verify findings before reporting:

Step 1: entry-point-analyzer Map all public/external entry points and their parameters.

Step 2: audit-context-building Deep code analysis. Apply 5 Whys to every conditional branch involving user-controlled parameters.

Step 3: guidelines-advisor Best practices and common pitfalls check.

Step 4: Verification For each raw finding, verify by tracing exact execution path. Filter findings with confidence < 8/10. Never report without verification—false positives waste developer time.

Output Format

After completing all steps, output ONLY a findings table.

Include ONLY findings that could lead to: loss of funds, unauthorized access, reward manipulation, or denial of service.

Exclude: documentation gaps, gas optimizations, code style, theoretical issues with unsupported token types.

| ID | Finding | Severity | File:Line | Evidence |
|----|---------|----------|-----------|----------|
| 1  | [Description] | CRITICAL/HIGH/MEDIUM/LOW | `File.sol:XX` | [Proof or attack path] |

Input Validation & 5 Whys

Every user-provided input must be validated. Apply 5 Whys to understand why each branch exists and whether an attacker can exploit it:

Observation: Token transfer is skipped when owner == address(this)

Why #1: Why is the transfer skipped?
→ The code assumes tokens are already in the contract

Why #2: Why would tokens already be there?
→ Internal flow deposits tokens before calling

Why #3: Why is there no check that internal flow actually called?
→ The code trusts only internal flows use owner == address(this)

Why #4: Why would only internal flows use that value?
→ No reason - external callers can pass any owner value

Why #5: Why is this dangerous?
→ Attacker creates unbacked stake, drains rewards

Special values to test for each user-controlled parameter:

• •address(0) - zero address
• •address(this) - the contract itself
• •msg.sender - caller address
• •type(uintX).max - maximum values

Validation questions:

• •Is there any legitimate reason to allow this input value?
• •Does the code path assume trusted caller but accept untrusted input?
• •Are all branches of conditional logic safe for all possible inputs?

Example vulnerability:

// VULNERABLE - allows arbitrary owner without validation:
function stakeOnBehalf(..., address owner, ...) external {
    // Missing: if (owner == address(this)) revert InvalidOwner();
    depositId = _stake(amount, delegatee, beneficiary, owner, false);
}

// In _stake():
if (owner != address(this)) {
    ds.staking.stakeToken.safeTransferFrom(msg.sender, proxy, amount);
} else {
    // NO TRANSFER - assumes internal use only
    // But stakeOnBehalf allows external callers to reach this path!
}

Token Flow Analysis

Always trace: (1) where tokens come FROM, (2) where tokens go TO, (3) who gets ownership/benefits

// Tokens: FROM msg.sender, TO proxy
ds.staking.stakeToken.safeTransferFrom(msg.sender, proxy, amount);

// Questions to answer:
// - Who pays the tokens? (msg.sender vs arbitrary parameter)
// - Who receives the tokens/deposit?
// - Who gets the benefits (rewards, ownership)?
// - What happens if transfer is skipped?

EVM Call Frames and Revert Behavior

Both regular function calls and low-level calls create new call frames. The difference is how reverts are handled:

// Regular function call - revert propagates automatically:
function foo() external {
    bar();  // If bar() reverts, foo() reverts too, all state rolled back
}

// Low-level call - revert is captured, caller decides:
function foo() external {
    storage.value = 1;  // This persists even if inner call reverts!

    (bool success, ) = address(this).call(
        abi.encodeCall(IContract.method, (arg))
    );

    if (!success) {
        // Inner call reverted - but we continue execution
        // State changes INSIDE the .call() are rolled back
        // State changes BEFORE the .call() persist!
    }
}

Key points:

• •Low-level address.call(): captures revert, returns (false, errorData), caller decides to bubble up or continue
• •try-catch: syntactic sugar that also captures reverts, state before try persists on catch
• •State changes BEFORE .call() or try persist even if inner call reverts

Common False Positives

Intentional Design Patterns

Recognize when "vulnerabilities" are intentional:

• •Silent failures in batch operations: Prevent one bad item from bricking entire batch
• •Self-healing mechanisms: Failed operations retry on next call
• •Cross-chain resilience: L1→L2 messages are expensive to retry

Finding Severity Guidelines