Skip to content

Commit

Permalink
feat: ✨ unimplemented functions detector
Browse files Browse the repository at this point in the history
  • Loading branch information
mfbevan committed Sep 18, 2023
1 parent fae316c commit 878688f
Show file tree
Hide file tree
Showing 9 changed files with 262 additions and 46 deletions.
4 changes: 2 additions & 2 deletions modules/analyser/detectors/abstract-detector.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { AnalyserConfig, Detector, DetectorViolation, ParsedContract, Severity } from "@/types";
import { AnalyserConfig, Detector, DetectorViolation, ParsedContracts, Severity } from "@/types";

export abstract class AbstractDetector implements Detector {
public abstract id: string;
public abstract title: string;
public abstract description: string;
public abstract severity: Severity;

abstract detect(code: ParsedContract, config?: AnalyserConfig): Promise<DetectorViolation[]>;
abstract detect(code: ParsedContracts, config?: AnalyserConfig): Promise<DetectorViolation[]>;
}
5 changes: 3 additions & 2 deletions modules/analyser/detectors/arbitray-send-eth.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { AbstractDetector } from "./abstract-detector";

import { DetectorViolation, ParsedContract, Severity } from "@/types";
import { DetectorViolation, ParsedContracts, Severity } from "@/types";
import { AnalyserConfig } from "@/types/config";

