Suspicious Loop Detector

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
 

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"

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<MistiTactWarning[]> {
+    const processedLoopIds = new Set<number>();
+    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<number>,
+  ): 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"
+    );
+  }
+}

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<string, DetectorEntry> = {
+  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<string, DetectorEntry> = {
       ),
     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<string, DetectorEntry> = {
       ),
     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,
+  },
 };
 
 /**

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.`,

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[],

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,

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

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

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;
+            }
+        }
+    }
+}