diff --git a/packages/scanner-core/src/plugin-registry.test.ts b/packages/scanner-core/src/plugin-registry.test.ts index 1d564cc..bdbc79d 100644 --- a/packages/scanner-core/src/plugin-registry.test.ts +++ b/packages/scanner-core/src/plugin-registry.test.ts @@ -46,6 +46,16 @@ describe('PluginRegistry', () => { expect(registry.size).toBe(1); }); + it('should register initial plugins from the constructor', () => { + const registryWithInitialPlugins = new PluginRegistry([ + createMockPlugin('reentrancy'), + createMockPlugin('unchecked-return'), + ]); + + expect(registryWithInitialPlugins.size).toBe(2); + expect(registryWithInitialPlugins.get('unchecked-return')?.metadata.category).toBe('CUSTOM'); + }); + it('should retrieve a registered plugin', () => { const plugin = createMockPlugin('test-plugin'); registry.register(plugin); diff --git a/packages/scanner-core/src/plugin-registry.ts b/packages/scanner-core/src/plugin-registry.ts index 710b1bd..8832ecd 100644 --- a/packages/scanner-core/src/plugin-registry.ts +++ b/packages/scanner-core/src/plugin-registry.ts @@ -4,6 +4,10 @@ import type { AnalysisContext, IRulePlugin, PluginMetadata } from '@veridion/sca export class PluginRegistry { private plugins = new Map(); + constructor(initialPlugins: IRulePlugin[] = []) { + this.registerAll(initialPlugins); + } + register(plugin: IRulePlugin): void { if (this.plugins.has(plugin.metadata.id)) { logger.warn({ pluginId: plugin.metadata.id }, 'Plugin already registered, overwriting'); diff --git a/plugins/unchecked-return/.eslintrc.js b/plugins/unchecked-return/.eslintrc.js new file mode 100644 index 0000000..7c0ed9b --- /dev/null +++ b/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); diff --git a/plugins/unchecked-return/package.json b/plugins/unchecked-return/package.json new file mode 100644 index 0000000..0634d98 --- /dev/null +++ b/plugins/unchecked-return/package.json @@ -0,0 +1,28 @@ +{ + "name": "@veridion/plugin-unchecked-return", + "version": "0.1.0", + "private": true, + "description": "Unchecked low-level call return 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" + } +} diff --git a/plugins/unchecked-return/src/index.test.ts b/plugins/unchecked-return/src/index.test.ts new file mode 100644 index 0000000..d620cf9 --- /dev/null +++ b/plugins/unchecked-return/src/index.test.ts @@ -0,0 +1,173 @@ +import type { AnalysisContext } from '@veridion/scanner-types'; +import { FindingSeverity } from '@veridion/shared'; +import { describe, expect, it } from 'vitest'; + +import { UncheckedReturnPlugin } from './index'; + +describe('UncheckedReturnPlugin', () => { + const plugin = new UncheckedReturnPlugin(); + + function createContext(sourceCode: string): AnalysisContext { + return { + contractName: 'Vault', + sourceCode, + chain: 'ethereum', + language: 'solidity', + compilerVersion: '0.8.19', + metadata: {}, + }; + } + + it('should have correct metadata', () => { + expect(plugin.metadata.id).toBe('unchecked-return'); + expect(plugin.metadata.severity).toBe('HIGH'); + expect(plugin.metadata.category).toBe('UNCHECKED_RETURN'); + expect(plugin.metadata.languages).toContain('solidity'); + }); + + it('should support solidity on EVM chains', () => { + expect(plugin.supportsContext(createContext(''))).toBe(true); + }); + + it('should reject unsupported languages', () => { + expect( + plugin.supportsContext({ + ...createContext(''), + language: 'rust', + }), + ).toBe(false); + }); + + it('should detect unchecked call return values', async () => { + const findings = await plugin.analyze( + createContext(` +contract Vault { + function withdraw(address target, bytes memory data) external { + target.call(data); + } +}`), + ); + + expect(findings).toHaveLength(1); + expect(findings[0]?.title).toContain('call'); + expect(findings[0]?.recommendation).toContain('Capture the boolean return value'); + }); + + it('should detect assigned but unchecked call return values', async () => { + const findings = await plugin.analyze( + createContext(` +contract Vault { + function withdraw(address target, bytes memory data) external { + (bool success, bytes memory result) = target.call(data); + result; + } +}`), + ); + + expect(findings).toHaveLength(1); + expect(findings[0]?.recommendation).toContain('success'); + }); + + it('should detect unchecked send return values', async () => { + const findings = await plugin.analyze( + createContext(` +contract Vault { + function refund(address payable target, uint256 amount) external { + target.send(amount); + } +}`), + ); + + expect(findings).toHaveLength(1); + expect(findings[0]?.title).toContain('send'); + }); + + it('should detect unchecked delegatecall return values', async () => { + const findings = await plugin.analyze( + createContext(` +contract Proxy { + function forward(address implementation, bytes memory data) external { + (bool ok, ) = implementation.delegatecall(data); + } +}`), + ); + + expect(findings).toHaveLength(1); + expect(findings[0]?.title).toContain('delegatecall'); + }); + + it('should not flag call return values checked with require', async () => { + const findings = await plugin.analyze( + createContext(` +contract Vault { + function withdraw(address target, bytes memory data) external { + (bool success, ) = target.call(data); + require(success, "low-level call failed"); + } +}`), + ); + + expect(findings).toHaveLength(0); + }); + + it('should not flag send return values checked inline', async () => { + const findings = await plugin.analyze( + createContext(` +contract Vault { + function refund(address payable target, uint256 amount) external { + if (!target.send(amount)) { + revert("refund failed"); + } + } +}`), + ); + + expect(findings).toHaveLength(0); + }); + + it('should not flag delegatecall values checked with a branch', async () => { + const findings = await plugin.analyze( + createContext(` +contract Proxy { + function forward(address implementation, bytes memory data) external { + (bool ok, ) = implementation.delegatecall(data); + if (!ok) { + revert("delegatecall failed"); + } + } +}`), + ); + + expect(findings).toHaveLength(0); + }); + + it('should ignore comments that mention low-level calls', async () => { + const findings = await plugin.analyze( + createContext(` +contract Vault { + // target.call(data); + function ok() external {} +}`), + ); + + expect(findings).toHaveLength(0); + }); + + it('should provide a fix recommendation for findings', () => { + const recommendation = plugin.getFixRecommendation({ + pluginId: 'unchecked-return', + title: 'Unchecked call Return Value', + description: '', + severity: FindingSeverity.HIGH, + filePath: 'Vault.sol', + lineStart: 3, + lineEnd: 3, + codeSnippet: 'target.call(data);', + recommendation: '', + confidence: 0.9, + references: [], + }); + + expect(recommendation).toContain('require(success'); + }); +}); diff --git a/plugins/unchecked-return/src/index.ts b/plugins/unchecked-return/src/index.ts new file mode 100644 index 0000000..a6fc6de --- /dev/null +++ b/plugins/unchecked-return/src/index.ts @@ -0,0 +1,162 @@ +import type { + AnalysisContext, + FindingResult, + IRulePlugin, + PluginMetadata, +} from '@veridion/scanner-types'; +import { FindingSeverity } from '@veridion/shared'; + +const LOW_LEVEL_CALL_PATTERN = /\.(call|send|delegatecall)\s*(?:\{[^}]*\})?\s*\(/g; +const EVM_CHAINS = ['ethereum', 'polygon', 'bsc', 'avalanche', 'arbitrum', 'optimism']; +const CHECK_LOOKAHEAD_LINES = 12; + +const metadata: PluginMetadata = { + id: 'unchecked-return', + name: 'Unchecked Return Value Detector', + version: '1.0.0', + description: + 'Detects low-level Solidity call, send, and delegatecall results that are ignored or assigned without validation.', + severity: FindingSeverity.HIGH, + category: 'UNCHECKED_RETURN', + chains: EVM_CHAINS, + languages: ['solidity'], + tags: ['unchecked-return', 'low-level-call', 'call', 'send', 'delegatecall'], + author: 'Veridion', + references: [ + 'https://swcregistry.io/docs/SWC-104', + 'https://consensys.github.io/smart-contract-best-practices/development-recommendations/general/external-calls/', + ], +}; + +export class UncheckedReturnPlugin implements IRulePlugin { + readonly metadata = metadata; + + async initialize(_config?: Record): Promise { + // noop + } + + // eslint-disable-next-line @typescript-eslint/require-await + async analyze(context: AnalysisContext): Promise { + const findings: FindingResult[] = []; + const lines = context.sourceCode.split('\n'); + + for (let i = 0; i < lines.length; i++) { + const sourceLine = lines[i]; + if (!sourceLine) continue; + + const codeLine = stripInlineComment(sourceLine); + LOW_LEVEL_CALL_PATTERN.lastIndex = 0; + + let match: RegExpExecArray | null; + while ((match = LOW_LEVEL_CALL_PATTERN.exec(codeLine)) !== null) { + const callKind = match[1]; + if (!callKind || isInlineChecked(codeLine, match.index)) continue; + + const checkedVariable = extractCheckedVariable(codeLine, match.index, callKind); + if (checkedVariable && isVariableChecked(lines, i, checkedVariable)) continue; + + findings.push(createFinding(context, sourceLine, i + 1, callKind, checkedVariable)); + } + } + + return findings; + } + + getFixRecommendation(finding: FindingResult): string { + return `Handle the boolean return from the low-level call at ${finding.filePath}:${finding.lineStart}. Assign it to a variable and enforce it with require(success, "low-level call failed") or revert inside an if (!success) branch.`; + } + + supportsContext(context: AnalysisContext): boolean { + return ( + this.metadata.chains.includes(context.chain) && + this.metadata.languages.includes(context.language) + ); + } +} + +function stripInlineComment(line: string): string { + const commentIndex = line.indexOf('//'); + return commentIndex === -1 ? line : line.slice(0, commentIndex); +} + +function isInlineChecked(line: string, matchIndex: number): boolean { + const prefix = line.slice(0, matchIndex); + + return ( + /\b(?:require|assert)\s*\([^;]*$/.test(prefix) || + /\bif\s*\([^;]*$/.test(prefix) || + /\breturn\s+[^;]*$/.test(prefix) + ); +} + +function extractCheckedVariable(line: string, matchIndex: number, callKind: string): string | null { + const prefix = line.slice(0, matchIndex); + + if (callKind === 'send') { + const boolAssignment = prefix.match(/\b(?:bool\s+)?([A-Za-z_$][\w$]*)\s*=\s*[^=]*$/); + return boolAssignment?.[1] ?? null; + } + + const tupleAssignment = prefix.match(/\(\s*(?:bool\s+)?([A-Za-z_$][\w$]*)\s*,/); + return tupleAssignment?.[1] ?? null; +} + +function isVariableChecked(lines: string[], startIndex: number, variableName: string): boolean { + if (variableName === '_') return false; + + const escapedName = escapeRegExp(variableName); + const variablePattern = new RegExp(`\\b${escapedName}\\b`); + const positiveGuardPattern = new RegExp( + `\\b(?:require|assert)\\s*\\(\\s*${escapedName}(?:\\s*(?:==|!=)\\s*(?:true|false))?\\b`, + ); + const negativeGuardPattern = new RegExp(`\\bif\\s*\\(\\s*!\\s*${escapedName}\\b`); + const branchGuardPattern = new RegExp(`\\bif\\s*\\(\\s*${escapedName}\\b`); + + for (let i = startIndex; i < Math.min(startIndex + CHECK_LOOKAHEAD_LINES, lines.length); i++) { + const currentLine = stripInlineComment(lines[i] ?? ''); + if (!variablePattern.test(currentLine)) continue; + + if ( + positiveGuardPattern.test(currentLine) || + negativeGuardPattern.test(currentLine) || + branchGuardPattern.test(currentLine) + ) { + return true; + } + } + + return false; +} + +function createFinding( + context: AnalysisContext, + sourceLine: string, + lineNumber: number, + callKind: string, + checkedVariable: string | null, +): FindingResult { + const recommendation = + checkedVariable === null + ? 'Capture the boolean return value and require it before continuing, for example: (bool success, ) = target.call(data); require(success, "low-level call failed");' + : `Validate ${checkedVariable} with require(${checkedVariable}, "low-level call failed") or revert when it is false.`; + + return { + pluginId: metadata.id, + title: `Unchecked ${callKind} Return Value`, + description: + `The result of a low-level Solidity ${callKind} operation is not checked. ` + + 'Ignoring this return value can let execution continue after a failed external call, leaving contract state inconsistent.', + severity: metadata.severity, + filePath: `${context.contractName}.sol`, + lineStart: lineNumber, + lineEnd: lineNumber, + codeSnippet: sourceLine.trim(), + recommendation, + confidence: checkedVariable === null ? 0.9 : 0.85, + references: metadata.references ?? [], + }; +} + +function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} diff --git a/plugins/unchecked-return/tsconfig.json b/plugins/unchecked-return/tsconfig.json new file mode 100644 index 0000000..54f3e6d --- /dev/null +++ b/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"] +} diff --git a/plugins/unchecked-return/vitest.config.ts b/plugins/unchecked-return/vitest.config.ts new file mode 100644 index 0000000..7dd1325 --- /dev/null +++ b/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'], + }, +}); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 9f79a6f..cc339f9 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -854,6 +854,34 @@ importers: specifier: ^1.6.0 version: 1.6.1(@types/node@20.19.43)(jsdom@24.1.3)(terser@5.50.0) + plugins/unchecked-return: + dependencies: + '@veridion/logger': + specifier: workspace:* + version: link:../../packages/logger + '@veridion/scanner-types': + specifier: workspace:* + version: link:../../packages/scanner-types + '@veridion/shared': + specifier: workspace:* + version: link:../../packages/shared + devDependencies: + '@veridion/eslint-config': + specifier: workspace:* + version: link:../../packages/config/eslint + '@veridion/tsconfig': + specifier: workspace:* + version: link:../../packages/config/tsconfig + eslint: + specifier: ^8.57.0 + version: 8.57.1 + typescript: + specifier: ^5.4.5 + version: 5.9.3 + vitest: + specifier: ^1.6.0 + version: 1.6.1(@types/node@20.19.43)(jsdom@24.1.3)(terser@5.50.0) + packages: '@alloc/quick-lru@5.2.0': @@ -5216,10 +5244,6 @@ packages: js-tokens@9.0.1: resolution: {integrity: sha512-mxa9E9ITFOt0ban3j6L5MpjwegGz6lBQmM1IJkWeBZGcMxto50+eWdjC/52xDbS2vy0k7vIMK0Fe2wfL9OQSpQ==} - js-yaml@3.15.0: - resolution: {integrity: sha512-ttBQIIQPDeLjpPOohtUdXuXUVoA2uIB6fEH9HyJ7234s5mBJ5wTx20njxplLZQgLaOfpmPQA7X2t5AX6tIPbog==} - hasBin: true - js-yaml@3.15.1: resolution: {integrity: sha512-S99WuO3HlhO3XN41EtYUNl9zzXjoJx7QvmipxsJVxtCBT0YHEFy+iOJhjSvrmV12nYhWpZaM8lPHkJm0yUMbag==} hasBin: true @@ -7727,7 +7751,7 @@ snapshots: '@changesets/parse@0.4.3': dependencies: '@changesets/types': 6.1.0 - js-yaml: 4.3.0 + js-yaml: 4.3.1 '@changesets/pre@2.0.2': dependencies: @@ -8077,7 +8101,7 @@ snapshots: globals: 13.24.0 ignore: 5.3.2 import-fresh: 3.3.1 - js-yaml: 4.3.0 + js-yaml: 4.3.1 minimatch: 3.1.5 strip-json-comments: 3.1.1 transitivePeerDependencies: @@ -9938,9 +9962,9 @@ snapshots: dependencies: acorn: 8.18.0 - acorn-jsx@5.3.2(acorn@8.17.0): + acorn-jsx@5.3.2(acorn@8.18.0): dependencies: - acorn: 8.17.0 + acorn: 8.18.0 acorn-walk@8.3.5: dependencies: @@ -10559,7 +10583,7 @@ snapshots: dependencies: env-paths: 2.2.1 import-fresh: 3.3.1 - js-yaml: 4.3.0 + js-yaml: 4.3.1 parse-json: 5.2.0 optionalDependencies: typescript: 5.9.3 @@ -11109,8 +11133,8 @@ snapshots: espree@9.6.1: dependencies: - acorn: 8.17.0 - acorn-jsx: 5.3.2(acorn@8.17.0) + acorn: 8.18.0 + acorn-jsx: 5.3.2(acorn@8.18.0) eslint-visitor-keys: 3.4.3 esprima@4.0.1: {} @@ -12231,11 +12255,6 @@ snapshots: js-tokens@9.0.1: {} - js-yaml@3.15.0: - dependencies: - argparse: 1.0.10 - esprima: 4.0.1 - js-yaml@3.15.1: dependencies: argparse: 1.0.10 @@ -13218,7 +13237,7 @@ snapshots: read-yaml-file@1.1.0: dependencies: graceful-fs: 4.2.11 - js-yaml: 3.15.0 + js-yaml: 3.15.1 pify: 4.0.1 strip-bom: 3.0.0