Add SuspiciousMessageMode detector

CHANGELOG.md

@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 
 ### Added
+- `SuspiciousMessageMode` detector: PR [#193](https://github.com/nowarp/misti/pull/193)
 - `SendInLoop` detector: PR [#168](https://github.com/nowarp/misti/pull/168)
 - `CellOverflow` detector: PR [#177](https://github.com/nowarp/misti/pull/177)
 - `UnboundMap` detector: Issue [#50](https://github.com/nowarp/misti/issues/50)

README.md

@@ -2,7 +2,7 @@
 Misti is a static analysis tool designed for smart contracts on the [TON blockchain](https://ton.org/).
 
 ### Language Support
-- [Tact](https://tact-lang.org/): 24 detectors [are available](https://nowarp.io/tools/misti/docs/next/detectors)
+- [Tact](https://tact-lang.org/): 25 detectors [are available](https://nowarp.io/tools/misti/docs/next/detectors)
 - [FunC](https://docs.ton.org/develop/func/overview) support is [planned](https://github.com/nowarp/misti/issues/56) by the end of the year
 
 ### Use Cases

src/detectors/builtin/suspiciousMessageMode.ts

@@ -0,0 +1,126 @@
+import { CompilationUnit } from "../../internals/ir";
+import { forEachExpression } from "../../internals/tact";
+import { MistiTactWarning, Severity } from "../../internals/warnings";
+import { ASTDetector } from "../detector";
+import {
+  AstExpression,
+  AstStructInstance,
+  idText,
+  AstOpBinary,
+} from "@tact-lang/compiler/dist/grammar/ast";
+
+/**
+ * Detects suspicious usage of the `mode` field in `SendParameters` struct instances.
+ *
+ * ## Why is it bad?
+ * Incorrect usage of the `mode` field in `SendParameters` can lead to unintended behavior when sending messages,
+ * such as incorrect flags being set, which can cause security vulnerabilities or unexpected contract behavior.
+ *
+ * **What it checks:**
+ * - Ensures that the `mode` expression only uses the bitwise OR operator `|`.
+ * - Warns if integer literals are used instead of symbolic constants.
+ * - Warns if the same flag is used multiple times in the `mode` expression.
+ *
+ * ## Example
+ *
+ * ```tact
+ * // Suspicious usage:
+ * send(SendParameters{
+ *     to: recipient,
+ *     value: amount,
+ *     mode: SendRemainingBalance | SendRemainingBalance // Bad: Duplicate flag
+ * });
+ *
+ * // Correct usage:
+ * send(SendParameters{
+ *     to: recipient,
+ *     value: amount,
+ *     mode: SendRemainingBalance | SendDestroyIfZero // Ok
+ * });
+ * ```
+ */
+export class SuspiciousMessageMode extends ASTDetector {
+  severity = Severity.MEDIUM;
+
+  async check(cu: CompilationUnit): Promise<MistiTactWarning[]> {
+    const warnings: MistiTactWarning[] = [];
+    Array.from(cu.ast.getProgramEntries()).forEach((node) => {
+      forEachExpression(node, (expr) => {
+        if (
+          expr.kind === "struct_instance" &&
+          idText(expr.type) === "SendParameters"
+        ) {
+          this.checkSendParameters(expr, warnings);
+        }
+      });
+    });
+    return warnings;
+  }
+
+  private checkSendParameters(
+    expr: AstStructInstance,
+    warnings: MistiTactWarning[],
+  ): void {
+    const args = expr.args;
+    const modeField = args.find((arg) => idText(arg.field) === "mode");
+    if (modeField) {
+      this.checkModeExpression(modeField.initializer, warnings);
+    }
+  }
+
+  private checkModeExpression(
+    expr: AstExpression,
+    warnings: MistiTactWarning[],
+  ): void {
+    const flagsUsed = new Set<string>();
+    forEachExpression(expr, (e) => {
+      switch (e.kind) {
+        case "op_binary":
+          const opBinary = e as AstOpBinary;
+          if (opBinary.op !== "|") {
+            warnings.push(
+              this.makeWarning(
+                "Mode expression should only contain the '|' operator",
+                e.loc,
+                {
+                  suggestion:
+                    "Use the '|' operator (bitwise OR) to combine flags",
+                },
+              ),
+            );
+          }
+          break;
+        case "id":
+          const flagName = idText(e);
+          if (flagsUsed.has(flagName)) {
+            warnings.push(
+              this.makeWarning(
+                `Flag \`${flagName}\` is used multiple times in the \`mode\` expression`,
+                e.loc,
+                {
+                  suggestion:
+                    "Use each flag at most once in the mode expression",
+                },
+              ),
+            );
+          }
+          flagsUsed.add(flagName);
+          break;
+        case "number":
+          warnings.push(
+            this.makeWarning(
+              "Integer literals should not be used in mode expression; use symbolic constants instead",
+              e.loc,
+              {
+                suggestion:
+                  "Replace integer literals with symbolic flag constants",
+              },
+            ),
+          );
+          break;
+        default:
+          break;
+      }
+    });
+  }
+}

src/detectors/detector.ts

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

test/detectors/SuspiciousMessageMode.expected.out

@@ -0,0 +1,71 @@
+[MEDIUM] SuspiciousMessageMode: Flag `SendRemainingValue` is used multiple times in the `mode` expression
+test/detectors/SuspiciousMessageMode.tact:16:40:
+  15 |             value: 0,
+> 16 |             mode: SendRemainingValue | SendRemainingValue // Bad
+                                              ^
+  17 |         });
+Help: Use each flag at most once in the mode expression
+See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousMessageMode
+
+[MEDIUM] SuspiciousMessageMode: Mode expression should only contain the '|' operator
+test/detectors/SuspiciousMessageMode.tact:25:19:
+  24 |             value: 0,
+> 25 |             mode: SendRemainingValue + SendIgnoreErrors // Bad
+                         ^
+  26 |         });
+Help: Use the '|' operator (bitwise OR) to combine flags
+See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousMessageMode
+
+[MEDIUM] SuspiciousMessageMode: Integer literals should not be used in mode expression; use symbolic constants instead
+test/detectors/SuspiciousMessageMode.tact:34:19:
+  33 |             value: 0,
+> 34 |             mode: 64 // Bad: Integer literal instead of symbolic constant 
+                         ^
+  35 |         });
+Help: Replace integer literals with symbolic flag constants
+See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousMessageMode
+
+[MEDIUM] SuspiciousMessageMode: Mode expression should only contain the '|' operator
+test/detectors/SuspiciousMessageMode.tact:52:19:
+  51 |             value: 0,
+> 52 |             mode: SendRemainingValue + SendRemainingValue + 64 // Bad: Duplicate flags, '+' operator, integer literal 
+                         ^
+  53 |         });
+Help: Use the '|' operator (bitwise OR) to combine flags
+See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousMessageMode
+
+[MEDIUM] SuspiciousMessageMode: Flag `SendRemainingValue` is used multiple times in the `mode` expression
+test/detectors/SuspiciousMessageMode.tact:52:40:
+  51 |             value: 0,
+> 52 |             mode: SendRemainingValue + SendRemainingValue + 64 // Bad: Duplicate flags, '+' operator, integer literal 
+                                              ^
+  53 |         });
+Help: Use each flag at most once in the mode expression
+See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousMessageMode
+
+[MEDIUM] SuspiciousMessageMode: Integer literals should not be used in mode expression; use symbolic constants instead
+test/detectors/SuspiciousMessageMode.tact:52:61:
+  51 |             value: 0,
+> 52 |             mode: SendRemainingValue + SendRemainingValue + 64 // Bad: Duplicate flags, '+' operator, integer literal 
+                                                                   ^
+  53 |         });
+Help: Replace integer literals with symbolic flag constants
+See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousMessageMode
+
+[MEDIUM] SuspiciousMessageMode: Mode expression should only contain the '|' operator
+test/detectors/SuspiciousMessageMode.tact:61:19:
+  60 |             value: 0,
+> 61 |             mode: -(SendRemainingValue * SendPayGasSeparately) - SendIgnoreErrors // Bad
+                         ^
+  62 |         });
+Help: Use the '|' operator (bitwise OR) to combine flags
+See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousMessageMode
+
+[MEDIUM] SuspiciousMessageMode: Mode expression should only contain the '|' operator
+test/detectors/SuspiciousMessageMode.tact:61:21:
+  60 |             value: 0,
+> 61 |             mode: -(SendRemainingValue * SendPayGasSeparately) - SendIgnoreErrors // Bad
+                           ^
+  62 |         });
+Help: Use the '|' operator (bitwise OR) to combine flags
+See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousMessageMode

