Skip to content

Commit

Permalink
feat(detectors): Add UnboundMap
Browse files Browse the repository at this point in the history
Closes #50
  • Loading branch information
jubnzv committed Oct 20, 2024
1 parent f054161 commit a2b2649
Show file tree
Hide file tree
Showing 6 changed files with 146 additions and 1 deletion.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Added
- `SendInLoop` detector: PR [#168](https://github.com/nowarp/misti/pulls/168)
- `CellOverflow` detector: PR [#177](https://github.com/nowarp/misti/pulls/177)
- `UnboundMap` detector: Issue [#50](https://github.com/nowarp/misti/issues/50)
- Import Graph: PR [#180](https://github.com/nowarp/misti/pulls/180)

### Changed
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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/): 22 detectors [are available](https://nowarp.io/tools/misti/docs/next/detectors)
- [Tact](https://tact-lang.org/): 23 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
Expand Down
115 changes: 115 additions & 0 deletions src/detectors/builtin/unboundMap.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
import { CompilationUnit } from "../../internals/ir";
import { collectFields, forEachExpression, isSelf } from "../../internals/tact";
import { intersectSets } from "../../internals/util";
import { MistiTactWarning, Severity } from "../../internals/warnings";
import { ASTDetector } from "../detector";
import {
AstContract,
AstFieldDecl,
idText,
} from "@tact-lang/compiler/dist/grammar/ast";

type FieldName = string;

const MAP_ADD_OPERATIONS = new Set<string>(["set"]);
const MAP_DEL_OPERATIONS = new Set<string>(["del"]);

/**
* An optional detector that highlights cases where a map field allows inserting
* values (e.g., via `.set`) but lacks functionality for removing entries (e.g., via `.del`).
*
* ## Why is it bad?
* A map without a method to remove elements can lead to storage overflow, particularly
* in long-term contract usage. Failing to provide a way to clear or delete entries
* can result in uncontrolled storage growth, which not only wastes resources but
* may also increase the cost of contract execution and maintenance over time.
*
* ## Example
* ```tact
* contract Test {
* map: Map<Int, String>;
*
* setEntry(key: Int, value: String) {
* self.map.set(key, value); // Bad
* }
* }
* ```
*
* Use instead:
* ```tact
* contract Test {
* map: Map<Int, String>;
*
* setEntry(key: Int, value: String) {
* self.map.set(key, value);
* }
*
* delEntry(key: Int) {
* self.map.del(key); // Fixed: Added a new API method
* }
* }
* ```
*/
export class UnboundMap extends ASTDetector {
severity = Severity.LOW;

async check(cu: CompilationUnit): Promise<MistiTactWarning[]> {
return Array.from(cu.ast.getContracts()).reduce(
(acc, contract) => acc.concat(this.checkContract(contract)),
[] as MistiTactWarning[],
);
}

private checkContract(contract: AstContract): MistiTactWarning[] {
const mapFields = Array.from(collectFields(contract).values()).reduce(
(acc, field) => {
if (field.type.kind === "map_type") acc.set(idText(field.name), field);
return acc;
},
new Map<FieldName, AstFieldDecl>(),
);
const { added, removed } = this.findUses(contract, mapFields);
return Array.from(intersectSets(added, removed)).map((notRemovedName) => {
const decl = mapFields.get(notRemovedName)!;
return this.makeWarning(
`Map self.${notRemovedName} could be unbound`,
decl.loc,
{
extraDescription:
"There are operations adding elements to this map, but there is no API to remove them",
suggestion:
"Consider adding a method to remove elements or suppress this warning",
},
);
});
}

/**
* Finds which map fields has the operation adding elements to it (`added`)
* and to remove them (`removed`).
*/
private findUses(
contract: AstContract,
mapFields: Map<FieldName, AstFieldDecl>,
): { added: Set<FieldName>; removed: Set<FieldName> } {
return contract.declarations.reduce(
({ added, removed }, decl) => {
forEachExpression(decl, (expr) => {
if (
expr.kind === "method_call" &&
expr.self.kind === "field_access" &&
isSelf(expr.self.aggregate) &&
mapFields.has(idText(expr.self.field))
) {
if (MAP_ADD_OPERATIONS.has(idText(expr.method)))
added.add(idText(expr.self.field));
else if (MAP_DEL_OPERATIONS.has(idText(expr.method)))
removed.add(idText(expr.self.field));
}
});
return { added, removed };
},
{ added: new Set<FieldName>(), removed: new Set<FieldName>() },
);
}
}
7 changes: 7 additions & 0 deletions src/detectors/detector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,13 @@ export const BuiltInDetectors: Record<string, DetectorEntry> = {
),
enabledByDefault: true,
},
UnboundMap: {
loader: (ctx: MistiContext) =>
import("./builtin/unboundMap").then(
(module) => new module.UnboundMap(ctx),
),
enabledByDefault: false,
},
};

/**
Expand Down
9 changes: 9 additions & 0 deletions test/detectors/UnboundMap.expected.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
[LOW] UnboundMap: Map self.m_ok could be unbound
test/detectors/UnboundMap.tact:3:5:
2 | m_bad: map<Int, Int> = emptyMap();
> 3 | m_ok: map<Int, Int> = emptyMap();
^
4 | // OK: Don't report it since we put nothing in this map
There are operations adding elements to this map, but there is no API to remove them
Help: Consider adding a method to remove elements or suppress this warning
See: https://nowarp.io/tools/misti/docs/detectors/UnboundMap
13 changes: 13 additions & 0 deletions test/detectors/UnboundMap.tact
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
contract C {
m_bad: map<Int, Int> = emptyMap();
m_ok: map<Int, Int> = emptyMap();
// OK: Don't report it since we put nothing in this map
m_unused: map<Int, Int> = emptyMap();

fun test() {
self.m_ok.set(1, 1);
self.m_ok.del(1);

self.m_bad.set(1, 1);
}
}

0 comments on commit a2b2649

Please sign in to comment.