feat(scanner): implement Unchecked Return Value detection plugin (#16)

packages/scanner-core/package.json

@@ -20,6 +20,7 @@
   },
   "dependencies": {
     "@veridion/logger": "workspace:*",
+    "@veridion/plugin-unchecked-return": "workspace:*",
     "@veridion/scanner-types": "workspace:*",
     "@veridion/shared": "workspace:*"
   },

packages/scanner-core/src/plugin-registry.ts

@@ -1,5 +1,23 @@
 import { logger } from '@veridion/logger';
 import type { AnalysisContext, IRulePlugin, PluginMetadata } from '@veridion/scanner-types';
+import { UncheckedReturnPlugin } from '@veridion/plugin-unchecked-return';
+
+/**
+ * Instantiate the plugins that ship with the scanner. New built-in plugins
+ * should be added here so they are picked up by {@link createDefaultRegistry}.
+ */
+export function createBuiltinPlugins(): IRulePlugin[] {
+  return [new UncheckedReturnPlugin()];
+}
+
+/**
+ * Create a registry pre-populated with all built-in plugins.
+ */
+export function createDefaultRegistry(): PluginRegistry {
+  const registry = new PluginRegistry();
+  registry.registerAll(createBuiltinPlugins());
+  return registry;
+}
 
 export class PluginRegistry {
   private plugins = new Map<string, IRulePlugin>();

plugins/unchecked-return/.eslintrc.js

@@ -0,0 +1,4 @@
+const { createConfig } = require('@veridion/eslint-config/base');
+
+/** @type {import('eslint').Linter.Config} */
+module.exports = createConfig(__dirname);

plugins/unchecked-return/package.json

@@ -0,0 +1,28 @@
+{
+  "name": "@veridion/plugin-unchecked-return",
+  "version": "0.1.0",
+  "private": true,
+  "description": "Unchecked return value detection plugin for Veridion scanner",
+  "main": "./src/index.ts",
+  "types": "./src/index.ts",
+  "scripts": {
+    "build": "tsc",
+    "typecheck": "tsc --noEmit",
+    "lint": "eslint src/ --max-warnings 0",
+    "test": "vitest run",
+    "test:watch": "vitest",
+    "clean": "rm -rf dist"
+  },
+  "dependencies": {
+    "@veridion/scanner-types": "workspace:*",
+    "@veridion/shared": "workspace:*",
+    "@veridion/logger": "workspace:*"
+  },
+  "devDependencies": {
+    "@veridion/eslint-config": "workspace:*",
+    "@veridion/tsconfig": "workspace:*",
+    "eslint": "^8.57.0",
+    "typescript": "^5.4.5",
+    "vitest": "^1.6.0"
+  }
+}

plugins/unchecked-return/src/index.test.ts

@@ -0,0 +1,162 @@
+import { describe, expect, it } from 'vitest';
+
+import { UncheckedReturnPlugin } from './index';
+
+const ctx = (contractName: string, sourceCode: string) => ({
+  contractName,
+  sourceCode,
+  chain: 'ethereum',
+  language: 'solidity',
+  compilerVersion: '0.8.19',
+  metadata: {},
+});
+
+describe('UncheckedReturnPlugin', () => {
+  const plugin = new UncheckedReturnPlugin();
+
+  it('should have correct metadata', () => {
+    expect(plugin.metadata.id).toBe('unchecked-return');
+    expect(plugin.metadata.severity).toBe('MEDIUM');
+    expect(plugin.metadata.category).toBe('UNCHECKED_RETURN');
+  });
+
+  it('should support solidity on ethereum', () => {
+    expect(plugin.supportsContext(ctx('Test', ''))).toBe(true);
+  });
+
+  it('should not support vyper or non-configured chains', () => {
+    expect(
+      plugin.supportsContext({ ...ctx('Test', ''), language: 'vyper' }),
+    ).toBe(false);
+    expect(
+      plugin.supportsContext({ ...ctx('Test', ''), chain: 'solana' }),
+    ).toBe(false);
+  });
+
+  it('should detect an unchecked .send() return value', async () => {
+    const code = `
+contract Vulnerable {
+    function pay(address payable to, uint256 amount) public {
+        to.send(amount);
+    }
+}`;
+    const findings = await plugin.analyze(ctx('Vulnerable', code));
+    expect(findings.length).toBe(1);
+    expect(findings[0]?.pluginId).toBe('unchecked-return');
+    expect(findings[0]?.title).toContain('send');
+  });
+
+  it('should detect an unchecked .call() return value', async () => {
+    const code = `
+contract Vulnerable {
+    function pay(address to, uint256 amount) public {
+        to.call{value: amount}("");
+    }
+}`;
+    const findings = await plugin.analyze(ctx('Vulnerable', code));
+    expect(findings.length).toBe(1);
+    expect(findings[0]?.title).toContain('call');
+  });
+
+  it('should detect an unchecked .delegatecall() return value', async () => {
+    const code = `
+contract Vulnerable {
+    function forward(address impl, bytes calldata data) public {
+        impl.delegatecall(data);
+    }
+}`;
+    const findings = await plugin.analyze(ctx('Vulnerable', code));
+    expect(findings.length).toBe(1);
+    expect(findings[0]?.title).toContain('delegatecall');
+  });
+
+  it('should NOT flag a send() checked with require', async () => {
+    const code = `
+contract Safe {
+    function pay(address payable to, uint256 amount) public {
+        require(to.send(amount), "send failed");
+    }
+}`;
+    const findings = await plugin.analyze(ctx('Safe', code));
+    expect(findings.length).toBe(0);
+  });
+
+  it('should NOT flag a call() whose result is captured and checked', async () => {
+    const code = `
+contract Safe {
+    function pay(address to, uint256 amount) public {
+        (bool success, ) = to.call{value: amount}("");
+        require(success, "call failed");
+    }
+}`;
+    const findings = await plugin.analyze(ctx('Safe', code));
+    expect(findings.length).toBe(0);
+  });
+
+  it('should NOT flag a send() assigned to a bool variable', async () => {
+    const code = `
+contract Safe {
+    function pay(address payable to, uint256 amount) public {
+        bool ok = to.send(amount);
+        require(ok);
+    }
+}`;
+    const findings = await plugin.analyze(ctx('Safe', code));
+    expect(findings.length).toBe(0);
+  });
+
+  it('should NOT flag a call() used directly in an if condition', async () => {
+    const code = `
+contract Safe {
+    function pay(address to) public {
+        if (to.call("")) {
+            revert();
+        }
+    }
+}`;
+    const findings = await plugin.analyze(ctx('Safe', code));
+    expect(findings.length).toBe(0);
+  });
+
+  it('should ignore matches inside comments', async () => {
+    const code = `
+contract Documented {
+    // to.send(amount); is unsafe, do not do this
+    function pay() public {}
+}`;
+    const findings = await plugin.analyze(ctx('Documented', code));
+    expect(findings.length).toBe(0);
+  });
+
+  it('should flag multiple distinct unchecked calls', async () => {
+    const code = `
+contract Vulnerable {
+    function a(address payable to, uint256 amount) public {
+        to.send(amount);
+        to.call{value: amount}("");
+    }
+}`;
+    const findings = await plugin.analyze(ctx('Vulnerable', code));
+    expect(findings.length).toBe(2);
+  });
+
+  it('should provide a require(success) fix recommendation', async () => {
+    const code = `
+contract Vulnerable {
+    function pay(address to) public {
+        to.call("");
+    }
+}`;
+    const findings = await plugin.analyze(ctx('Vulnerable', code));
+    const finding = findings[0];
+    expect(finding).toBeDefined();
+    if (!finding) return;
+    const fix = plugin.getFixRecommendation(finding);
+    expect(fix).toContain('require(success)');
+    expect(fix).toContain('Vulnerable.sol');
+  });
+
+  it('should initialize without error', async () => {
+    await expect(plugin.initialize()).resolves.toBeUndefined();
+  });
+});

plugins/unchecked-return/src/index.ts

@@ -0,0 +1,147 @@
+import type {
+  AnalysisContext,
+  FindingResult,
+  IRulePlugin,
+  PluginMetadata,
+} from '@veridion/scanner-types';
+import { FindingSeverity } from '@veridion/shared';
+
+const metadata: PluginMetadata = {
+  id: 'unchecked-return',
+  name: 'Unchecked Return Value Detector',
+  version: '1.0.0',
+  description:
+    'Detects low-level calls (address.call, address.send, address.delegatecall) whose boolean return value is ignored. Silently ignoring a failed call can leave the contract in an inconsistent state.',
+  severity: FindingSeverity.MEDIUM,
+  category: 'UNCHECKED_RETURN',
+  chains: ['ethereum', 'polygon', 'bsc', 'avalanche', 'arbitrum', 'optimism'],
+  languages: ['solidity'],
+  tags: ['unchecked-return', 'low-level-call', 'send', 'call', 'delegatecall', 'error-handling'],
+  author: 'Veridion',
+  references: [
+    'https://swcregistry.io/docs/SWC-104',
+    'https://consensys.github.io/smart-contract-best-practices/development-recommendations/general/external-calls/',
+  ],
+};
+
+interface CallPattern {
+  readonly kind: 'call' | 'send' | 'delegatecall';
+  readonly regex: RegExp;
+}
+
+const CALL_PATTERNS: readonly CallPattern[] = [
+  // .call(...) and .call{value: ...}(...)
+  { kind: 'call', regex: /\.call\s*[({]/ },
+  // .delegatecall(...)
+  { kind: 'delegatecall', regex: /\.delegatecall\s*\(/ },
+  // .send(...)
+  { kind: 'send', regex: /\.send\s*\(/ },
+];
+
+export class UncheckedReturnPlugin implements IRulePlugin {
+  readonly metadata = metadata;
+
+  async initialize(_config?: Record<string, unknown>): Promise<void> {
+    // noop
+  }
+
+  // eslint-disable-next-line @typescript-eslint/require-await
+  async analyze(context: AnalysisContext): Promise<FindingResult[]> {
+    const findings: FindingResult[] = [];
+    const lines = context.sourceCode.split('\n');
+
+    for (let i = 0; i < lines.length; i++) {
+      const line = lines[i];
+      if (!line || this.isComment(line)) continue;
+
+      for (const pattern of CALL_PATTERNS) {
+        const match = pattern.regex.exec(line);
+        if (!match) continue;
+        // Only report the first matching kind on a given line to avoid duplicates
+        // (e.g. ".call" also appearing inside a longer expression).
+        if (this.isReturnChecked(line, match.index)) break;
+
+        findings.push({
+          pluginId: this.metadata.id,
+          title: `Unchecked return value from low-level ${pattern.kind}()`,
+          description:
+            `The return value of \`${pattern.kind}()\` is not checked. Low-level calls do not ` +
+            'revert on failure; they return a boolean indicating success. Ignoring it means a ' +
+            'failed transfer or call is treated as a success, potentially corrupting contract state.',
+          severity: this.metadata.severity,
+          filePath: `${context.contractName}.sol`,
+          lineStart: i + 1,
+          lineEnd: i + 1,
+          codeSnippet: line.trim(),
+          recommendation:
+            'Capture the boolean result and validate it, e.g. `(bool success, ) = target.call(...); ' +
+            'require(success, "call failed");`.',
+          confidence: 0.8,
+          references: this.metadata.references ?? [],
+        });
+        break;
+      }
+    }
+
+    return findings;
+  }
+
+  getFixRecommendation(finding: FindingResult): string {
+    return `To fix the unchecked return value at ${finding.filePath}:${finding.lineStart}:
+
+Low-level calls (\`call\`, \`send\`, \`delegatecall\`) return a boolean success flag instead of
+reverting. Always capture and check it:
+
+\`\`\`solidity
+// Unsafe: return value ignored
+recipient.send(amount);
+recipient.call{value: amount}("");
+
+// Safe: return value checked
+require(recipient.send(amount), "send failed");
+
+(bool success, ) = recipient.call{value: amount}("");
+require(success, "call failed");
+\`\`\`
+
+Prefer \`call\` over \`send\`/\`transfer\` for value transfers, and always guard the result with
+\`require(success)\`. For plain Ether transfers that must revert on failure, \`transfer()\` is also
+acceptable because it reverts automatically.`;
+  }
+
+  supportsContext(context: AnalysisContext): boolean {
+    return (
+      this.metadata.chains.includes(context.chain) &&
+      this.metadata.languages.includes(context.language)
+    );
+  }
+
+  /**
+   * Determine whether the boolean return value of a low-level call is consumed.
+   * We inspect the code preceding the call on the same line: an assignment,
+   * destructuring, or use inside a control/condition expression counts as checked.
+   */
+  private isReturnChecked(line: string, callIndex: number): boolean {
+    const before = line.slice(0, callIndex);
+
+    // Assigned to a variable or destructured: `bool ok = a.send(...)`, `(bool ok, ) = a.call(...)`
+    if (/[=]\s*$/.test(before) || /\)\s*=\s*$/.test(before) || /=\s*[\w.]*$/.test(before)) {
+      return true;
+    }
+
+    // Used inside require/assert/if/while/return or a boolean expression.
+    if (/\b(require|assert|if|while|return)\s*\(?[^;]*$/.test(before)) {
+      return true;
+    }
+    if (/(&&|\|\||!)\s*[\w.]*$/.test(before)) {
+      return true;
+    }
+
+    return false;
+  }
+
+  private isComment(line: string): boolean {
+    const trimmed = line.trim();
+    return trimmed.startsWith('//') || trimmed.startsWith('*') || trimmed.startsWith('/*');
+  }
+}

plugins/unchecked-return/tsconfig.json

@@ -0,0 +1,9 @@
+{
+  "extends": "@veridion/tsconfig/base.json",
+  "compilerOptions": {
+    "outDir": "./dist",
+    "rootDir": "./src"
+  },
+  "include": ["src/**/*.ts"],
+  "exclude": ["node_modules", "dist"]
+}

plugins/unchecked-return/vitest.config.ts

@@ -0,0 +1,9 @@
+import { defineConfig } from 'vitest/config';
+
+export default defineConfig({
+  test: {
+    globals: true,
+    environment: 'node',
+    include: ['src/**/*.test.ts'],
+  },
+});