test/detectors/SuspiciousMessageMode.tact

@@ -0,0 +1,74 @@
+contract SendParametersTestContract {
+    // Correct usage: should not trigger any warnings
+    fun correctUsage() {
+        send(SendParameters{
+            to: sender(),
+            value: 0,
+            mode: SendRemainingValue | SendPayGasSeparately // Ok
+        });
+    }
+
+    // Duplicate flag usage: should trigger a warning about flags used multiple times
+    fun duplicateFlagUsage() {
+        send(SendParameters{
+            to: sender(),
+            value: 0,
+            mode: SendRemainingValue | SendRemainingValue // Bad
+        });
+    }
+
+    // Invalid operator usage: should trigger a warning about using '+' instead of '|'
+    fun invalidOperatorUsage() {
+        send(SendParameters{
+            to: sender(),
+            value: 0,
+            mode: SendRemainingValue + SendIgnoreErrors // Bad
+        });
+    }
+
+    // Integer literal usage: should trigger a warning about using integer literals
+    fun integerLiteralUsage() {
+        send(SendParameters{
+            to: sender(),
+            value: 0,
+            mode: 64 // Bad: Integer literal instead of symbolic constant 
+        });
+    }
+
+    // Correct combination usage: should not trigger any warnings
+    fun correctCombinationUsage() {
+        send(SendParameters{
+            to: sender(),
+            value: 0,
+            mode: SendRemainingBalance | SendDestroyIfZero | SendIgnoreErrors // Ok
+        });
+    }
+
+    // Multiple issues: should trigger multiple warnings
+    fun multipleIssues() {
+        send(SendParameters{
+            to: sender(),
+            value: 0,
+            mode: SendRemainingValue + SendRemainingValue + 64 // Bad: Duplicate flags, '+' operator, integer literal 
+        });
+    }
+
+    // Complex expression with nested invalid operators and unary operation
+    fun complexInvalidOperator() {
+        send(SendParameters{
+            to: sender(),
+            value: 0,
+            mode: -(SendRemainingValue * SendPayGasSeparately) - SendIgnoreErrors // Bad
+        });
+    }
+
+    // Correct usage with variables
+    fun correctUsageWithVariables() {
+        let modeFlag: Int = SendRemainingValue | SendIgnoreErrors;
+        send(SendParameters{
+            to: sender(),
+            value: 0,
+            mode: modeFlag // Ok
+        });
+    }
+}