diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a67ca7..fd10690 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.6.2] - 2024-12-25 + ### Fixed - Callgraph: Don't add state write effects when changing local maps/strings/cells +- Regression in the single-contract mode execution: Issue [#233](https://github.com/nowarp/misti/issues/233) ## [0.6.1] - 2024-12-22 diff --git a/package.json b/package.json index 29dc5f4..0dcfa0b 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@nowarp/misti", - "version": "0.6.1", + "version": "0.6.2", "repository": { "type": "git", "url": "git+https://github.com/nowarp/misti.git" diff --git a/src/detectors/builtin/suspiciousLoop.ts b/src/detectors/builtin/suspiciousLoop.ts new file mode 100644 index 0000000..05262c4 --- /dev/null +++ b/src/detectors/builtin/suspiciousLoop.ts @@ -0,0 +1,132 @@ +import { CompilationUnit } from "../../internals/ir"; +import { + foldStatements, + foldExpressions, + evalExpr, +} from "../../internals/tact"; +import { MistiTactWarning, Severity } from "../../internals/warnings"; +import { AstDetector } from "../detector"; +import { + AstStatement, + AstExpression, +} from "@tact-lang/compiler/dist/grammar/ast"; + +/** + * SuspiciousLoop Detector + * + * An optional detector that identifies potentially problematic loops, such as those + * with unbounded conditions or excessive iteration counts. + * + * ## Why is it bad? + * Loops with always-true conditions or massive iteration limits can lead to high + * gas consumption and even denial of service (DoS) issues. By flagging these loops, + * this detector aids auditors in catching potential performance or security risks. + * + * ## Example + * ```tact + * repeat (1_000_001) { // Highlighted by detector as high iteration count + * // ... + * } + * + * while (true) { // Highlighted as unbounded condition + * // ... + * } + * ``` + */ +export class SuspiciousLoop extends AstDetector { + severity = Severity.HIGH; + + async check(cu: CompilationUnit): Promise { + const processedLoopIds = new Set(); + return Array.from(cu.ast.getProgramEntries()).reduce((acc, node) => { + return acc.concat( + ...foldStatements( + node, + (acc, stmt) => { + return acc.concat( + this.analyzeLoopStatement(stmt, processedLoopIds), + ); + }, + acc, + { flatStmts: true }, + ), + ); + }, [] as MistiTactWarning[]); + } + + /** + * Analyzes a loop statement to determine if it contains a suspicious condition. + * @param stmt - The statement to analyze. + * @param processedLoopIds - A set of loop IDs already processed. + * @returns An array of MistiTactWarning objects if a suspicious loop is detected. + */ + private analyzeLoopStatement( + stmt: AstStatement, + processedLoopIds: Set, + ): MistiTactWarning[] { + if (processedLoopIds.has(stmt.id)) { + return []; + } + if (this.isLoop(stmt)) { + processedLoopIds.add(stmt.id); + return foldExpressions( + stmt, + (acc, expr) => { + if (this.isSuspiciousCondition(expr)) { + acc.push( + this.makeWarning( + "Potential unbounded or high-cost loop", + stmt.loc, + { + suggestion: + "Avoid excessive iterations or unbounded conditions in loops", + }, + ), + ); + } + return acc; + }, + [] as MistiTactWarning[], + ); + } + return []; + } + + /** + * Checks if an expression is a suspicious condition, indicating an unbounded + * loop or excessive iteration. + * @param expr - The expression to evaluate. + * @returns True if the expression is suspicious, otherwise false. + */ + + private isSuspiciousCondition(expr: AstExpression): boolean { + // Try evaluating the expression as a constant. + const result = evalExpr(expr); + if (result !== undefined) { + if (typeof result === "boolean" && result === true) { + // Unbounded loop detected + return true; + } + // Check if the result is a large constant for repeat counts. + const threshold = 100_000; + if (typeof result === "bigint" && result > BigInt(threshold)) { + return true; + } + } + return false; + } + + /** + * Determines if a statement is a loop. + * @param stmt - The statement to evaluate. + * @returns True if the statement represents a loop, otherwise false. + */ + private isLoop(stmt: AstStatement): boolean { + return ( + stmt.kind === "statement_while" || + stmt.kind === "statement_repeat" || + stmt.kind === "statement_until" || + stmt.kind === "statement_foreach" + ); + } +} diff --git a/src/detectors/detector.ts b/src/detectors/detector.ts index 572ea85..b16207a 100644 --- a/src/detectors/detector.ts +++ b/src/detectors/detector.ts @@ -205,6 +205,13 @@ interface DetectorEntry { * A mapping of detector names to their respective loader functions and default enablement status. */ export const BuiltInDetectors: Record = { + NeverAccessedVariables: { + loader: (ctx: MistiContext) => + import("./builtin/neverAccessedVariables").then( + (module) => new module.NeverAccessedVariables(ctx), + ), + enabledByDefault: true, + }, DivideBeforeMultiply: { loader: (ctx: MistiContext) => import("./builtin/divideBeforeMultiply").then( @@ -219,13 +226,6 @@ export const BuiltInDetectors: Record = { ), enabledByDefault: true, }, - NeverAccessedVariables: { - loader: (ctx: MistiContext) => - import("./builtin/neverAccessedVariables").then( - (module) => new module.NeverAccessedVariables(ctx), - ), - enabledByDefault: true, - }, UnboundLoop: { loader: (ctx: MistiContext) => import("./builtin/unboundLoop").then( @@ -399,6 +399,20 @@ export const BuiltInDetectors: Record = { ), enabledByDefault: true, }, + SuspiciousLoop: { + loader: (ctx: MistiContext) => + import("./builtin/suspiciousLoop").then( + (module) => new module.SuspiciousLoop(ctx), + ), + enabledByDefault: false, + }, + SuspiciousLoop: { + loader: (ctx: MistiContext) => + import("./builtin/suspiciousLoop").then( + (module) => new module.SuspiciousLoop(ctx), + ), + enabledByDefault: false, + }, }; /** diff --git a/src/internals/ir/callGraph.ts b/src/internals/ir/callGraph.ts index b83653c..a9e14de 100644 --- a/src/internals/ir/callGraph.ts +++ b/src/internals/ir/callGraph.ts @@ -102,6 +102,10 @@ class CGNode { return (this.effects & effect) !== 0; } + public hasAnyEffect(...effects: Effect[]): boolean { + return effects.some((effect) => this.hasEffect(effect)); + } + /** * Pretty-prints a signature of the function is available */ @@ -161,11 +165,11 @@ export class CallGraph { } /** - * Retrieves a node's ID by its AST ID. - * @param astId The AST ID of the function. + * Retrieves a node's ID by the AST ID of its definition. + * @param astId The AST ID of the function definition. * @returns The corresponding node ID, or `undefined` if not found. */ - public getNodeIdByAstId(astId: number): CGNodeId | undefined { + public getNodeIdByAstId(astId: AstNode["id"]): CGNodeId | undefined { return this.astIdToNodeId.get(astId); } @@ -267,9 +271,7 @@ export class CallGraph { const node = new CGNode(func.id, funcName, this.logger); this.nodeMap.set(node.idx, node); this.nameToNodeId.set(funcName, node.idx); - if (func.id !== undefined) { - this.astIdToNodeId.set(func.id, node.idx); - } + this.astIdToNodeId.set(func.id, node.idx); } else { this.logger.error( `Function with id ${func.id} has no name and will be skipped.`, diff --git a/src/internals/ir/cfg.ts b/src/internals/ir/cfg.ts index d2e52f5..486a981 100644 --- a/src/internals/ir/cfg.ts +++ b/src/internals/ir/cfg.ts @@ -8,7 +8,11 @@ import { AstStore } from "./astStore"; import { IdxGenerator } from "./indices"; import { FunctionName } from "./types"; import { InternalException } from "../exceptions"; -import { AstStatement, SrcInfo } from "@tact-lang/compiler/dist/grammar/ast"; +import { + AstNode, + AstStatement, + SrcInfo, +} from "@tact-lang/compiler/dist/grammar/ast"; import { ItemOrigin } from "@tact-lang/compiler/dist/grammar/grammar"; export type EdgeIdx = number & { readonly __brand: unique symbol }; @@ -118,7 +122,7 @@ export class Cfg { */ constructor( public name: FunctionName, - public id: number, + public id: AstNode["id"], public kind: FunctionKind, public origin: ItemOrigin, public nodes: BasicBlock[], diff --git a/src/internals/tact/config.ts b/src/internals/tact/config.ts index 87de8c8..2630132 100644 --- a/src/internals/tact/config.ts +++ b/src/internals/tact/config.ts @@ -57,14 +57,22 @@ export class TactConfigManager { ) as ProjectName, vfs: VirtualFileSystem, ): TactConfigManager { - const absoluteProjectRoot = vfs.resolve(projectRoot); - const absoluteContractPath = path.resolve(projectRoot, contractPath); - + let vfsContractPath: string = ""; + if (vfs.type === "local") { + vfsContractPath = path.relative(projectRoot, contractPath); + } else { + const absoluteProjectRoot = vfs.resolve(projectRoot); + const absoluteContractPath = path.resolve(projectRoot, contractPath); + vfsContractPath = path.relative( + absoluteProjectRoot, + absoluteContractPath, + ); + } const tactConfig: TactConfig = { projects: [ { name: projectName, - path: path.relative(absoluteProjectRoot, absoluteContractPath), + path: vfsContractPath, output: "/tmp/misti/output", // never used options: { debug: false, diff --git a/test/detectors/NeverAccessedVariables.tact b/test/detectors/NeverAccessedVariables.tact index cb1e1a4..e908dcc 100644 --- a/test/detectors/NeverAccessedVariables.tact +++ b/test/detectors/NeverAccessedVariables.tact @@ -5,10 +5,10 @@ fun test1(): Int { } return 42; } - -fun test2() { - while(true) { let a: Int = 0; } -} +// suppress SuspiciousLoop +//fun test2() { +// while(true) { let a: Int = 0; } +//} fun test3(): Int { let a: Int = 20; // read-only variable diff --git a/test/detectors/SuspiciousLoop.expected.out b/test/detectors/SuspiciousLoop.expected.out new file mode 100644 index 0000000..5beb003 --- /dev/null +++ b/test/detectors/SuspiciousLoop.expected.out @@ -0,0 +1,35 @@ +[HIGH] SuspiciousLoop: Potential unbounded or high-cost loop +test/detectors/SuspiciousLoop.tact:5:9: + 4 | let i: Int = 0; +> 5 | while (true) { // This should be flagged for an unbounded condition + ^ + 6 | i = i + 1; +Help: Avoid excessive iterations or unbounded conditions in loops +See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousLoop + +[HIGH] SuspiciousLoop: Potential unbounded or high-cost loop +test/detectors/SuspiciousLoop.tact:11:9: + 10 | fun testRepeatHighCount() { +> 11 | repeat (1_000_001) { // This should be flagged for excessive iteration + ^ + 12 | let x = 1; +Help: Avoid excessive iterations or unbounded conditions in loops +See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousLoop + +[HIGH] SuspiciousLoop: Potential unbounded or high-cost loop +test/detectors/SuspiciousLoop.tact:18:9: + 17 | let i: Int = 0; +> 18 | while (i < 1_000_000) { // This should be flagged for large constant comparison + ^ + 19 | i = i + 1; +Help: Avoid excessive iterations or unbounded conditions in loops +See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousLoop + +[HIGH] SuspiciousLoop: Potential unbounded or high-cost loop +test/detectors/SuspiciousLoop.tact:25:9: + 24 | let i: Int = 0; +> 25 | while (i < 10) { + ^ + 26 | repeat (1_000_000) { // Should be flagged within nested loop +Help: Avoid excessive iterations or unbounded conditions in loops +See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousLoop \ No newline at end of file diff --git a/test/detectors/SuspiciousLoop.tact b/test/detectors/SuspiciousLoop.tact new file mode 100644 index 0000000..888c50f --- /dev/null +++ b/test/detectors/SuspiciousLoop.tact @@ -0,0 +1,31 @@ +contract TestSuspiciousLoops { + + fun testWhileInfinite() { + let i: Int = 0; + while (true) { // This should be flagged for an unbounded condition + i = i + 1; + } + } + + fun testRepeatHighCount() { + repeat (1_000_001) { // This should be flagged for excessive iteration + let x = 1; + } + } + + fun testWhileLargeLimit() { + let i: Int = 0; + while (i < 1_000_000) { // This should be flagged for large constant comparison + i = i + 1; + } + } + + fun testNestedLoops() { + let i: Int = 0; + while (i < 10) { + repeat (1_000_000) { // Should be flagged within nested loop + i = i + 1; + } + } + } +}