diff --git a/packages/scanner-core/package.json b/packages/scanner-core/package.json index 41a3b94..d7dd214 100644 --- a/packages/scanner-core/package.json +++ b/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:*" }, diff --git a/packages/scanner-core/src/plugin-registry.test.ts b/packages/scanner-core/src/plugin-registry.test.ts index 1d564cc..e19572d 100644 --- a/packages/scanner-core/src/plugin-registry.test.ts +++ b/packages/scanner-core/src/plugin-registry.test.ts @@ -2,7 +2,7 @@ import type { IRulePlugin, PluginMetadata } from '@veridion/scanner-types'; import { FindingSeverity } from '@veridion/shared'; import { beforeEach, describe, expect, it } from 'vitest'; -import { PluginRegistry } from './plugin-registry'; +import { BUILTIN_PLUGIN_SPECIFIERS, PluginRegistry } from './plugin-registry'; function createMockPlugin( id: string, @@ -84,4 +84,15 @@ describe('PluginRegistry', () => { const allMeta = registry.getAllMetadata(); expect(allMeta).toHaveLength(2); }); + + it('exposes the built-in plugin specifier catalogue', () => { + expect(BUILTIN_PLUGIN_SPECIFIERS['unchecked-return']).toBe('@veridion/plugin-unchecked-return'); + }); + + it('registerBuiltins resolves without throwing and leaves size unchanged when the package is missing', async () => { + const before = registry.size; + const added = await registry.registerBuiltins(); + expect(added).toEqual([]); + expect(registry.size).toBe(before); + }); }); diff --git a/packages/scanner-core/src/plugin-registry.ts b/packages/scanner-core/src/plugin-registry.ts index 710b1bd..9ce4899 100644 --- a/packages/scanner-core/src/plugin-registry.ts +++ b/packages/scanner-core/src/plugin-registry.ts @@ -1,6 +1,79 @@ import { logger } from '@veridion/logger'; import type { AnalysisContext, IRulePlugin, PluginMetadata } from '@veridion/scanner-types'; +/** + * Built-in plugins shipped with the scanner, keyed by plugin id. + * + * Each entry stores a module specifier instead of being a static `import`, + * so that `scanner-core` keeps zero compile-time knowledge of plugin + * implementations (see ARCHITECTURE.md: "Scanner-core has zero knowledge of + * individual plugins"). At runtime the module is loaded dynamically and every + * export that satisfies {@link IRulePlugin} is instantiated. + */ +export const BUILTIN_PLUGIN_SPECIFIERS: Readonly> = { + 'unchecked-return': '@veridion/plugin-unchecked-return', +}; + +function isRulePlugin(value: unknown): value is IRulePlugin { + if (typeof value !== 'object' || value === null) return false; + const candidate = value as Partial; + return ( + typeof candidate.metadata?.id === 'string' && + typeof candidate.initialize === 'function' && + typeof candidate.analyze === 'function' && + typeof candidate.getFixRecommendation === 'function' && + typeof candidate.supportsContext === 'function' + ); +} + +/** + * Turn a single module export into a plugin instance. Plugins are shipped as + * a class (constructor), but an already-instantiated object is also accepted. + */ +function tryInstantiate(exportedValue: unknown): IRulePlugin | null { + if (isRulePlugin(exportedValue)) return exportedValue; + + if (typeof exportedValue === 'function') { + try { + const instance: unknown = new (exportedValue as new () => unknown)(); + if (isRulePlugin(instance)) return instance; + } catch { + // Not a constructable plugin class; ignore this export. + } + } + + return null; +} + +/** + * Dynamically import every built-in plugin and instantiate it. + * + * A failure to resolve a module (for example because the plugin package has + * not been declared as a dependency of the consuming package) is logged and + * skipped, so this call never throws. + */ +export async function loadBuiltinPlugins(): Promise { + const plugins: IRulePlugin[] = []; + + for (const [pluginId, specifier] of Object.entries(BUILTIN_PLUGIN_SPECIFIERS)) { + try { + const namespace = (await import(/* webpackIgnore: true */ specifier)) as unknown; + const exports = Object.values(namespace as Record); + for (const exportedValue of exports) { + const instance = tryInstantiate(exportedValue); + if (instance !== null) plugins.push(instance); + } + } catch (error) { + logger.warn( + { pluginId, specifier, err: error instanceof Error ? error.message : String(error) }, + 'Built-in plugin could not be loaded; install the matching workspace package to enable it', + ); + } + } + + return plugins; +} + export class PluginRegistry { private plugins = new Map(); @@ -21,6 +94,25 @@ export class PluginRegistry { } } + /** + * Register every plugin returned by {@link loadBuiltinPlugins}. + * + * Newly-loaded plugins that were not previously registered are added; + * plugins already in the registry are left untouched (use {@link register} + * to overwrite). + */ + async registerBuiltins(): Promise { + const loaded = await loadBuiltinPlugins(); + const added: IRulePlugin[] = []; + for (const plugin of loaded) { + if (!this.plugins.has(plugin.metadata.id)) { + this.register(plugin); + added.push(plugin); + } + } + return added; + } + unregister(pluginId: string): boolean { return this.plugins.delete(pluginId); } 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..822039e --- /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 return value detection plugin for Veridion scanner (SWC-104)", + "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..285ed68 --- /dev/null +++ b/plugins/unchecked-return/src/index.test.ts @@ -0,0 +1,384 @@ +import type { AnalysisContext } from '@veridion/scanner-types'; +import { FindingSeverity } from '@veridion/shared'; +import { beforeEach, describe, expect, it } from 'vitest'; + +import { UncheckedReturnPlugin } from './index'; + +function makeContext( + sourceCode: string, + overrides: Partial = {}, +): AnalysisContext { + return { + contractName: 'Vault', + sourceCode, + chain: 'ethereum', + language: 'solidity', + compilerVersion: '0.8.19', + metadata: {}, + ...overrides, + }; +} + +describe('UncheckedReturnPlugin', () => { + let plugin: UncheckedReturnPlugin; + + beforeEach(() => { + plugin = new UncheckedReturnPlugin(); + }); + + describe('metadata', () => { + it('exposes a kebab-case id matching the directory name', () => { + expect(plugin.metadata.id).toBe('unchecked-return'); + }); + + it('is categorised as UNCHECKED_RETURN with HIGH default severity', () => { + expect(plugin.metadata.category).toBe('UNCHECKED_RETURN'); + expect(plugin.metadata.severity).toBe(FindingSeverity.HIGH); + }); + + it('declares supported chains, languages and references', () => { + expect(plugin.metadata.chains).toContain('ethereum'); + expect(plugin.metadata.languages).toContain('solidity'); + expect(plugin.metadata.references?.length ?? 0).toBeGreaterThan(0); + }); + }); + + describe('initialize', () => { + it('resolves without configuration', async () => { + await expect(plugin.initialize()).resolves.toBeUndefined(); + }); + + it('accepts an arbitrary configuration object', async () => { + await expect(plugin.initialize({ includeErc20: true })).resolves.toBeUndefined(); + }); + }); + + describe('supportsContext', () => { + it('supports solidity on a declared chain', () => { + expect(plugin.supportsContext(makeContext('contract A {}'))).toBe(true); + }); + + it('rejects unsupported chains', () => { + expect(plugin.supportsContext(makeContext('contract A {}', { chain: 'solana' }))).toBe(false); + }); + + it('rejects unsupported languages', () => { + expect(plugin.supportsContext(makeContext('contract A {}', { language: 'vyper' }))).toBe( + false, + ); + }); + }); + + describe('low-level call detection', () => { + it('detects an unchecked .call() return value', async () => { + const findings = await plugin.analyze( + makeContext(`pragma solidity ^0.8.0; +contract Vault { + function pay(address to) public { + to.call{value: 1 ether}(""); + } +}`), + ); + + expect(findings).toHaveLength(1); + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + expect(findings[0]!.severity).toBe(FindingSeverity.HIGH); + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + expect(findings[0]!.confidence).toBeGreaterThanOrEqual(0.9); + }); + + it('detects an unchecked .send() return value', async () => { + const findings = await plugin.analyze( + makeContext(`contract Vault { + function pay(address to) public { + to.send(1 ether); + } +}`), + ); + + expect(findings).toHaveLength(1); + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + expect(findings[0]!.title).toContain('send'); + }); + + it('detects an unchecked .delegatecall() return value', async () => { + const findings = await plugin.analyze( + makeContext(`contract Vault { + function exec(address impl, bytes memory data) public { + impl.delegatecall(data); + } +}`), + ); + + expect(findings).toHaveLength(1); + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + expect(findings[0]!.title).toContain('delegatecall'); + }); + + it('reports the 1-based line number of the offending statement', async () => { + const findings = await plugin.analyze( + makeContext(`contract Vault { + function pay(address to) public { + to.call(""); + } +}`), + ); + + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + expect(findings[0]!.lineStart).toBe(3); + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + expect(findings[0]!.lineEnd).toBeGreaterThanOrEqual(findings[0]!.lineStart); + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + expect(findings[0]!.codeSnippet).toContain('.call'); + }); + + it('detects several unchecked calls in one contract', async () => { + const findings = await plugin.analyze( + makeContext(`contract Vault { + function a(address t) public { t.call(""); } + function b(address t) public { t.send(1); } +}`), + ); + + expect(findings).toHaveLength(2); + }); + }); + + describe('ERC-20 detection', () => { + it('detects an unchecked two-argument .transfer()', async () => { + const findings = await plugin.analyze( + makeContext(`contract Vault { + function pay(address token, address to) public { + IERC20(token).transfer(to, 100); + } +}`), + ); + + expect(findings).toHaveLength(1); + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + expect(findings[0]!.severity).toBe(FindingSeverity.MEDIUM); + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + expect(findings[0]!.confidence).toBeLessThan(0.9); + }); + + it('detects unchecked transferFrom() and approve()', async () => { + const findings = await plugin.analyze( + makeContext(`contract Vault { + function sweep(address token) public { + IERC20(token).transferFrom(msg.sender, address(this), 100); + IERC20(token).approve(msg.sender, 100); + } +}`), + ); + + expect(findings).toHaveLength(2); + }); + + it('ignores the native single-argument .transfer() because it reverts on failure', async () => { + const findings = await plugin.analyze( + makeContext(`contract Vault { + function pay(address payable to) public { + to.transfer(1 ether); + } +}`), + ); + + expect(findings).toHaveLength(0); + }); + + it('ignores SafeERC20 helpers', async () => { + const findings = await plugin.analyze( + makeContext(`contract Vault { + function pay(IERC20 token, address to) public { + token.safeTransfer(to, 100); + token.safeTransferFrom(msg.sender, to, 100); + } +}`), + ); + + expect(findings).toHaveLength(0); + }); + }); + + describe('safe patterns that must not be flagged', () => { + it('accepts a call wrapped in require()', async () => { + const findings = await plugin.analyze( + makeContext(`contract Vault { + function pay(address to) public { + require(to.call{value: 1 ether}(""), "call failed"); + } +}`), + ); + + expect(findings).toHaveLength(0); + }); + + it('accepts a send wrapped in an if statement', async () => { + const findings = await plugin.analyze( + makeContext(`contract Vault { + function pay(address to) public { + if (!to.send(1 ether)) revert("send failed"); + } +}`), + ); + + expect(findings).toHaveLength(0); + }); + + it('accepts a tuple assignment validated by require(success)', async () => { + const findings = await plugin.analyze( + makeContext(`contract Vault { + function pay(address to) public { + (bool success, ) = to.call{value: 1 ether}(""); + require(success, "call failed"); + } +}`), + ); + + expect(findings).toHaveLength(0); + }); + + it('accepts a bool assignment validated later', async () => { + const findings = await plugin.analyze( + makeContext(`contract Vault { + function pay(address to) public { + bool sent = to.send(1 ether); + if (!sent) revert("send failed"); + } +}`), + ); + + expect(findings).toHaveLength(0); + }); + + it('accepts a return value propagated to the caller', async () => { + const findings = await plugin.analyze( + makeContext(`contract Vault { + function pay(address to) public returns (bool) { + return to.send(1 ether); + } +}`), + ); + + expect(findings).toHaveLength(0); + }); + + it('accepts ERC-20 transfers wrapped in require()', async () => { + const findings = await plugin.analyze( + makeContext(`contract Vault { + function pay(IERC20 token, address to) public { + require(token.transfer(to, 100), "transfer failed"); + } +}`), + ); + + expect(findings).toHaveLength(0); + }); + }); + + describe('edge cases', () => { + it('returns no findings for an empty contract', async () => { + await expect(plugin.analyze(makeContext(''))).resolves.toEqual([]); + }); + + it('returns no findings for a contract without external calls', async () => { + const findings = await plugin.analyze( + makeContext(`pragma solidity ^0.8.0; +contract Vault { + uint256 public total; + function add(uint256 x) public { total += x; } +}`), + ); + + expect(findings).toHaveLength(0); + }); + + it('ignores calls that only appear in comments', async () => { + const findings = await plugin.analyze( + makeContext(`contract Vault { + // to.call(""); + /* to.send(1 ether); */ + function pay(address to) public { + // to.call(""); + } +}`), + ); + + expect(findings).toHaveLength(0); + }); + + it('ignores call-looking text inside string literals', async () => { + const findings = await plugin.analyze( + makeContext(`contract Vault { + string public note = "to.call(\\"\\")"; +}`), + ); + + expect(findings).toHaveLength(0); + }); + + it('handles a multi-line call statement and reports the first line', async () => { + const findings = await plugin.analyze( + makeContext(`contract Vault { + function pay(address to) public { + to.call{ + value: 1 ether + }(""); + } +}`), + ); + + expect(findings).toHaveLength(1); + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + expect(findings[0]!.lineStart).toBe(3); + }); + + it('does not confuse the member access of a struct with a call', async () => { + const findings = await plugin.analyze( + makeContext(`contract Vault { + struct Config { bool send; uint256 call; } + function read(Config memory c) public pure returns (bool) { return c.send; } +}`), + ); + + expect(findings).toHaveLength(0); + }); + }); + + describe('finding structure', () => { + it('stamps every finding with the plugin id and file path', async () => { + const findings = await plugin.analyze( + makeContext(`contract Vault { + function pay(address to) public { to.call(""); } +}`), + ); + + expect(findings.length).toBeGreaterThan(0); + for (const finding of findings) { + expect(finding.pluginId).toBe('unchecked-return'); + expect(finding.filePath).toBe('Vault.sol'); + expect(finding.references.length).toBeGreaterThan(0); + expect(finding.recommendation.length).toBeGreaterThan(0); + expect(finding.confidence).toBeGreaterThan(0); + expect(finding.confidence).toBeLessThanOrEqual(1); + } + }); + }); + + describe('getFixRecommendation', () => { + it('recommends the require(success) pattern', async () => { + const findings = await plugin.analyze( + makeContext(`contract Vault { + function pay(address to) public { to.call(""); } +}`), + ); + + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + const recommendation = plugin.getFixRecommendation(findings[0]!); + + expect(recommendation).toContain('require(success'); + expect(recommendation).toContain('bool success'); + expect(recommendation).toContain('Vault.sol'); + }); + }); +}); diff --git a/plugins/unchecked-return/src/index.ts b/plugins/unchecked-return/src/index.ts new file mode 100644 index 0000000..1d6b06f --- /dev/null +++ b/plugins/unchecked-return/src/index.ts @@ -0,0 +1,396 @@ +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 (.call/.send/.delegatecall/.staticcall) and ERC-20 transfers whose boolean return value is never checked, allowing failed transfers to fail silently (SWC-104).', + severity: FindingSeverity.HIGH, + category: 'UNCHECKED_RETURN', + chains: ['ethereum', 'polygon', 'bsc', 'avalanche', 'arbitrum', 'optimism'], + languages: ['solidity'], + tags: [ + 'unchecked-return', + 'swc-104', + 'call', + 'send', + 'delegatecall', + 'staticcall', + 'erc20', + 'silent-failure', + ], + author: 'Veridion', + references: [ + 'https://swcregistry.io/docs/SWC-104', + 'https://consensys.github.io/smart-contract-best-practices/development-recommendations/general/external-calls/', + 'https://cwe.mitre.org/data/definitions/252.html', + ], +}; + +type CallKind = 'LOW_LEVEL' | 'ERC20'; + +interface CallSite { + kind: CallKind; + receiver: string; + method: string; + /** Index of the receiver expression inside the comment-stripped source. */ + start: number; + /** Index just past the opening parenthesis of the argument list. */ + argsStart: number; +} + +/** + * Low-level members that report failure through a boolean return value instead + * of reverting. `transfer` is intentionally absent: for native ETH it reverts on + * failure and is therefore safe, while the ERC-20 overload is handled by + * ERC20_PATTERN (disambiguated by argument count). + */ +const LOW_LEVEL_PATTERN = + /\b([A-Za-z_$][\w$]*(?:\s*(?:\.[A-Za-z_$][\w$]*|\([^()]*\)))*)\s*\.\s*(call|callcode|delegatecall|staticcall|send)\s*(\{[^{}]*\})?\s*\(/g; + +const ERC20_PATTERN = + /\b([A-Za-z_$][\w$]*(?:\s*(?:\.[A-Za-z_$][\w$]*|\([^()]*\)))*)\s*\.\s*(transferFrom|approve|transfer)\s*\(/g; + +const MAX_SNIPPET_LENGTH = 200; + +/** + * Blank out comments while preserving every character offset, so line numbers + * keep matching the original source. Code inside comments must never produce a + * finding. + */ +function stripComments(source: string): string { + let out = ''; + let i = 0; + let inLineComment = false; + let inBlockComment = false; + let stringDelimiter: '"' | "'" | null = null; + + while (i < source.length) { + const ch = source.charAt(i); + const next = source.charAt(i + 1); + + if (inLineComment) { + if (ch === '\n') { + inLineComment = false; + out += ch; + } else { + out += ' '; + } + i += 1; + continue; + } + + if (inBlockComment) { + if (ch === '*' && next === '/') { + out += ' '; + i += 2; + inBlockComment = false; + } else { + out += ch === '\n' ? '\n' : ' '; + i += 1; + } + continue; + } + + if (stringDelimiter !== null) { + if (ch === '\\') { + // Blank out the escape and the escaped character while preserving + // newlines so line numbers still match the original source. + out += ' '; + i += 2; + continue; + } + if (ch === stringDelimiter) { + stringDelimiter = null; + out += ch; + i += 1; + continue; + } + out += ch === '\n' ? '\n' : ' '; + i += 1; + continue; + } + + if (ch === '/' && next === '/') { + out += ' '; + i += 2; + inLineComment = true; + continue; + } + + if (ch === '/' && next === '*') { + out += ' '; + i += 2; + inBlockComment = true; + continue; + } + + if (ch === '"' || ch === "'") { + stringDelimiter = ch; + out += ch; + i += 1; + continue; + } + + out += ch; + i += 1; + } + + return out; +} + +function lineNumberOf(source: string, index: number): number { + let line = 1; + for (let i = 0; i < index && i < source.length; i++) { + if (source.charAt(i) === '\n') line += 1; + } + return line; +} + +/** Walk backwards to the closest statement boundary (`;`, `{` or `}`). */ +function findStatementStart(source: string, index: number): number { + for (let i = index - 1; i >= 0; i--) { + const ch = source.charAt(i); + if (ch === ';' || ch === '{' || ch === '}') return i + 1; + } + return 0; +} + +/** + * Walk forwards to the end of the logical statement. Braces and parentheses are + * balanced so that `call{value: x}(...)` is not mistaken for a statement end. + */ +function findStatementEnd(source: string, index: number): number { + let depth = 0; + for (let i = index; i < source.length; i++) { + const ch = source.charAt(i); + if (ch === '(' || ch === '{') { + depth += 1; + } else if (ch === ')' || ch === '}') { + if (depth === 0) return i; + depth -= 1; + } else if (ch === ';' && depth === 0) { + return i + 1; + } + } + return source.length; +} + +/** Count top-level arguments of the call whose opening paren is at `openParen`. */ +function countArguments(source: string, openParen: number): number { + let depth = 0; + let separators = 0; + let hasContent = false; + + for (let i = openParen; i < source.length; i++) { + const ch = source.charAt(i); + if (ch === '(' || ch === '{' || ch === '[') { + depth += 1; + } else if (ch === ')' || ch === '}' || ch === ']') { + depth -= 1; + if (depth === 0) return hasContent ? separators + 1 : 0; + } else if (depth === 1) { + if (ch === ',') separators += 1; + else if (!/\s/.test(ch)) hasContent = true; + } + } + + return hasContent ? separators + 1 : 0; +} + +/** + * True when the boolean produced by the call is consumed by a guard. + * Covers `require(x)`, `assert(x)`, `if (x)`, `if (!x)`, `while (x)` and + * `return x`, where `x` is the call itself. + */ +function isWrappedInGuard(statement: string): boolean { + const trimmed = statement.trimStart(); + return ( + /^(require|assert|if|while|for)\s*\(/.test(trimmed) || + /^return\b/.test(trimmed) || + /^(!|\|\||&&)/.test(trimmed) || + /^(bool|var)\s/.test(trimmed) + ); +} + +/** Variable names introduced by the statement, e.g. `bool ok` or `(bool ok, )`. */ +function collectTargetNames(textBeforeCall: string): string[] { + const names = new Set(); + + const declared = /\bbool\s+([A-Za-z_$][\w$]*)/.exec(textBeforeCall); + if (declared?.[1]) names.add(declared[1]); + + const assigned = /([A-Za-z_$][\w$]*)\s*=(?!=)/.exec(textBeforeCall); + if (assigned?.[1]) names.add(assigned[1]); + + return Array.from(names); +} + +/** True when `name` is later used inside a guard such as `require(success)`. */ +function isNameCheckedLater(source: string, name: string, fromIndex: number): boolean { + const escaped = name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const pattern = new RegExp( + `\\b(?:require|assert|if|while|return)\\s*\\(?\\s*!?\\s*${escaped}\\b`, + 'm', + ); + return pattern.test(source.slice(fromIndex)); +} + +function isChecked( + source: string, + statementStart: number, + statementEnd: number, + callStart: number, +): boolean { + const statement = source.slice(statementStart, statementEnd); + if (isWrappedInGuard(statement)) return true; + + const textBeforeCall = source.slice(statementStart, callStart); + const targets = collectTargetNames(textBeforeCall); + if (targets.some((name) => isNameCheckedLater(source, name, statementEnd))) return true; + + const textAfterCall = source.slice(callStart, statementEnd); + return /\brevert\b/.test(textAfterCall) || /\|\|/.test(textAfterCall); +} + +function collectCallSites(source: string): CallSite[] { + const sites: CallSite[] = []; + + LOW_LEVEL_PATTERN.lastIndex = 0; + let match: RegExpExecArray | null; + while ((match = LOW_LEVEL_PATTERN.exec(source)) !== null) { + const receiver = match[1]; + const method = match[2]; + if (receiver === undefined || method === undefined) continue; + sites.push({ + kind: 'LOW_LEVEL', + receiver, + method, + start: match.index, + argsStart: match.index + match[0].length, + }); + } + + ERC20_PATTERN.lastIndex = 0; + let erc20Match: RegExpExecArray | null; + while ((erc20Match = ERC20_PATTERN.exec(source)) !== null) { + const receiver = erc20Match[1]; + const method = erc20Match[2]; + if (receiver === undefined || method === undefined) continue; + + // `address.transfer(uint256)` is the native ETH variant: it reverts on + // failure, so it carries no unchecked-return risk. ERC-20 `transfer` + // always takes two arguments (recipient, amount). + if ( + method === 'transfer' && + countArguments(source, erc20Match.index + erc20Match[0].length - 1) < 2 + ) { + continue; + } + + sites.push({ + kind: 'ERC20', + receiver, + method, + start: erc20Match.index, + argsStart: erc20Match.index + erc20Match[0].length, + }); + } + + return sites.sort((a, b) => a.start - b.start); +} + +export class UncheckedReturnPlugin implements IRulePlugin { + readonly metadata = metadata; + + async initialize(_config?: Record): Promise { + // No external resources are required; detection is purely source-based. + } + + // eslint-disable-next-line @typescript-eslint/require-await + async analyze(context: AnalysisContext): Promise { + const findings: FindingResult[] = []; + const source = stripComments(context.sourceCode); + if (source.trim().length === 0) return findings; + + for (const site of collectCallSites(source)) { + const statementStart = findStatementStart(source, site.start); + const statementEnd = findStatementEnd(source, site.start); + if (isChecked(source, statementStart, statementEnd, site.start)) continue; + + const isLowLevel = site.kind === 'LOW_LEVEL'; + const lineStart = lineNumberOf(source, site.start); + const lineEnd = Math.max(lineStart, lineNumberOf(source, statementEnd - 1)); + const snippet = source.slice(statementStart, statementEnd).trim(); + + findings.push({ + pluginId: this.metadata.id, + title: isLowLevel + ? `Unchecked return value of \`${site.method}()\` on \`${site.receiver}\`` + : `Unchecked ERC-20 return value of \`${site.method}()\` on \`${site.receiver}\``, + description: isLowLevel + ? `\`${site.receiver}.${site.method}()\` does not revert when the callee fails; it returns \`false\` instead. ` + + 'Because this return value is discarded, a failed transfer or call is silently ignored and the ' + + 'contract continues executing as if it succeeded, which can corrupt accounting or lock funds.' + : `\`${site.receiver}.${site.method}()\` returns a boolean that is not part of the ERC-20 guarantee: ` + + 'many tokens return `false` on failure instead of reverting. Discarding the result means a failed ' + + 'token movement is silently ignored. Non-standard tokens (e.g. USDT) may not return a value at all, ' + + 'so prefer OpenZeppelin SafeERC20.', + severity: isLowLevel ? FindingSeverity.HIGH : FindingSeverity.MEDIUM, + filePath: `${context.contractName}.sol`, + lineStart, + lineEnd, + codeSnippet: snippet.slice(0, MAX_SNIPPET_LENGTH), + recommendation: isLowLevel + ? 'Capture the boolean result and enforce it: ' + + '`(bool success, ) = target.call{value: amount}(""); require(success, "call failed");`. ' + + 'Alternatively use OpenZeppelin `Address.sendValue(target, amount)`, which reverts on failure.' + : 'Use OpenZeppelin SafeERC20 (`safeTransfer`, `safeTransferFrom`, `safeApprove`) which reverts on ' + + 'failure, or check the return value explicitly: `require(token.transfer(to, amount), "transfer failed");`.', + confidence: isLowLevel ? 0.9 : 0.65, + references: this.metadata.references ?? [], + }); + } + + return findings; + } + + getFixRecommendation(finding: FindingResult): string { + return `To fix the unchecked return value at ${finding.filePath}:${finding.lineStart}: + +1. Capture the boolean returned by the external call: + \`\`\`solidity + (bool success, ) = target.call{value: amount}(""); + \`\`\` + +2. Enforce it with \`require(success)\` before continuing: + \`\`\`solidity + require(success, "transfer failed"); + \`\`\` + +3. For ERC-20 tokens prefer OpenZeppelin SafeERC20, which reverts on failure and + also supports tokens that return no value: + \`\`\`solidity + import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; + + using SafeERC20 for IERC20; + token.safeTransfer(to, amount); + \`\`\` + +${finding.recommendation}`; + } + + supportsContext(context: AnalysisContext): boolean { + return ( + this.metadata.chains.includes(context.chain) && + this.metadata.languages.includes(context.language) + ); + } +} 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..572fe3a 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -556,6 +556,9 @@ importers: '@veridion/logger': specifier: workspace:* version: link:../logger + '@veridion/plugin-unchecked-return': + specifier: workspace:* + version: link:../../plugins/unchecked-return '@veridion/scanner-types': specifier: workspace:* version: link:../scanner-types @@ -854,6 +857,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':