feat(detectors): Add stringReceiversOverlap

Naive implementation, no dataflow information is used

Towards #121

src/detectors/builtin/stringReceiversOverlap.ts

@@ -0,0 +1,145 @@
+import { Detector } from "../detector";
+import { CompilationUnit } from "../../internals/ir";
+import { MistiTactWarning, Severity } from "../../internals/warnings";
+import {
+  forEachExpression,
+  forEachStatement,
+} from "../../internals/tactASTUtil";
+import {
+  AstExpression,
+  AstReceiver,
+} from "@tact-lang/compiler/dist/grammar/ast";
+
+/**
+ * A detector that finds overlapping messages between general string receivers and string receivers.
+ *
+ * ## Why is it bad?
+ *  Constant string receivers and general string receivers can have overlapping messages
+ *  in which case the constant string receiver always takes precedence.
+ *
+ * ## Example
+ * ```tact
+ * contract Test {
+ *   receive("foobar") { throw(1042) }
+ *   receive(msg: String) {
+ *     if (msg == "foobar") { throw(1043)  } // Bad: Dead code
+ *   }
+ * }
+ * ```
+ *
+ * Use instead:
+ * ```tact
+ * contract Test {
+ *   receive("foobar") { throw(1042) }
+ *   receive(msg: String) {}
+ * }
+ * ```
+ */
+export class StringReceiversOverlap extends Detector {
+  async check(cu: CompilationUnit): Promise<MistiTactWarning[]> {
+    const stringReceivers = this.getStringReceiverNames(cu);
+    return Array.from(cu.ast.getFunctions()).reduce((warnings, node) => {
+      if (node.kind === "receiver") {
+        const arg = this.findGenericReceiverArg(node);
+        if (arg !== undefined) {
+          return warnings.concat(
+            this.checkConditions(node, arg, stringReceivers),
+          );
+        }
+      }
+      return warnings;
+    }, [] as MistiTactWarning[]);
+  }
+
+  /**
+   * Checks violations of the detector rules in the body of generic string receiver.
+   * @param receiver Generic string receiver
+   * @param argName Name of the argument that overlaps with one of the string receivers
+   */
+  private checkConditions(
+    receiver: AstReceiver,
+    argName: string,
+    stringReceivers: Set<string>,
+  ): MistiTactWarning[] {
+    const warnings: MistiTactWarning[] = [];
+    forEachStatement(receiver, (stmt) => {
+      // Conditional statements
+      if (stmt.kind === "statement_condition") {
+        this.checkCondition(warnings, stmt.condition, argName, stringReceivers);
+      }
+    });
+    forEachExpression(receiver, (expr) => {
+      // Ternary conditions
+      if (expr.kind === "conditional") {
+        this.checkCondition(warnings, expr.condition, argName, stringReceivers);
+      }
+    });
+    return warnings;
+  }
+
+  /**
+   * Adds a warning to `warnings` if `condition` contains a comparison operation
+   * involving the overlapping arg.
+   */
+  private checkCondition(
+    warnings: MistiTactWarning[],
+    condition: AstExpression,
+    argName: string,
+    stringReceivers: Set<string>,
+  ): void {
+    const isArg = (expr: AstExpression) =>
+      expr.kind === "id" && expr.text === argName;
+    const isOverlappingStringLiteral = (expr: AstExpression) =>
+      expr.kind === "string" && stringReceivers.has(expr.value);
+    const isOverlappingComparison = (lhs: AstExpression, rhs: AstExpression) =>
+      isArg(lhs) && isOverlappingStringLiteral(rhs);
+    // Iterate recursively to find cases like `(msg === "overlap") && whatever_else`
+    forEachExpression(condition, (expr) => {
+      if (
+        expr.kind === "op_binary" &&
+        ["==", "!="].includes(expr.op) &&
+        (isOverlappingComparison(expr.left, expr.right) ||
+          isOverlappingComparison(expr.right, expr.left))
+      ) {
+        const receiverName = `receiver("${argName}")`;
+        const warn = this.makeWarning(
+          "String Receivers Overlap",
+          Severity.HIGH,
+          condition.loc,
+          {
+            extraDescription: [
+              `${receiverName} might be called instead.`,
+              `This condition might never be executed.`,
+            ].join(" "),
+            suggestion: `Implement the desired logic in ${receiverName} and remove ${expr.loc.contents}`,
+          },
+        );
+        warnings.push(warn);
+      }
+    });
+  }
+
+  /**
+   * Returns the name of the argument if the given receiver is a generic string
+   * receiver: `receive(arg: String)`.
+   */
+  private findGenericReceiverArg(receiver: AstReceiver): string | undefined {
+    return receiver.selector.kind === "internal-simple" &&
+      receiver.selector.param.type.kind === "type_id" &&
+      receiver.selector.param.type.text === "String"
+      ? receiver.selector.param.name.text
+      : undefined;
+  }
+
+  private getStringReceiverNames(cu: CompilationUnit): Set<string> {
+    return Array.from(cu.ast.getFunctions()).reduce((acc, node) => {
+      if (
+        node.kind === "receiver" &&
+        node.selector.kind === "internal-comment"
+      ) {
+        acc.add(node.selector.comment.value);
+      }
+      return acc;
+    }, new Set<string>());
+  }
+}

