From 9cf370072d3b6704d733a5254659675e58e87109 Mon Sep 17 00:00:00 2001 From: Esorat <95045543+Esorat@users.noreply.github.com> Date: Wed, 29 Jan 2025 19:22:02 +0700 Subject: [PATCH] feat(detectors): Add `SuspiciousLoop` (#206) Closes #171 --- CHANGELOG.md | 1 + README.md | 2 +- src/detectors/builtin/suspiciousLoop.ts | 103 +++++++++++++++++++++ src/detectors/detector.ts | 7 ++ test/README.md | 2 - test/detector.spec.ts | 3 +- test/detectors/NeverAccessedVariables.tact | 1 - test/detectors/SuspiciousLoop.expected.out | 44 +++++++++ test/detectors/SuspiciousLoop.tact | 36 +++++++ 9 files changed, 194 insertions(+), 5 deletions(-) create mode 100644 src/detectors/builtin/suspiciousLoop.ts create mode 100644 test/detectors/SuspiciousLoop.expected.out create mode 100644 test/detectors/SuspiciousLoop.tact diff --git a/CHANGELOG.md b/CHANGELOG.md index 068a0339..191b75da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - `UnprotectedCall` detector: PR [#235](https://github.com/nowarp/misti/pull/235) +- `SuspiciousLoop` detector: PR [#206](https://github.com/nowarp/misti/pull/206) - File-scoped CFG dumps: Issue [#241](https://github.com/nowarp/misti/issues/241) ### Fixed diff --git a/README.md b/README.md index 969ef5f0..083a0605 100644 --- a/README.md +++ b/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/): 28 detectors [are available](https://nowarp.io/tools/misti/docs/next/detectors) +- [Tact](https://tact-lang.org/): 30 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 #### Features diff --git a/src/detectors/builtin/suspiciousLoop.ts b/src/detectors/builtin/suspiciousLoop.ts new file mode 100644 index 00000000..dc204cfd --- /dev/null +++ b/src/detectors/builtin/suspiciousLoop.ts @@ -0,0 +1,103 @@ +import { CompilationUnit } from "../../internals/ir"; +import { + foldStatements, + evalsToValue, + evalsToPredicate, +} from "../../internals/tact"; +import { MistiTactWarning, Severity } from "../../internals/warnings"; +import { AstDetector } from "../detector"; +import { + AstStatement, + AstExpression, +} from "@tact-lang/compiler/dist/grammar/ast"; + +/** + * 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 (10_001) { // Bad: High iteration count + * // ... + * } + * + * while (true) { // Bad: Unbounded condition + * // ... + * } + * ``` + */ +export class SuspiciousLoop extends AstDetector { + severity = Severity.MEDIUM; + + async check(cu: CompilationUnit): Promise { + return Array.from(cu.ast.getProgramEntries()).reduce((acc, node) => { + return acc.concat( + ...foldStatements( + node, + (acc, stmt) => { + return acc.concat(this.analyzeLoopStatement(stmt)); + }, + acc, + ), + ); + }, [] as MistiTactWarning[]); + } + + /** + * Analyzes a loop statement to determine if it contains a suspicious condition. + */ + private analyzeLoopStatement(stmt: AstStatement): MistiTactWarning[] { + if ( + stmt.kind === "statement_repeat" && + evalsToPredicate(stmt.iterations, (v) => v > 10_000n) + ) { + return [ + this.makeWarning("Potential high-cost loop", stmt.iterations.loc, { + suggestion: "Avoid excessive iterations in loops", + }), + ]; + } + if (stmt.kind === "statement_while") { + let warnings: MistiTactWarning[] = []; + warnings = warnings.concat(this.checkTrueCondition(stmt.condition)); + if (warnings.length === 0 && stmt.statements.length > 0) { + warnings = warnings.concat(this.checkFalseCondition(stmt.condition)); + } + return warnings; + } + if (stmt.kind === "statement_until") { + let warnings: MistiTactWarning[] = []; + warnings = warnings.concat(this.checkTrueCondition(stmt.condition)); + return warnings; + } + return []; + } + + private checkFalseCondition(expr: AstExpression): MistiTactWarning[] { + if (evalsToValue(expr, "boolean", false)) { + return [ + this.makeWarning("Loop condition is always false", expr.loc, { + suggestion: + "The condition is always false; the body will never execute", + }), + ]; + } + return []; + } + + private checkTrueCondition(expr: AstExpression): MistiTactWarning[] { + if (evalsToValue(expr, "boolean", true)) { + return [ + this.makeWarning("Infinite loop detected", expr.loc, { + suggestion: "Avoid unbounded conditions in loops", + }), + ]; + } + return []; + } +} diff --git a/src/detectors/detector.ts b/src/detectors/detector.ts index 63ac53e3..222f1c7c 100644 --- a/src/detectors/detector.ts +++ b/src/detectors/detector.ts @@ -406,6 +406,13 @@ export const BuiltInDetectors: Record = { ), enabledByDefault: true, }, + SuspiciousLoop: { + loader: (ctx: MistiContext) => + import("./builtin/suspiciousLoop").then( + (module) => new module.SuspiciousLoop(ctx), + ), + enabledByDefault: true, + }, }; /** diff --git a/test/README.md b/test/README.md index 63df1682..f79f96b6 100644 --- a/test/README.md +++ b/test/README.md @@ -1,5 +1,3 @@ -Here's the updated text with the grammar fixed: - # Tests ## Structure diff --git a/test/detector.spec.ts b/test/detector.spec.ts index 3815ec54..cc9df7a9 100644 --- a/test/detector.spec.ts +++ b/test/detector.spec.ts @@ -13,7 +13,8 @@ describe("Common detectors functionality", () => { it(`should generate valid JSON output for ${CONTRACT_NAME}`, async () => { const filePath = ABSOLUTE_PATH; const output = await executeMisti([ - "--all-detectors", + "--enabled-detectors", + "NeverAccessedVariables", "--no-colors", "--output-format", "json", diff --git a/test/detectors/NeverAccessedVariables.tact b/test/detectors/NeverAccessedVariables.tact index cb1e1a4f..5362b76a 100644 --- a/test/detectors/NeverAccessedVariables.tact +++ b/test/detectors/NeverAccessedVariables.tact @@ -77,4 +77,3 @@ contract TestContract2 { let _test: Int = 123; // suppressed: no warning } } - diff --git a/test/detectors/SuspiciousLoop.expected.out b/test/detectors/SuspiciousLoop.expected.out new file mode 100644 index 00000000..0cb63675 --- /dev/null +++ b/test/detectors/SuspiciousLoop.expected.out @@ -0,0 +1,44 @@ +[MEDIUM] SuspiciousLoop: Infinite loop detected +test/detectors/SuspiciousLoop.tact:10:16: + 9 | let i: Int = 0; +> 10 | while (true) { i = i + 1; } // Bad + ^ + 11 | do { i = i + 1; } until (true); // Bad +Help: Avoid unbounded conditions in loops +See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousLoop + +[MEDIUM] SuspiciousLoop: Infinite loop detected +test/detectors/SuspiciousLoop.tact:11:34: + 10 | while (true) { i = i + 1; } // Bad +> 11 | do { i = i + 1; } until (true); // Bad + ^ + 12 | do { i = i + 1; } until (TRUE); // TODO: Unsupported in consteval +Help: Avoid unbounded conditions in loops +See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousLoop + +[MEDIUM] SuspiciousLoop: Potential high-cost loop +test/detectors/SuspiciousLoop.tact:16:17: + 15 | fun testRepeatHighCount() { +> 16 | repeat (10_001) { let x = 1; } // Bad + ^ + 17 | repeat (A + B) { let x = 1; } // TODO: Unsupported in consteval +Help: Avoid excessive iterations in loops +See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousLoop + +[MEDIUM] SuspiciousLoop: Potential high-cost loop +test/detectors/SuspiciousLoop.tact:24:21: + 23 | while (i < 10) { +> 24 | repeat (1_000_000) { i = i + 1; } // Bad + ^ + 25 | } +Help: Avoid excessive iterations in loops +See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousLoop + +[MEDIUM] SuspiciousLoop: Loop condition is always false +test/detectors/SuspiciousLoop.tact:30:16: + 29 | let a: Int = 0; +> 30 | while (false) { a = 1; } // Bad + ^ + 31 | while (false) { } // OK: Empty body +Help: The condition is always false; the body will never execute +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 00000000..2e00da45 --- /dev/null +++ b/test/detectors/SuspiciousLoop.tact @@ -0,0 +1,36 @@ +const TRUE: Bool = true; +const FALSE: Bool = false; +const A: Int = 1000; +const B: Int = 10000; + +contract TestSuspiciousLoops { + + fun testWhileInfinite() { + let i: Int = 0; + while (true) { i = i + 1; } // Bad + do { i = i + 1; } until (true); // Bad + do { i = i + 1; } until (TRUE); // TODO: Unsupported in consteval + } + + fun testRepeatHighCount() { + repeat (10_001) { let x = 1; } // Bad + repeat (A + B) { let x = 1; } // TODO: Unsupported in consteval + repeat (A) { let x = 1; } // OK + } + + fun testNestedLoops() { + let i: Int = 0; + while (i < 10) { + repeat (1_000_000) { i = i + 1; } // Bad + } + } + + fun testLoop() { + let a: Int = 0; + while (false) { a = 1; } // Bad + while (false) { } // OK: Empty body + do { a = 1; } until (false); // OK: Until is allowed + do {} until (false); // OK: Until is allowed + while (TRUE && FALSE) { a = 1; } // TODO: Unsupported in consteval + } +}