export const ARBITRARY_SEND_ETH_DETECTOR = "arbitrary-send-eth";
Expand All @@ -11,7 +11,8 @@ export class ArbitrarySendEthDetector implements AbstractDetector {
public description = "Detects arbitrary send of ETH";
public severity = Severity.High;

detect(code: ParsedContract, config: AnalyserConfig): Promise<DetectorViolation[]> {
detect(code: ParsedContracts, config: AnalyserConfig): Promise<DetectorViolation[]> {
// TODO implement this
console.log(code, config);
throw new Error("Method not implemented.");
}
Expand Down
8 changes: 4 additions & 4 deletions modules/analyser/detectors/naming-convention.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { AbstractDetector } from "./abstract-detector";

import { SolidityParser } from "@/modules";
import { Severity, ParsedContract, DetectorViolation } from "@/types";
import { Severity, ParsedContracts, DetectorViolation } from "@/types";

export const NAMING_CONVENTION_DETECTOR = "naming-convention";

Expand All @@ -12,13 +12,13 @@ export class NamingConventionDetector implements AbstractDetector {
public severity = Severity.Informational;

async detect(
parsedContract: ParsedContract
parsedContract: ParsedContracts
// config: AnalyserConfig = {}
): Promise<DetectorViolation[]> {
const violations: DetectorViolation[] = [];

const addViolation = (target: string, name: string, convention: string) => {
violations.push({ target, name, convention });
const addViolation = (target: string, name: string, violation: string) => {
violations.push({ target, name, violation });
};

const contracts = SolidityParser.getContracts(parsedContract);
Expand Down
81 changes: 81 additions & 0 deletions modules/analyser/detectors/unimplemented-function.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
import { ContractDefinition, FunctionDefinition } from "@solidity-parser/parser/dist/src/ast-types";

import { AbstractDetector } from "./abstract-detector";

import { SolidityParser } from "@/modules";
import { DetectorViolation, ParsedContracts, Severity } from "@/types";

export const UNIMPLEMENTED_FUNCTION_DETECTOR = "unimplemented-function";

export class UnimplementedFunctionDetector implements AbstractDetector {
public id = UNIMPLEMENTED_FUNCTION_DETECTOR;
public title = "Unimplemented Function";
public description = "Detects unimplemented functions on derived contracts";
public severity = Severity.Informational;

detect(
code: ParsedContracts
// config: AnalyserConfig = {}
): Promise<DetectorViolation[]> {
const violations: DetectorViolation[] = [];

const addViolation = (target: string, name: string, violation: string) => {
violations.push({ target, name, violation });
};

const contracts = SolidityParser.getContracts(code);

contracts.forEach((contract) => {
const functions = this.detectUnimplementedFunction(contract, contracts);
functions.forEach((func) => {
addViolation("function", func.name ?? "unknown", "unimplemented");
});
});

return Promise.resolve(violations);
}

private detectUnimplementedFunction(
contract: ContractDefinition,
contracts: ContractDefinition[]
): FunctionDefinition[] {
const unimplemented: FunctionDefinition[] = [];

const implementedFunctions = SolidityParser.getFunctions([contract]);
const inheritedContracts = SolidityParser.getInheritedContracts(contract, contracts);

inheritedContracts.forEach((inheritedContract) => {
const inheritedFunctions = SolidityParser.getFunctions([inheritedContract]);
inheritedFunctions.forEach((inheritedFunction) => {
if (inheritedFunction.isConstructor) return;
if (inheritedFunction.isFallback) return;

let isImplemented = implementedFunctions.some(
(func) => func.name === inheritedFunction.name
);

if (!isImplemented && this.matchStateVariable(contract, inheritedFunction)) {
isImplemented = true;
}

if (!isImplemented) {
unimplemented.push(inheritedFunction);
}
});
});

return unimplemented;
}

/**
* Since Solidity v0.5.1, a state variable can be used to return the value for a function.
*/
private matchStateVariable(contract: ContractDefinition, func: FunctionDefinition): boolean {
// TODO only run if Solidity version > 0.5.1
const stateVariables = SolidityParser.getStateVariables([contract]);

return stateVariables.some((stateVariable) =>
stateVariable.variables.some((variable) => variable.name === func.name)
);
}
}
50 changes: 27 additions & 23 deletions modules/parser/solidity-parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
StructDefinition,
} from "@solidity-parser/parser/dist/src/ast-types";

import { ParsedContract } from "@/types";
import { ParsedContracts } from "@/types";

/**
* A service for parsing Solidity code and files into an AST.
Expand Down Expand Up @@ -41,7 +41,7 @@ export class SolidityParser {
* @param ast The Solidity AST to search for contract definitions.
* @returns An array of all contract definitions in the given Solidity AST.
*/
public static getContracts(ast: ParsedContract): ContractDefinition[] {
public static getContracts(ast: ParsedContracts): ContractDefinition[] {
return ast.children.filter(
(node) => node.type === "ContractDefinition"
) as ContractDefinition[];
Expand All @@ -52,31 +52,23 @@ export class SolidityParser {
* @param ast The Solidity AST to search for state variables.
* @returns An array of all state variables in the given Solidity AST.
*/
public static getStateVariables(
contracts: ContractDefinition[]
): StateVariableDeclaration[] {
public static getStateVariables(contracts: ContractDefinition[]): StateVariableDeclaration[] {
return contracts
.map((contract) => contract.subNodes)
.flat()
.filter(
(node) => node.type === "StateVariableDeclaration"
) as StateVariableDeclaration[];
.filter((node) => node.type === "StateVariableDeclaration") as StateVariableDeclaration[];
}

/**
* Returns an array of all function definitions in the given Solidity AST.
* @param contracts The Solidity AST to search for function definitions.
* @returns An array of all function definitions in the given Solidity AST.
*/
public static getFunctions(
contracts: ContractDefinition[]
): FunctionDefinition[] {
public static getFunctions(contracts: ContractDefinition[]): FunctionDefinition[] {
return contracts
.map((contract) => contract.subNodes)
.flat()
.filter(
(node) => node.type === "FunctionDefinition"
) as FunctionDefinition[];
.filter((node) => node.type === "FunctionDefinition") as FunctionDefinition[];
}

/**
Expand All @@ -96,9 +88,7 @@ export class SolidityParser {
* @param contracts The Solidity AST to search for struct definitions.
* @returns An array of all struct definitions in the given Solidity AST.
*/
public static getStructs(
contracts: ContractDefinition[]
): StructDefinition[] {
public static getStructs(contracts: ContractDefinition[]): StructDefinition[] {
return contracts
.map((contract) => contract.subNodes)
.flat()
Expand All @@ -122,14 +112,28 @@ export class SolidityParser {
* @param contracts The Solidity AST to search for modifier definitions.
* @returns An array of all modifier definitions in the given Solidity AST.
*/
public static getModifiers(
contracts: ContractDefinition[]
): ModifierDefinition[] {
public static getModifiers(contracts: ContractDefinition[]): ModifierDefinition[] {
return contracts
.map((contract) => contract.subNodes)
.flat()
.filter(
(node) => node.type === "ModifierDefinition"
) as ModifierDefinition[];
.filter((node) => node.type === "ModifierDefinition") as ModifierDefinition[];
}

/**
*
* @param contract The contract to search for inherited contracts.
* @param contracts The Solidity AST to search for inherited contracts.
*/
public static getInheritedContracts(
contract: ContractDefinition,
contracts: ContractDefinition[]
): ContractDefinition[] {
const inheritedContractNames = contract.baseContracts.map(
(baseContract) => baseContract.baseName.namePath
);
return contracts.filter((node) => {
if (node.type !== "ContractDefinition") return false;
return inheritedContractNames.includes(node.name);
});
}
}
24 changes: 12 additions & 12 deletions test/detectors/naming-convention.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ describe("NamingConventionDetector", () => {
const violation = violations[0];
expect(violation.target).toBe("contract");
expect(violation.name).toBe("myContract");
expect(violation.convention).toBe("CapWords");
expect(violation.violation).toBe("CapWords");
});

test("should return a violation for an invalid event name", async () => {
Expand All @@ -72,7 +72,7 @@ describe("NamingConventionDetector", () => {
const violation = violations[0];
expect(violation.target).toBe("event");
expect(violation.name).toBe("myEvent");
expect(violation.convention).toBe("CapWords");
expect(violation.violation).toBe("CapWords");
});

test("should return a violation for an invalid struct name", async () => {
Expand All @@ -88,7 +88,7 @@ describe("NamingConventionDetector", () => {
const violation = violations[0];
expect(violation.target).toBe("struct");
expect(violation.name).toBe("myStruct");
expect(violation.convention).toBe("CapWords");
expect(violation.violation).toBe("CapWords");
});

test("should return a violation for an invalid enum name", async () => {
Expand All @@ -104,7 +104,7 @@ describe("NamingConventionDetector", () => {
const violation = violations[0];
expect(violation.target).toBe("enum");
expect(violation.name).toBe("myEnum");
expect(violation.convention).toBe("CapWords");
expect(violation.violation).toBe("CapWords");
});

test("should return a violation for an invalid modifier name", async () => {
Expand All @@ -120,7 +120,7 @@ describe("NamingConventionDetector", () => {
const violation = violations[0];
expect(violation.target).toBe("modifier");
expect(violation.name).toBe("MyModifier");
expect(violation.convention).toBe("mixedCase");
expect(violation.violation).toBe("mixedCase");
});

test("should return a violation for an invalid variable name (casing)", async () => {
Expand All @@ -136,7 +136,7 @@ describe("NamingConventionDetector", () => {
const violation = violations[0];
expect(violation.target).toBe("variable");
expect(violation.name).toBe("MyVariable");
expect(violation.convention).toBe("mixedCase");
expect(violation.violation).toBe("mixedCase");
});

test("should return a violation for an invalid variable name (avoidNames)", async () => {
Expand All @@ -152,7 +152,7 @@ describe("NamingConventionDetector", () => {
const violation = violations[0];
expect(violation.target).toBe("variable");
expect(violation.name).toBe("l");
expect(violation.convention).toBe("avoidNames");
expect(violation.violation).toBe("avoidNames");
});

test("should return a violation for a public variable name with invalid casing", async () => {
Expand All @@ -167,7 +167,7 @@ describe("NamingConventionDetector", () => {
const violation = violations[0];
expect(violation.target).toBe("variable");
expect(violation.name).toBe("MyVariable");
expect(violation.convention).toBe("mixedCase");
expect(violation.violation).toBe("mixedCase");
});

test("should return no violation for an ERC20 standard constant", async () => {
Expand Down Expand Up @@ -206,7 +206,7 @@ describe("NamingConventionDetector", () => {
const violation = violations[0];
expect(violation.target).toBe("variable");
expect(violation.name).toBe("myConstant");
expect(violation.convention).toBe("UPPER_CASE");
expect(violation.violation).toBe("UPPER_CASE");
});

test("should return a violation for a private variable with invalid casing", async () => {
Expand All @@ -221,7 +221,7 @@ describe("NamingConventionDetector", () => {
const violation = violations[0];
expect(violation.target).toBe("variable");
expect(violation.name).toBe("MY_Variable");
expect(violation.convention).toBe("mixedCase");
expect(violation.violation).toBe("mixedCase");
});

test("should return no violation for a private variable with valid casing", async () => {
Expand Down Expand Up @@ -267,7 +267,7 @@ describe("NamingConventionDetector", () => {
const violation = violations[0];
expect(violation.target).toBe("function");
expect(violation.name).toBe("MyFunction");
expect(violation.convention).toBe("mixedCase");
expect(violation.violation).toBe("mixedCase");
});

test("should not return a violation for an internal function with invalid casing if it starts with underscore", async () => {
Expand Down Expand Up @@ -321,7 +321,7 @@ describe("NamingConventionDetector", () => {
const violation = violations[0];
expect(violation.target).toBe("parameter");
expect(violation.name).toBe("MyParameter");
expect(violation.convention).toBe("mixedCase");
expect(violation.violation).toBe("mixedCase");
});

test("should allow for a parameter if it starts with an underscore", async () => {
Expand Down
Loading

0 comments on commit 878688f

Please sign in to comment.