src/detectors/detector.ts

@@ -228,6 +228,13 @@ const BuiltInDetectors: Record<string, DetectorEntry> = {
       ),
     enabledByDefault: true,
   },
+  StringReceiversOverlap: {
+    loader: (ctx: MistiContext) =>
+      import("./builtin/stringReceiversOverlap").then(
+        (module) => new module.StringReceiversOverlap(ctx),
+      ),
+    enabledByDefault: true,
+  },
 };
 
 /**

test/good/string-receivers-overlap-1.cfg.dot

@@ -0,0 +1,20 @@
+digraph "string-receivers-overlap-1" {
+    node [shape=box];
+    subgraph "cluster_Test__receive_internal_comment_1532_test" {
+        label="Test__receive_internal_comment_1532_test";
+    }
+    subgraph "cluster_Test__receive_internal_simple_1566" {
+        label="Test__receive_internal_simple_1566";
+        "Test__receive_internal_simple_1566_142" [label="if (msg == \"test\")"];
+        "Test__receive_internal_simple_1566_143" [label="if (\"test\" != msg)"];
+        "Test__receive_internal_simple_1566_145" [label="if (\"test\" != msg && WHATEVER)"];
+        "Test__receive_internal_simple_1566_147" [label="if ((WHATEVER || WHATEVER) && \"test\" != msg)"];
+        "Test__receive_internal_simple_1566_149" [label="let a: String = \"foo\""];
+        "Test__receive_internal_simple_1566_151" [label="if (\"test\" != a)",style=filled,fillcolor="#66A7DB"];
+        "Test__receive_internal_simple_1566_142" -> "Test__receive_internal_simple_1566_143";
+        "Test__receive_internal_simple_1566_143" -> "Test__receive_internal_simple_1566_145";
+        "Test__receive_internal_simple_1566_145" -> "Test__receive_internal_simple_1566_147";
+        "Test__receive_internal_simple_1566_147" -> "Test__receive_internal_simple_1566_149";
+        "Test__receive_internal_simple_1566_149" -> "Test__receive_internal_simple_1566_151";
+    }
+}

test/good/string-receivers-overlap-1.cfg.json

@@ -0,0 +1,108 @@
+{
+  "projectName": "string-receivers-overlap-1",
+  "functions": [],
+  "contracts": [
+    {
+      "name": "Test",
+      "methods": [
+        {
+          "name": "Test.receive_internal_comment_1532_test",
+          "cfg": {
+            "nodes": [],
+            "edges": []
+          }
+        },
+        {
+          "name": "Test.receive_internal_simple_1566",
+          "cfg": {
+            "nodes": [
+              {
+                "id": 142,
+                "stmtID": 1539,
+                "srcEdges": [],
+                "dstEdges": [
+                  144
+                ]
+              },
+              {
+                "id": 143,
+                "stmtID": 1543,
+                "srcEdges": [
+                  144
+                ],
+                "dstEdges": [
+                  146
+                ]
+              },
+              {
+                "id": 145,
+                "stmtID": 1549,
+                "srcEdges": [
+                  146
+                ],
+                "dstEdges": [
+                  148
+                ]
+              },
+              {
+                "id": 147,
+                "stmtID": 1557,
+                "srcEdges": [
+                  148
+                ],
+                "dstEdges": [
+                  150
+                ]
+              },
+              {
+                "id": 149,
+                "stmtID": 1561,
+                "srcEdges": [
+                  150
+                ],
+                "dstEdges": [
+                  152
+                ]
+              },
+              {
+                "id": 151,
+                "stmtID": 1565,
+                "srcEdges": [
+                  152
+                ],
+                "dstEdges": []
+              }
+            ],
+            "edges": [
+              {
+                "id": 144,
+                "src": 142,
+                "dst": 143
+              },
+              {
+                "id": 146,
+                "src": 143,
+                "dst": 145
+              },
+              {
+                "id": 148,
+                "src": 145,
+                "dst": 147
+              },
+              {
+                "id": 150,
+                "src": 147,
+                "dst": 149
+              },
+              {
+                "id": 152,
+                "src": 149,
+                "dst": 151
+              }
+            ]
+          }
+        }
+      ]
+    }
+  ]
+}

test/good/string-receivers-overlap-1.expected.out

@@ -0,0 +1,44 @@
+test/good/string-receivers-overlap-1.tact:6:13:
+  5 |     receive(msg: String) {
+> 6 |         if (msg == "test") {} // Bad
+                  ^
+  7 |         if ("test" != msg) {} // Bad
+String Receivers Overlap: receiver("msg") might be called instead. This condition might never be executed.
+Help: Implement the desired logic in receiver("msg") and remove msg == "test"
+See: https://nowarp.github.io/tools/misti/docs/detectors/StringReceiversOverlap
+
+test/good/string-receivers-overlap-1.tact:7:13:
+  6 |         if (msg == "test") {} // Bad
+> 7 |         if ("test" != msg) {} // Bad
+                  ^
+  8 |         if (("test" != msg) && WHATEVER) {} // Bad
+String Receivers Overlap: receiver("msg") might be called instead. This condition might never be executed.
+Help: Implement the desired logic in receiver("msg") and remove "test" != msg
+See: https://nowarp.github.io/tools/misti/docs/detectors/StringReceiversOverlap
+
+test/good/string-receivers-overlap-1.tact:8:13:
+  7 |         if ("test" != msg) {} // Bad
+> 8 |         if (("test" != msg) && WHATEVER) {} // Bad
+                  ^
+  9 |         if ((WHATEVER || WHATEVER) && ("test" != msg)) {} // Bad
+String Receivers Overlap: receiver("msg") might be called instead. This condition might never be executed.
+Help: Implement the desired logic in receiver("msg") and remove "test" != msg
+See: https://nowarp.github.io/tools/misti/docs/detectors/StringReceiversOverlap
+
+test/good/string-receivers-overlap-1.tact:9:13:
+   8 |         if (("test" != msg) && WHATEVER) {} // Bad
+>  9 |         if ((WHATEVER || WHATEVER) && ("test" != msg)) {} // Bad
+                   ^
+  10 | 
+String Receivers Overlap: receiver("msg") might be called instead. This condition might never be executed.
+Help: Implement the desired logic in receiver("msg") and remove "test" != msg
+See: https://nowarp.github.io/tools/misti/docs/detectors/StringReceiversOverlap
+
+test/good/string-receivers-overlap-1.tact:13:23:
+  12 |         let a: String = "foo";
+> 13 |         if ("test" != a) {}
+                             ^
+  14 |     }
+Read-only variable
+Help: Consider creating a constant instead
+See: https://nowarp.github.io/tools/misti/docs/detectors/ReadOnlyVariables

test/good/string-receivers-overlap-1.tact

@@ -0,0 +1,15 @@
+const WHATEVER: Bool = true;
+
+contract Test {
+    receive("test") {}
+    receive(msg: String) {
+        if (msg == "test") {} // Bad
+        if ("test" != msg) {} // Bad
+        if (("test" != msg) && WHATEVER) {} // Bad
+        if ((WHATEVER || WHATEVER) && ("test" != msg)) {} // Bad
+
+        // No false positives
+        let a: String = "foo";
+        if ("test" != a) {}
+    }
+}