diff --git a/package.json b/package.json index 9e0df47..f00544d 100644 --- a/package.json +++ b/package.json @@ -44,6 +44,15 @@ "solc-0.7.5": "npm:solc@0.7.5", "solc-0.7.6": "npm:solc@0.7.6", "solc-0.8.0": "npm:solc@0.8.0", + "solc-0.8.1": "npm:solc@0.8.1", + "solc-0.8.2": "npm:solc@0.8.2", + "solc-0.8.3": "npm:solc@0.8.3", + "solc-0.8.4": "npm:solc@0.8.4", + "solc-0.8.5": "npm:solc@0.8.5", + "solc-0.8.6": "npm:solc@0.8.6", + "solc-0.8.7": "npm:solc@0.8.7", + "solc-0.8.8": "npm:solc@0.8.8", + "solc-0.8.9": "npm:solc@0.8.9", "solc-0.8.10": "npm:solc@0.8.10", "solc-0.8.11": "npm:solc@0.8.11", "solc-0.8.12": "npm:solc@0.8.12", @@ -52,14 +61,13 @@ "solc-0.8.15": "npm:solc@0.8.15", "solc-0.8.16": "npm:solc@0.8.16", "solc-0.8.17": "npm:solc@0.8.17", - "solc-0.8.2": "npm:solc@0.8.2", - "solc-0.8.3": "npm:solc@0.8.3", - "solc-0.8.4": "npm:solc@0.8.4", - "solc-0.8.5": "npm:solc@0.8.5", - "solc-0.8.6": "npm:solc@0.8.6", - "solc-0.8.8": "npm:solc@0.8.8", - "solc-0.8.9": "npm:solc@0.8.9", - "solidity-ast": "^0.4.35", + "solc-0.8.18": "npm:solc@0.8.18", + "solc-0.8.19": "npm:solc@0.8.19", + "solc-0.8.20": "npm:solc@0.8.20", + "solc-0.8.21": "npm:solc@0.8.21", + "solc-0.8.22": "npm:solc@0.8.22", + "solc-0.8.23": "npm:solc@0.8.23", + "solidity-ast": "^0.4.53", "ts-node": "^10.9.1", "typescript": "^4.7.4" }, diff --git a/src/compile.ts b/src/compile.ts index 1dea880..0fda6f5 100644 --- a/src/compile.ts +++ b/src/compile.ts @@ -2,6 +2,7 @@ import fs from 'fs'; import path from 'path'; import semver from 'semver'; import type { SourceUnit } from 'solidity-ast'; +import { recursiveExploration } from './utils'; const versions = Object.keys(require('../package.json').dependencies) .filter(s => s.startsWith('solc-')) @@ -70,6 +71,16 @@ const findImports = (basePath: string) => { relativePath = relativePath.replace(line.split('=')[0], line.split('=')[1]); } } + + // Dedouping logic for nested remappings + let arrayRelativePath = relativePath.split("/"); + let dedoupedArrayRelativePath = [arrayRelativePath[0]]; + for(let j = 1; j < arrayRelativePath.length; j++){ + if(arrayRelativePath[j - 1] != arrayRelativePath[j]){ + dedoupedArrayRelativePath.push(arrayRelativePath[j]); + } + } + relativePath = dedoupedArrayRelativePath.join("/"); const absolutePath = path.resolve(basePath, relativePath); const source = fs.readFileSync(absolutePath, 'utf8'); @@ -99,7 +110,7 @@ const compileAndBuildAST = async (basePath: string, fileNames: string[]): Promis /** Read scope and fill file list */ let i = 0; for (const file of fileNames) { - const content = fs.readFileSync(path.join(basePath, file), { encoding: 'utf8', flag: 'r' }); + const content = await fs.readFileSync(path.join(basePath, file), { encoding: 'utf8', flag: 'r' }); if (!!content) { if (!content.match(/pragma solidity (.*);/)) { console.log(`Cannot find pragma in ${path.join(basePath, file)}`); @@ -133,6 +144,11 @@ const compileAndBuildAST = async (basePath: string, fileNames: string[]): Promis }, {}), basePath, ).then(output => { + if(output.errors && output.errors.length > 0){ + for (const _error of output.errors) { + console.error(_error); + } + } for (const f of filteredSources) { if (!output.sources[f.file]?.ast) { console.log(`Cannot compile AST for ${f.file}`); diff --git a/src/index.ts b/src/index.ts index db9598e..cade67e 100644 --- a/src/index.ts +++ b/src/index.ts @@ -9,21 +9,12 @@ import main from './main'; // ================================= PARAMETERS ================================ -import { program } from 'commander'; +const basePath = + process.argv.length > 2 ? (process.argv[2].endsWith('/') ? process.argv[2] : process.argv[2] + '/') : 'contracts/'; +const scopeFile = process.argv.length > 3 && process.argv[3].endsWith('txt') ? process.argv[3] : null; +const githubLink = process.argv.length > 4 && process.argv[4] ? process.argv[4] : null; +const out = 'report.md'; -program - .argument('[string]', 'Path were the contracts lies') - .option('-s, --scope ', '.txt file containing the contest scope') - .option('-g, --github ', 'github url to generate links to code') - .option('-o, --out ', 'where to save the report') - .action((basePath:string, options) => { - basePath = basePath ||'contracts/'; - basePath = basePath.endsWith('/') ? basePath : `${basePath}/`; - console.log(`basePath: ${basePath}`); - console.log(`scope: ${options.scope||'----'}`); - console.log(`github: ${options.github||'----'}`); - console.log(`out: ${options.out||'report.md'}`); - console.log('*****************************') - // ============================== GENERATE REPORT ============================== - main(basePath, options.scope, options.gihub, options.out || 'report.md'); - }).parse(); +// ============================== GENERATE REPORT ============================== + +main(basePath, scopeFile, githubLink, out); diff --git a/src/issues/GAS/ERC721A.ts b/src/issues/GAS/ERC721A.ts new file mode 100644 index 0000000..db9dfc2 --- /dev/null +++ b/src/issues/GAS/ERC721A.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Use ERC721A instead ERC721', + description: + "ERC721A standard, ERC721A is an improvement standard for ERC721 tokens. It was proposed by the Azuki team and used for developing their NFT collection. Compared with ERC721, ERC721A is a more gas-efficient standard to mint a lot of of NFTs simultaneously. It allows developers to mint multiple NFTs at the same gas price. This has been a great improvement due to Ethereum's sky-rocketing gas fee.\n\n Reference: https://nextrope.com/erc721-vs-erc721a-2/", + regex: /import.+openz.+ERC721\.sol/gi, +}; + +export default issue; diff --git a/src/issues/GAS/_msgSender.ts b/src/issues/GAS/_msgSender.ts new file mode 100644 index 0000000..bebd52c --- /dev/null +++ b/src/issues/GAS/_msgSender.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: "Don't use `_msgSender()` if not supporting EIP-2771", + description: + "Use `msg.sender` if the code does not implement [EIP-2771 trusted forwarder](https://eips.ethereum.org/EIPS/eip-2771) support", + regex: /_msgSender\(\)/gi, +}; + +export default issue; diff --git a/src/issues/GAS/addPlusEqual.ts b/src/issues/GAS/addPlusEqual.ts new file mode 100644 index 0000000..4d03b69 --- /dev/null +++ b/src/issues/GAS/addPlusEqual.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: '`a = a + b` is more gas effective than `a += b` for state variables (excluding arrays and mappings)', + description: + 'This saves **16 gas per instance.**', + regex: /\+=/gi, +}; + +export default issue; diff --git a/src/issues/GAS/addressBalance.ts b/src/issues/GAS/addressBalance.ts deleted file mode 100644 index 3c7c890..0000000 --- a/src/issues/GAS/addressBalance.ts +++ /dev/null @@ -1,33 +0,0 @@ -import { InputType, IssueTypes, Instance, ASTIssue } from '../../types'; -import { findAll } from 'solidity-ast/utils'; -import { instanceFromSRC } from '../../utils'; - -const issue: ASTIssue = { - regexOrAST: 'AST', - type: IssueTypes.GAS, - title: 'Use `selfbalance()` instead of `address(this).balance`', - description: - "Use assembly when getting a contract's balance of ETH.\n\nYou can use `selfbalance()` instead of `address(this).balance` when getting your contract's balance of ETH to save gas.\nAdditionally, you can use `balance(address)` instead of `address.balance()` when getting an external contract's balance of ETH.\n\n*Saves 15 gas when checking internal balance, 6 for external*", - detector: (files: InputType): Instance[] => { - let instances: Instance[] = []; - - for (const file of files) { - if (!!file.ast) { - for (const node of findAll('MemberAccess', file.ast)) { - // Look for Address(X).balance - if ( - node.nodeType === 'MemberAccess' && - node.memberName === 'balance' && - node.expression.nodeType === 'FunctionCall' && - node.expression.typeDescriptions.typeString === 'address' - ) { - instances.push(instanceFromSRC(file, node.src)); - } - } - } - } - return instances; - }, -}; - -export default issue; diff --git a/src/issues/GAS/assignUpdateArray.ts b/src/issues/GAS/assignUpdateArray.ts index 671af42..47f90b9 100644 --- a/src/issues/GAS/assignUpdateArray.ts +++ b/src/issues/GAS/assignUpdateArray.ts @@ -7,7 +7,7 @@ const issue: ASTIssue = { type: IssueTypes.GAS, title: '`array[index] += amount` is cheaper than `array[index] = array[index] + amount` (or related variants)', description: - 'When updating a value in an array with arithmetic, using `array[index] += amount` is cheaper than `array[index] = array[index] + amount`.\nThis is because you avoid an additonal `mload` when the array is stored in memory, and an `sload` when the array is stored in storage.\nThis can be applied for any arithmetic operation including `+=`, `-=`,`/=`,`*=`,`^=`,`&=`, `%=`, `<<=`,`>>=`, and `>>>=`.\nThis optimization can be particularly significant if the pattern occurs during a loop.\n\n*Saves 28 gas for a storage array, 38 for a memory array*', + 'When updating a value in an array with arithmetic, using `array[index] += amount` is cheaper than `array[index] = array[index] + amount`.\n\nThis is because you avoid an additional `mload` when the array is stored in memory, and an `sload` when the array is stored in storage.\n\nThis can be applied for any arithmetic operation including `+=`, `-=`,`/=`,`*=`,`^=`,`&=`, `%=`, `<<=`,`>>=`, and `>>>=`.\n\nThis optimization can be particularly significant if the pattern occurs during a loop.\n\n*Saves 28 gas for a storage array, 38 for a memory array*', detector: (files: InputType): Instance[] => { let instances: Instance[] = []; diff --git a/src/issues/GAS/boolCompare.ts b/src/issues/GAS/boolCompare.ts new file mode 100644 index 0000000..aa2c1d8 --- /dev/null +++ b/src/issues/GAS/boolCompare.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Comparing to a Boolean constant', + description: + 'Comparing to a constant (`true` or `false`) is a bit more expensive than directly checking the returned boolean value.\n\nConsider using `if(directValue)` instead of `if(directValue == true)` and `if(!directValue)` instead of `if(directValue == false)`', + regex: /(== true)|(== false)/gi, // storage variable | part of mapping | struct +}; + +export default issue; diff --git a/src/issues/GAS/cacheArrayLength.ts b/src/issues/GAS/cacheArrayLength.ts index b984f5d..8ea4316 100644 --- a/src/issues/GAS/cacheArrayLength.ts +++ b/src/issues/GAS/cacheArrayLength.ts @@ -6,7 +6,7 @@ const issue: RegexIssue = { title: 'Cache array length outside of loop', description: 'If not cached, the solidity compiler will always read the length of the array during each iteration. That is, if it is a storage array, this is an extra sload operation (100 additional extra gas for each iteration except for the first) and if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first).', - regex: /for?.(.*\.length)/g, + regex: /for ?\(.*\.length/gi, }; export default issue; diff --git a/src/issues/GAS/calldataViewFunctions.ts b/src/issues/GAS/calldataViewFunctions.ts index c2dcb51..a264ba5 100644 --- a/src/issues/GAS/calldataViewFunctions.ts +++ b/src/issues/GAS/calldataViewFunctions.ts @@ -7,13 +7,16 @@ const issue: ASTIssue = { type: IssueTypes.GAS, title: 'Use calldata instead of memory for function arguments that do not get mutated', description: - 'Mark data types as `calldata` instead of `memory` where possible. This makes it so that the data is not automatically loaded into memory. If the data passed into the function does not need to be changed (like updating values in an array), it can be passed in as `calldata`. The one exception to this is if the argument must later be passed into another function that takes an argument that specifies `memory` storage.', + "When a function with a `memory` array is called externally, the `abi.decode()` step has to use a for-loop to copy each index of the `calldata` to the `memory` index. Each iteration of this for-loop costs at least 60 gas (i.e. `60 * .length`). Using `calldata` directly bypasses this loop. \n\nIf the array is passed to an `internal` function which passes the array to another internal function where the array is modified and therefore `memory` is used in the `external` call, it's still more gas-efficient to use `calldata` when the `external` function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one. \n\n *Saves 60 gas per instance*", detector: (files: InputType): Instance[] => { let instances: Instance[] = []; for (const file of files) { if (!!file.ast) { for (const node of findAll('FunctionDefinition', file.ast)) { + if(node.kind == "constructor"){ + continue; + } if (node.visibility === 'external' || node.visibility === 'public') { for (const param of Object.values(node.parameters.parameters)) { if (param.storageLocation === 'memory') { diff --git a/src/issues/GAS/customErrors.ts b/src/issues/GAS/customErrors.ts index a4671aa..6f1de28 100644 --- a/src/issues/GAS/customErrors.ts +++ b/src/issues/GAS/customErrors.ts @@ -3,10 +3,10 @@ import { IssueTypes, RegexIssue } from '../../types'; const issue: RegexIssue = { regexOrAST: 'Regex', type: IssueTypes.GAS, - title: 'Use Custom Errors', + title: 'Use Custom Errors instead of Revert Strings to save Gas', description: - '[Source](https://blog.soliditylang.org/2021/04/21/custom-errors/)\nInstead of using error strings, to reduce deployment and runtime cost, you should use Custom Errors. This would save both deployment and runtime cost.', - regex: /(require|revert)\(.*,?".*"\)/g, + 'Custom errors are available from solidity version 0.8.4. Custom errors save [**~50 gas**](https://gist.github.com/IllIllI000/ad1bd0d29a0101b25e57c293b4b0c746) each time they\'re hit by [avoiding having to allocate and store the revert string](https://blog.soliditylang.org/2021/04/21/custom-errors/#errors-in-depth). Not defining the strings also save deployment gas\n\nAdditionally, custom errors can be used inside and outside of contracts (including interfaces and libraries).\n\nSource: :\n\n> Starting from [Solidity v0.8.4](https://github.com/ethereum/solidity/releases/tag/v0.8.4), there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., `revert("Insufficient funds.");`), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.\n\nConsider replacing **all revert strings** with custom errors in the solution, and particularly those that have multiple occurrences:', + regex: /(require|revert)\(.*,?".*"\)/gi, }; export default issue; diff --git a/src/issues/GAS/delegatecallAddressCheck.ts b/src/issues/GAS/delegatecallAddressCheck.ts new file mode 100644 index 0000000..9b4f36d --- /dev/null +++ b/src/issues/GAS/delegatecallAddressCheck.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Avoid contract existence checks by using low level calls', + description: + "Prior to 0.8.10 the compiler inserted extra code, including `EXTCODESIZE` (**100 gas**), to check for contract existence for external function calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value. Similar behavior can be achieved in earlier versions by using low-level calls, since low level calls never check for contract existence", + regex: /\.delegatecall|\.balanceOf\(|\.recover\(/gi, +}; + +export default issue; diff --git a/src/issues/GAS/dontCacheIfUsedOnce.ts b/src/issues/GAS/dontCacheIfUsedOnce.ts new file mode 100644 index 0000000..88e3503 --- /dev/null +++ b/src/issues/GAS/dontCacheIfUsedOnce.ts @@ -0,0 +1,52 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.GAS, + title: 'Stack variable used as a cheaper cache for a state variable is only used once', + description: + "If the variable is only accessed once, it's cheaper to use the state variable directly that one time, and save the **3 gas** the extra stack assignment would spend", + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const a of findAll('FunctionDefinition', file.ast)) { + for (const b of findAll('VariableDeclarationStatement', a)) { + if ( + b.declarations && + b.declarations[0] && + b.declarations[0].name && + b.initialValue && + b.initialValue['name'] + ) { + let isStateVar = 0; + for (const c of findAll('VariableDeclaration', file.ast)) { + if ((c.stateVariable || c.storageLocation == "storage") && b.initialValue['name'] + '' == c.name) { + isStateVar++; + } + } + if (isStateVar > 0) { + let accessCounter = 0; + for (const c of findAll('Identifier', a)) { + if (c.name == b.declarations[0].name) { + accessCounter++; + // console.log("accessCounter: ",accessCounter); + } + } + if (accessCounter == 1) { + instances.push(instanceFromSRC(file, b.src)); + } + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/GAS/immutableConstructor.ts b/src/issues/GAS/immutableConstructor.ts new file mode 100644 index 0000000..3556a02 --- /dev/null +++ b/src/issues/GAS/immutableConstructor.ts @@ -0,0 +1,64 @@ +import { ContractDefinition } from 'solidity-ast'; +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC, topLevelFiles, getStorageVariable } from '../../utils'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.GAS, + title: 'State variables only set in the constructor should be declared `immutable`', + description: + 'Variables only set in the constructor and never edited afterwards should be marked as immutable, as it would avoid the expensive storage-writing operation in the constructor (around **20 000 gas** per variable) and replace the expensive storage-reading operations (around **2100 gas** per reading) to a less expensive value reading (**3 gas**)', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + + for (const file of files) { + if (!!file.ast) { + let storageVariables; + let assignSrcs = new Map(); + for (const contract of findAll('ContractDefinition', file.ast)) { + /** Build list of storage variables */ + storageVariables = getStorageVariable(contract); + // Counting assigments in a mapping? + for (const func of findAll('FunctionDefinition', contract)) { + if (func.kind === 'constructor') { + for (const assign of findAll('Assignment', func)) { + if ( + assign.leftHandSide.nodeType === 'Identifier' && + storageVariables.includes(assign.leftHandSide.name) && + (!assign.rightHandSide || + !assign.rightHandSide['kind'] || + assign.rightHandSide['kind'] != 'structConstructorCall') + ) { + const assignName = assign.leftHandSide.name; + assignSrcs.set(assignName, assign.src); + } + } + } + } + + for (const func of findAll('FunctionDefinition', contract)) { + if (func.kind !== 'constructor') { + for (const assign of findAll('Assignment', func)) { + if (assign.leftHandSide.nodeType === 'Identifier' && assignSrcs.has(assign.leftHandSide.name)) { + const assignName = assign.leftHandSide.name; + assignSrcs.set(assignName, ''); + } + } + } + } + + for (const assignSrc of assignSrcs.keys()) { + const src = assignSrcs.get(assignSrc); + if (!!src && src != '' && src != 'owner') { + instances.push(instanceFromSRC(file, src)); + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/GAS/initializeDefaultValue.ts b/src/issues/GAS/initializeDefaultValue.ts index c10fcb1..7ed425f 100644 --- a/src/issues/GAS/initializeDefaultValue.ts +++ b/src/issues/GAS/initializeDefaultValue.ts @@ -4,7 +4,9 @@ const issue: RegexIssue = { regexOrAST: 'Regex', type: IssueTypes.GAS, title: "Don't initialize variables with default value", - regex: /((uint|int)[0-9]*?.*[a-z,A-Z,0-9]+.?=.?0;)|(bool.[a-z,A-Z,0-9]+.?=.?false;)/g, + description: + 'This is only valid for state variables, as memory ones will be taken care of by the compiler.\n\nIf a state variable is not set/initialized, it is assumed to have the default value (`0` for `uint`, `false` for `bool`, `address(0)` for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas (around **3 gas** per instance).\n\nConsider removing explicit initializations for default values.\n\n*Saves 5000 gas per instance*', + regex: /^(?!.*\bfor\b).*(int.* = 0|address.* = address\(0\)|bool.* = false)/gi, }; export default issue; diff --git a/src/issues/GAS/longRevertString.ts b/src/issues/GAS/longRevertString.ts index 5f4980e..f98aa69 100644 --- a/src/issues/GAS/longRevertString.ts +++ b/src/issues/GAS/longRevertString.ts @@ -3,8 +3,10 @@ import { IssueTypes, RegexIssue } from '../../types'; const issue: RegexIssue = { regexOrAST: 'Regex', type: IssueTypes.GAS, - title: 'Long revert strings', - regex: /(revert|require)\(.*,?.(\"|\').{33,}(\"|\')\)/g, + title: 'Reduce the size of error messages (Long revert Strings)', + description: + 'Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.\n\nRevert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.\n\nConsider shortening the revert strings to fit in 32 bytes.\n\n*Saves around 18 gas per instance*', + regex: /^((?!import).)*".{33,}"/gi, }; export default issue; diff --git a/src/issues/GAS/postIncrement.ts b/src/issues/GAS/postIncrement.ts index 5f70172..ef91d4b 100644 --- a/src/issues/GAS/postIncrement.ts +++ b/src/issues/GAS/postIncrement.ts @@ -3,9 +3,10 @@ import { IssueTypes, RegexIssue } from '../../types'; const issue: RegexIssue = { regexOrAST: 'Regex', type: IssueTypes.GAS, - title: "`++i` costs less gas than `i++`, especially when it's used in `for`-loops (`--i`/`i--` too)", - description: '*Saves 5 gas per loop*', - regex: /[^ \t]+\+\+/g, + title: '`++i` costs less gas compared to `i++` or `i += 1` (same for `--i` vs `i--` or `i -= 1`)', + description: + 'Pre-increments and pre-decrements are cheaper.\n\nFor a `uint256 i` variable, the following is true with the Optimizer enabled at 10k:\n\n**Increment:**\n\n- `i += 1` is the most expensive form\n- `i++` costs 6 gas less than `i += 1`\n- `++i` costs 5 gas less than `i++` (11 gas less than `i += 1`)\n\n**Decrement:**\n\n- `i -= 1` is the most expensive form\n- `i--` costs 11 gas less than `i -= 1`\n- `--i` costs 5 gas less than `i--` (16 gas less than `i -= 1`)\n\nNote that post-increments (or post-decrements) return the old value before incrementing or decrementing, hence the name *post-increment*:\n\n```solidity\nuint i = 1; \nuint j = 2;\nrequire(j == i++, "This will be false as i is incremented after the comparison");\n```\n \nHowever, pre-increments (or pre-decrements) return the new value:\n \n```solidity\nuint i = 1; \nuint j = 2;\nrequire(j == ++i, "This will be true as i is incremented before the comparison");\n```\n\nIn the pre-increment case, the compiler has to create a temporary variable (when used) for returning `1` instead of `2`.\n\nConsider using pre-increments and pre-decrements where they are relevant (meaning: not where post-increments/decrements logic are relevant).\n\n*Saves 5 gas per instance*', + regex: /\S((?> 1` instead of `/ 2`\n- Use `>> 2` instead of `/ 4`\n- Use `<< 3` instead of `* 8`\n- ...\n- Use `>> 5` instead of `/ 2^5 == / 32`\n- Use `<< 6` instead of `* 2^6 == * 64`\n\nTL;DR:\n- Shifting left by N is like multiplying by 2^N (Each bits to the left is an increased power of 2)\n- Shifting right by N is like dividing by 2^N (Each bits to the right is a decreased power of 2)\n\n*Saves around 2 gas + 20 for unchecked per instance*", + regex: /(?|<=|>=)/gi, +}; + +export default issue; diff --git a/src/issues/H/get_dy_underlyingFlashLoan.ts b/src/issues/H/get_dy_underlyingFlashLoan.ts new file mode 100644 index 0000000..dc6f37f --- /dev/null +++ b/src/issues/H/get_dy_underlyingFlashLoan.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.H, + title: '`get_dy_underlying()` is not a flash-loan-resistant price', + impact: "`get_dy_underlying()` calculates the price based on the contract's underlying reserves, which can be manipulated by sandwiching the call with a flash loan. Therefore, using its output as a price oracle is not safe and will lead to loss of funds. Use a Chainlink oracle instead.", + regex: /get_dy_underlying\(/gi, +}; + + +export default issue; diff --git a/src/issues/H/msg.valueInLoop.ts b/src/issues/H/msg.valueInLoop.ts new file mode 100644 index 0000000..aa2f8aa --- /dev/null +++ b/src/issues/H/msg.valueInLoop.ts @@ -0,0 +1,27 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC, lineFromIndex } from '../../utils'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.H, + title: 'Using `msg.value` in a loop', + description: 'This is a classic dangerous pattern', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const a of findAll('ForStatement', file.ast)) { + for (const b of findAll('MemberAccess', a)) { + if (b.memberName == 'value' && b.expression && b.expression['name'] == 'msg') { + instances.push(instanceFromSRC(file, b.src)); + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/H/wstETHPriceStEth.ts b/src/issues/H/wstETHPriceStEth.ts new file mode 100644 index 0000000..c943b1f --- /dev/null +++ b/src/issues/H/wstETHPriceStEth.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.H, + title: "`wstETH`'s functions operate on units of stEth, not Eth", + impact: "`wstETH`'s functions return values related to [units of stEth](https://docs.lido.fi/contracts/wsteth/#view-methods), not units of Eth. Even after the Shanghai upgrade, the price of stETH is [not the same](https://coinmarketcap.com/currencies/steth/steth/eth/) as the prices of ETH", + regex: /(price.*\*.*WstETH.*stEthPerToken|WstETH.*stEthPerToken.*\*.*price)/gi, +}; + +export default issue; \ No newline at end of file diff --git a/src/issues/L/2stepowner.ts b/src/issues/L/2stepowner.ts new file mode 100644 index 0000000..e51d2b6 --- /dev/null +++ b/src/issues/L/2stepowner.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: 'Use a 2-step ownership transfer pattern', + description: + 'Recommend considering implementing a two step process where the owner or admin nominates an account and the nominated account needs to call an `acceptOwnership()` function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account. Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.', + regex: /is.*Ownable(?!2)/gi, +}; + +export default issue; \ No newline at end of file diff --git a/src/issues/L/NFTNoHandleHardForks.ts b/src/issues/L/NFTNoHandleHardForks.ts new file mode 100644 index 0000000..04d2618 --- /dev/null +++ b/src/issues/L/NFTNoHandleHardForks.ts @@ -0,0 +1,40 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC, lineFromIndex } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.L, + title: 'NFT doesn\'t handle hard forks', + description: + "When there are hard forks, users often have to go through [many hoops](https://twitter.com/elerium115/status/1558471934924431363) to ensure that they control ownership on every fork. Consider adding `require(1 == chain.chainId)`, or the chain ID of whichever chain you prefer, to the functions below, or at least include the chain ID in the URI, so that there is no confusion about which chain is the owner of the NFT.", + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const cd of findAll('ContractDefinition', file.ast)) { + if (cd.contractKind == 'interface') { + continue; + } + // All calls to external functions + for (const fd of findAll('FunctionDefinition', cd)) { + if (fd.name == "tokenUri") { + const inst = instanceFromSRC(file, fd.src); + let str: string = inst.fileContent.split('\n')[inst.line - 1]; + const matches: any = [...str.matchAll(/chainId/gi)]; + if (matches.length == 0) { + let fdBodyEnd = fd.body && fd.body.statements && fd.body.statements; + if (!!fdBodyEnd) { + instances.push(instanceFromSRC(file, fd.src, fdBodyEnd[fdBodyEnd.length - 1].src)); + } + } + } + } + } + } + } + return instances; + }, +}; +export default issue; diff --git a/src/issues/L/SomeERC20TransferRevertsOn0.ts b/src/issues/L/SomeERC20TransferRevertsOn0.ts new file mode 100644 index 0000000..7195619 --- /dev/null +++ b/src/issues/L/SomeERC20TransferRevertsOn0.ts @@ -0,0 +1,49 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.L, + title: 'Some tokens may revert when zero value transfers are made', + description: 'Example: https://github.com/d-xo/weird-erc20#revert-on-zero-value-transfers.\n\nIn spite of the fact that EIP-20 [states](https://github.com/ethereum/EIPs/blob/46b9b698815abbfa628cd1097311deee77dd45c5/EIPS/eip-20.md?plain=1#L116) that zero-valued transfers must be accepted, some tokens, such as LEND will revert if this is attempted, which may cause transactions that involve other tokens (such as batch operations) to fully revert. Consider skipping the transfer if the amount is zero, which will also save gas.', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const a of findAll('FunctionCall', file.ast)) { + if ( + a.expression && + (a.expression['memberName'] == 'transfer' || + a.expression['memberName'] == 'transferFrom' || + a.expression['memberName'] == 'safeTransferFrom' || + a.expression['memberName'] == 'safeTransfer') + ) { + for (const b of findAll('Identifier', a)) { + // Case of cast + if (b.name.indexOf('ERC20') > -1) { + instances.push(instanceFromSRC(file, a.src)); + } + + // Case of real type + for (const c of findAll('VariableDeclaration', file.ast)) { + if (b.name == c.name) { + for (const d of findAll('IdentifierPath', c)) { + // True only on real types, not cast + if (d.name.indexOf('ERC20') > -1) { + instances.push(instanceFromSRC(file, a.src)); + } + } + } + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/L/address0Check.ts b/src/issues/L/address0Check.ts new file mode 100644 index 0000000..8acf299 --- /dev/null +++ b/src/issues/L/address0Check.ts @@ -0,0 +1,48 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { getStorageVariable, instanceFromSRC } from '../../utils'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.L, + title: 'Missing checks for `address(0)` when assigning values to address state variables', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + + for (const file of files) { + if (!!file.ast) { + for (const contract of findAll('ContractDefinition', file.ast)) { + /** Build list of storage variables */ + let storageVariables = getStorageVariable(contract); + + for (const func of findAll('FunctionDefinition', contract)) { + for (const assign of findAll('Assignment', func)) { + if ( + assign.leftHandSide.nodeType === 'Identifier' && + storageVariables.includes(assign.leftHandSide.name) && + assign.leftHandSide.typeDescriptions.typeString === 'address' && + assign.rightHandSide.nodeType === 'Identifier' + ) { + const assignName = assign.rightHandSide.name; + let check = false; + for (const test of findAll('BinaryOperation', func)) { + if ( + (test.operator === '!=' || test.operator === '==') && + ((test.rightExpression.nodeType === 'Identifier' && test.rightExpression.name === assignName) || + (test.leftExpression.nodeType === 'Identifier' && test.leftExpression.name === assignName)) + ) { + check = true; + } + } + if (!check) instances.push(instanceFromSRC(file, assign.src)); + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/L/avoidEcrecover.ts b/src/issues/L/avoidEcrecover.ts new file mode 100644 index 0000000..187cb14 --- /dev/null +++ b/src/issues/L/avoidEcrecover.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: 'Use of `ecrecover` is susceptible to signature malleability', + description: + 'The built-in EVM precompile `ecrecover` is susceptible to signature malleability, which could lead to replay attacks.\nReferences: , , and .\nWhile this is not immediately exploitable, this may become a vulnerability if used elsewhere.', + regex: /ecrecover\(/gi, +}; + +export default issue; diff --git a/src/issues/L/avoidEncodePacked.ts b/src/issues/L/avoidEncodePacked.ts index 654070b..590ce7a 100644 --- a/src/issues/L/avoidEncodePacked.ts +++ b/src/issues/L/avoidEncodePacked.ts @@ -1,13 +1,50 @@ -import { IssueTypes, RegexIssue } from '../../types'; +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; -const issue: RegexIssue = { - regexOrAST: 'Regex', +const issue: ASTIssue = { + regexOrAST: 'AST', type: IssueTypes.L, title: - ' `abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()`', + '`abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()`', description: 'Use `abi.encode()` instead which will pad items to 32 bytes, which will [prevent hash collisions](https://docs.soliditylang.org/en/v0.8.13/abi-spec.html#non-standard-packed-mode) (e.g. `abi.encodePacked(0x123,0x456)` => `0x123456` => `abi.encodePacked(0x1,0x23456)`, but `abi.encode(0x123,0x456)` => `0x0...1230...456`). "Unless there is a compelling reason, `abi.encode` should be preferred". If there is only one argument to `abi.encodePacked()` it can often be cast to `bytes()` or `bytes32()` [instead](https://ethereum.stackexchange.com/questions/30912/how-to-compare-strings-in-solidity#answer-82739).\nIf all arguments are strings and or bytes, `bytes.concat()` should be used instead', - regex: /keccak(256)?.?\(abi.encodePacked/g, + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const a of findAll('FunctionCall', file.ast)) { + if ( + a.expression && + (a.expression['name'] == 'encodePacked' || a.expression['memberName'] == 'encodePacked') + ) { + for (const b of a.arguments) { + if (b.typeDescriptions && b.typeDescriptions.typeIdentifier && ( + (b.typeDescriptions.typeIdentifier.indexOf('t_string') > -1) + || (b.typeDescriptions.typeIdentifier.indexOf('t_struct') > -1) + || (b.typeDescriptions.typeIdentifier.indexOf('t_bytes_') > -1) + || (b.typeDescriptions.typeIdentifier.indexOf('_memory_') > -1) + || (b.typeDescriptions.typeIdentifier.indexOf('_calldata_') > -1) + )) { + const inst = instanceFromSRC(file, b.src); + instances.push(inst); + } + if (b["arguments"]) { + for (const c of b["arguments"]) { + if (c.typeDescriptions && c.typeDescriptions.typeIdentifier && c.typeDescriptions.typeIdentifier.indexOf('_struct') > -1) { + const inst = instanceFromSRC(file, c.src); + instances.push(inst); + } + } + } + } + } + } + } + } + return instances; + }, }; export default issue; diff --git a/src/issues/L/calc_token_amountAlreadyHasSlippage.ts b/src/issues/L/calc_token_amountAlreadyHasSlippage.ts new file mode 100644 index 0000000..40c2268 --- /dev/null +++ b/src/issues/L/calc_token_amountAlreadyHasSlippage.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: '`calc_token_amount()` has slippage added on top of Curve\'s calculated slippage', + description: + 'According to the Curve [docs](https://curve.readthedocs.io/_/downloads/en/latest/pdf/), `StableSwap.calc_token_amount()` already includes slippage but not fees, so adding extra slippage on top of the returned result, as is done by the caller of functions higher up the caller chain, is an incorrect operation.', + regex: /\.calc_token_amount\(/gi, +}; + +export default issue; diff --git a/src/issues/L/decimalsNotERC20.ts b/src/issues/L/decimalsNotERC20.ts new file mode 100644 index 0000000..b1f536b --- /dev/null +++ b/src/issues/L/decimalsNotERC20.ts @@ -0,0 +1,13 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: '`decimals()` is not a part of the ERC-20 standard', + description: + "The `decimals()` function is not a part of the [ERC-20 standard](https://eips.ethereum.org/EIPS/eip-20), and was added later as an [optional extension](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/IERC20Metadata.sol). As such, some valid ERC20 tokens do not support this interface, so it is unsafe to blindly cast all tokens to this interface, and then call this function.", + + regex: /\.decimals\(\)/gi, +}; + +export default issue; diff --git a/src/issues/L/decimalsNotUint8.ts b/src/issues/L/decimalsNotUint8.ts new file mode 100644 index 0000000..e577512 --- /dev/null +++ b/src/issues/L/decimalsNotUint8.ts @@ -0,0 +1,10 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: '`decimals()` should be of type `uint8`', + regex: /uint(?!8)(?!.*(\/\/|;)).*decimals/gi, +}; + +export default issue; diff --git a/src/issues/L/deprecatedApprove.ts b/src/issues/L/deprecatedApprove.ts new file mode 100644 index 0000000..7585617 --- /dev/null +++ b/src/issues/L/deprecatedApprove.ts @@ -0,0 +1,31 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.L, + title: 'Deprecated approve() function', + description: + "Due to the inheritance of ERC20's approve function, there's a vulnerability to the ERC20 approve and double spend front running attack. Briefly, an authorized spender could spend both allowances by front running an allowance-changing transaction. Consider implementing OpenZeppelin's `.safeApprove()` function to help mitigate this.", + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const fd of findAll('FunctionDefinition', file.ast)) { + if (fd.kind == 'constructor') { + continue; + } + for (const a of findAll('FunctionCall', fd)) { + if (a.expression && a.expression['memberName'] == 'approve') { + instances.push(instanceFromSRC(file, a.src)); + } + } + } + } + } + return instances; + }, +}; +export default issue; diff --git a/src/issues/L/deprecatedSafeApprove.ts b/src/issues/L/deprecatedSafeApprove.ts new file mode 100644 index 0000000..e6e152c --- /dev/null +++ b/src/issues/L/deprecatedSafeApprove.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: '`safeApprove()` is deprecated', + description: + "[Deprecated](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/bfff03c0d2a59bcd8e2ead1da9aed9edf0080d05/contracts/token/ERC20/utils/SafeERC20.sol#L38-L45) in favor of `safeIncreaseAllowance()` and `safeDecreaseAllowance()`. If only setting the initial allowance to the value that means infinite, `safeIncreaseAllowance()` can be used instead. The function may currently work, but if a bug is found in this version of OpenZeppelin, and the version that you're forced to upgrade to no longer has this function, you'll encounter unnecessary delays in porting and testing replacement contracts.", + regex: /\.safeApprove\(/gi, +}; + +export default issue; diff --git a/src/issues/L/deprecatedSetupRole.ts b/src/issues/L/deprecatedSetupRole.ts new file mode 100644 index 0000000..cabc6cf --- /dev/null +++ b/src/issues/L/deprecatedSetupRole.ts @@ -0,0 +1,10 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: 'Deprecated _setupRole() function', + regex: /_setupRole\(/gi, +}; + +export default issue; diff --git a/src/issues/L/disableInitImpl.ts b/src/issues/L/disableInitImpl.ts new file mode 100644 index 0000000..052852a --- /dev/null +++ b/src/issues/L/disableInitImpl.ts @@ -0,0 +1,42 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.L, + title: 'Do not leave an implementation contract uninitialized', + description: + 'An uninitialized implementation contract can be taken over by an attacker, which may impact the proxy. To prevent the implementation contract from being used, it\'s advisable to invoke the `_disableInitializers` function in the constructor to automatically lock it when it is deployed. This should look similar to this:\n```solidity\n /// @custom:oz-upgrades-unsafe-allow constructor\n constructor() {\n _disableInitializers();\n }\n```\n\nSources:\n- https://docs.openzeppelin.com/contracts/4.x/api/proxy#Initializable-_disableInitializers--\n- https://twitter.com/0xCygaar/status/1621417995905167360?s=20', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + + for (const file of files) { + if (!!file.ast) { + let isDisabled = false; + let constructorNodeSrc; + let foundInitialize = false; + for (const node of findAll('FunctionDefinition', file.ast)) { + if (node.name === 'initialize') { + foundInitialize = true; + } + + if (node.kind === 'constructor') { + constructorNodeSrc = node.src; + for (const node2 of findAll('Identifier', node)) { + if (node2.name === '_disableInitializers') { + isDisabled = true; + } + } + } + } + if (!isDisabled && constructorNodeSrc && foundInitialize) { + instances.push(instanceFromSRC(file, constructorNodeSrc)); + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/L/div0NotPrevented.ts b/src/issues/L/div0NotPrevented.ts new file mode 100644 index 0000000..fcd61d5 --- /dev/null +++ b/src/issues/L/div0NotPrevented.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: 'Division by zero not prevented', + description: + 'The divisions below take an input parameter which does not have any zero-value checks, which may lead to the functions reverting when zero is passed.', + regex: /(?.\nRemediation: Consider using the [implementation](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/EIP712.sol#L77-L90) from OpenZeppelin, which recalculates the domain separator if the current `block.chainid` is not the cached chain ID.\nPast occurrences of this issue:\n- [Reality Cards Contest](https://github.com/code-423n4/2021-06-realitycards-findings/issues/166)\n- [Swivel Contest](https://github.com/code-423n4/2021-09-swivel-findings/issues/98)\n- [Malt Finance Contest](https://github.com/code-423n4/2021-11-malt-findings/issues/349)", + regex: /domain.?separator/gi, +}; + +export default issue; \ No newline at end of file diff --git a/src/issues/L/duplicateImports.ts b/src/issues/L/duplicateImports.ts new file mode 100644 index 0000000..348d20e --- /dev/null +++ b/src/issues/L/duplicateImports.ts @@ -0,0 +1,32 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +// regex: /(\{\})|(\{ \})/gi, +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.L, + title: 'Duplicate import statements', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const b of findAll('ImportDirective', file.ast)) { + let counter = 0; + for (const c of findAll('ImportDirective', file.ast)) { + if (b.file == c.file) { + counter++; + if (counter > 1) { + instances.push(instanceFromSRC(file, b.src)); + instances.push(instanceFromSRC(file, c.src)); + } + } + } + } + } + } + return instances; + }, +}; +export default issue; diff --git a/src/issues/L/emptyBody.ts b/src/issues/L/emptyBody.ts index 2701254..be7e2ab 100644 --- a/src/issues/L/emptyBody.ts +++ b/src/issues/L/emptyBody.ts @@ -1,10 +1,35 @@ -import { IssueTypes, RegexIssue } from '../../types'; +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; -const issue: RegexIssue = { - regexOrAST: 'Regex', +// regex: /(\{\})|(\{ \})/gi, +const issue: ASTIssue = { + regexOrAST: 'AST', type: IssueTypes.L, title: 'Empty Function Body - Consider commenting why', - regex: /(\{\})|(\{ \})/g, + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const cd of findAll('ContractDefinition', file.ast)) { + if (cd.contractKind == 'interface') { + continue; + } + for (const fd of findAll('FunctionDefinition', cd)) { + if (fd.kind == 'constructor' || fd.virtual || fd.kind == 'fallback' || fd.kind == 'receive') { + continue; + } + if (!fd.body || !fd.body.statements || fd.body.statements.length == 0) { + if (!fd.documentation || !fd.documentation.text || fd.documentation.text.length == 0) { + instances.push(instanceFromSRC(file, fd.src)); + } + } + } + } + } + } + return instances; + }, }; - export default issue; diff --git a/src/issues/L/emptyFallbackRequire.ts b/src/issues/L/emptyFallbackRequire.ts new file mode 100644 index 0000000..74234e2 --- /dev/null +++ b/src/issues/L/emptyFallbackRequire.ts @@ -0,0 +1,37 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +// regex: /(\{\})|(\{ \})/gi, +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.L, + title: 'Empty `receive()/payable fallback()` function does not authenticate requests', + description: + 'If the intention is for the Ether to be used, the function should call another function, otherwise it should revert (e.g. require(msg.sender == address(weth))). Having no access control on the function means that someone may send Ether to the contract, and have no way to get anything back out, which is a loss of funds. If the concern is having to spend a small amount of gas to check the sender against an immutable address, the code should at least have a function to rescue unused Ether.', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const cd of findAll('ContractDefinition', file.ast)) { + if (cd.contractKind == 'interface') { + continue; + } + for (const fd of findAll('FunctionDefinition', cd)) { + if (fd.kind == 'constructor' || fd.virtual) { + continue; + } + if (fd.kind == 'fallback' || fd.kind == 'receive') { + if (!fd.body || !fd.body.statements || fd.body.statements.length == 0) { + instances.push(instanceFromSRC(file, fd.src)); + } + } + } + } + } + } + return instances; + }, +}; +export default issue; diff --git a/src/issues/L/extCallLoop.ts b/src/issues/L/extCallLoop.ts new file mode 100644 index 0000000..653b046 --- /dev/null +++ b/src/issues/L/extCallLoop.ts @@ -0,0 +1,33 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { getStorageVariable, instanceFromSRC } from '../../utils'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.L, + title: 'External calls in an un-bounded `for-`loop may result in a DOS', + description: 'Consider limiting the number of iterations in for-loops that make external calls', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + + for (const file of files) { + if (!!file.ast) { + for (const forloop of findAll('ForStatement', file.ast)) { + for (const functionCall of findAll('FunctionCall', forloop)) { + if ( + functionCall.expression && + functionCall.expression.nodeType == 'MemberAccess' && + functionCall.expression.expression && + functionCall.expression.expression.nodeType == 'IndexAccess' + ) { + instances.push(instanceFromSRC(file, functionCall.src)); + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/L/extCallRecipientGas.ts b/src/issues/L/extCallRecipientGas.ts new file mode 100644 index 0000000..6819d0c --- /dev/null +++ b/src/issues/L/extCallRecipientGas.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: 'External call recipient may consume all transaction gas', + description: + "There is no limit specified on the amount of gas used, so the recipient can use up all of the transaction's gas, causing it to revert. Use `addr.call{gas: }(\"\")` or [this](https://github.com/nomad-xyz/ExcessivelySafeCall) library instead.", + regex: /(?)=? block.timestamp/gi, +}; + +export default issue; diff --git a/src/issues/L/lackSlippage.ts b/src/issues/L/lackSlippage.ts new file mode 100644 index 0000000..a2f9862 --- /dev/null +++ b/src/issues/L/lackSlippage.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: 'Lack of Slippage check', + regexPreCondition: /UniswapV3Pool|UniswapV2Pool|UniswapV4Pool/gi, + regex: /amountOutMin.*:.*0/gi, +}; + +export default issue; diff --git a/src/issues/L/mathmaxInt.ts b/src/issues/L/mathmaxInt.ts new file mode 100644 index 0000000..fd16f80 --- /dev/null +++ b/src/issues/L/mathmaxInt.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: '`Math.max(,0)` used with `int` cast to `uint`', + description: + "The code casts an `int` to a `uint` before passing it to `Math.max()`. It seems as though the `Math.max()` call is attempting to prevent values from being negative, but since the `int` is being cast to `uint`, the value will never be negative, and instead will overflow if either the multiplication involving the slope and timestamp is positive. I wasn't able to find a scenario where this is the case, but this seems very dangerous, and the `Math.max()` call is sending misleading signals, so I suggest moving it to inside the cast to `uint`", + regex: /Math.max\(uint.*\(int.*/gi, +}; + +export default issue; diff --git a/src/issues/L/mintAndBurnAddress0.ts b/src/issues/L/mintAndBurnAddress0.ts new file mode 100644 index 0000000..13ee6b0 --- /dev/null +++ b/src/issues/L/mintAndBurnAddress0.ts @@ -0,0 +1,28 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.L, + title: 'Prevent accidentally burning tokens', + description: 'Minting and burning tokens to address(0) prevention', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const a of findAll('FunctionCall', file.ast)) { + for (const b of findAll('Identifier', a)) { + if (b.name.indexOf('mint') > -1 || b.name.indexOf('burn') > -1) { + instances.push(instanceFromSRC(file, a.src)); + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/L/nftOwnershipHardfork.ts b/src/issues/L/nftOwnershipHardfork.ts new file mode 100644 index 0000000..b911476 --- /dev/null +++ b/src/issues/L/nftOwnershipHardfork.ts @@ -0,0 +1,40 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.L, + title: 'NFT ownership doesn\'t support hard forks', + description: + 'To ensure clarity regarding the ownership of the NFT on a specific chain, it is recommended to add `require(block.chainid == 1, "Invalid Chain")` or the desired chain ID in the functions below.\n\nAlternatively, consider including the chain ID in the URI itself. By doing so, any confusion regarding the chain responsible for owning the NFT will be eliminated.', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + + for (const file of files) { + if (!!file.ast) { + for (const a of findAll('FunctionDefinition', file.ast)) { + if (a.name === 'tokenURI') { + let isProtected = false; + for (const b of findAll('IfStatement', a)) { + isProtected = true; + } + + for (const b of findAll('Identifier', a)) { + if (b.name == 'require') { + isProtected = true; + } + } + if (!isProtected) { + let endLine = '' + (a.body && a.body.statements && a.body.statements[a.body.statements.length - 1].src); + instances.push(instanceFromSRC(file, a.src, endLine)); + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/L/ownerRenounceDuringPause.ts b/src/issues/L/ownerRenounceDuringPause.ts new file mode 100644 index 0000000..85909d8 --- /dev/null +++ b/src/issues/L/ownerRenounceDuringPause.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: 'Owner can renounce while system is paused', + description: "The contract owner or single user with a role is not prevented from renouncing the role/ownership while the contract is paused, which would cause any user assets stored in the protocol, to be locked indefinitely.", + regex: /pause.+onlyowner/gi, +}; + +export default issue; diff --git a/src/issues/L/possibleRounding.ts b/src/issues/L/possibleRounding.ts new file mode 100644 index 0000000..db2edb0 --- /dev/null +++ b/src/issues/L/possibleRounding.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: 'Possible rounding issue', + description: + 'Division by large numbers may result in the result being zero, due to solidity not supporting fractions. Consider requiring a minimum amount for the numerator to ensure that it is always larger than the denominator. Also, there is indication of multiplication and division without the use of parenthesis which could result in issues.', + regex: / \/ .*(reserve|balance|supply|total)/gi, +}; + +export default issue; diff --git a/src/issues/L/pragmaExpDeprecated.ts b/src/issues/L/pragmaExpDeprecated.ts new file mode 100644 index 0000000..1eb7b7b --- /dev/null +++ b/src/issues/L/pragmaExpDeprecated.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: '`pragma experimental ABIEncoderV2` is deprecated', + description: + 'Use `pragma abicoder v2` [instead](https://github.com/ethereum/solidity/blob/69411436139acf5dbcfc5828446f18b9fcfee32c/docs/080-breaking-changes.rst#silent-changes-of-the-semantics)', + regex: /pragma experimental ABIEncoderV2/gi, +}; + +export default issue; diff --git a/src/issues/L/precisionLoss.ts b/src/issues/L/precisionLoss.ts new file mode 100644 index 0000000..bd4747c --- /dev/null +++ b/src/issues/L/precisionLoss.ts @@ -0,0 +1,13 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +//@note there will be false positives +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: 'Loss of precision', + description: + 'Division by large numbers may result in the result being zero, due to solidity not supporting fractions. Consider requiring a minimum amount for the numerator to ensure that it is always larger than the denominator', + regex: /(? { + let instances: Instance[] = []; + + for (const file of files) { + if (!!file.ast) { + let shouldBreak = false; + for (const a of findAll('ContractDefinition', file.ast)) { + if (a.contractKind == 'interface' || a.abstract) { + shouldBreak = true; + break; + } + } + if (shouldBreak) { + continue; + } + for (const node of findAll('PragmaDirective', file.ast)) { + // Look for Address(X).balance + let indexOf0point8 = node.literals.indexOf('0.8'); + let indexOfFloating = node.literals.indexOf('^'); + let indexOfSup = node.literals.findIndex(e => e.indexOf(">") > - 1); + let indexOf20Plus = node.literals.findIndex(e => e.indexOf("2") > - 1); + if ( + indexOf0point8 > -1 && (indexOfFloating - 1 || indexOfSup - 1 || indexOf20Plus > indexOf0point8) + ) { + instances.push(instanceFromSRC(file, node.src)); + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/L/safeTransferOwnership.ts b/src/issues/L/safeTransferOwnership.ts new file mode 100644 index 0000000..ae90414 --- /dev/null +++ b/src/issues/L/safeTransferOwnership.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: 'Use `Ownable2Step.transferOwnership` instead of `Ownable.transferOwnership`', + description: + 'Use [Ownable2Step.transferOwnership](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable2Step.sol) which is safer. Use it as it is more secure due to 2-stage ownership transfer.\n\n**Recommended Mitigation Steps**\n\nUse Ownable2Step.sol\n \n ```solidity\n function acceptOwnership() external {\n address sender = _msgSender();\n require(pendingOwner() == sender, "Ownable2Step: caller is not the new owner");\n _transferOwnership(sender);\n }\n```', + regex: /import.*Ownable(?!2)|transferOwnership/gi, +}; + +export default issue; \ No newline at end of file diff --git a/src/issues/L/solc1314OptimizerBug.ts b/src/issues/L/solc1314OptimizerBug.ts new file mode 100644 index 0000000..2b5ebd0 --- /dev/null +++ b/src/issues/L/solc1314OptimizerBug.ts @@ -0,0 +1,47 @@ +import { InputType, IssueTypes, Instance, ASTIssue } from '../../types'; +import { findAll } from 'solidity-ast/utils'; +import { instanceFromSRC } from '../../utils'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.L, + title: 'File allows a version of solidity that is susceptible to an assembly optimizer bug', + description: + 'In solidity versions 0.8.13 and 0.8.14, there is an [optimizer bug](https://github.com/ethereum/solidity-blog/blob/499ab8abc19391be7b7b34f88953a067029a5b45/_posts/2022-06-15-inline-assembly-memory-side-effects-bug.md) where, if the use of a variable is in a separate `assembly` block from the block in which it was stored, the `mstore` operation is optimized out, leading to uninitialized memory. The code currently does not have such a pattern of execution, but it does use `mstore`s in `assembly` blocks, so it is a risk for future changes. The affected solidity versions should be avoided if at all possible.', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + + for (const file of files) { + if (!!file.ast) { + let shouldBreak = false; + for (const a of findAll('ContractDefinition', file.ast)) { + if (a.contractKind == 'interface' || a.abstract) { + shouldBreak = true; + break; + } + } + if (shouldBreak) { + continue; + } + for (const node of findAll('PragmaDirective', file.ast)) { + let indexOf0point8 = node.literals.indexOf('0.8'); + let indexOfFloating = node.literals.indexOf('^'); + let indexOfSup = node.literals.findIndex(e => e.indexOf(">") > - 1); + let indexOf20Plus = node.literals.findIndex(e => e.indexOf("2") > - 1); + let hasMinVer = !!node.literals[indexOf0point8 + 1]; + let isLess15 = hasMinVer && parseInt(node.literals[indexOf0point8 + 1].replaceAll(".","")) < 15; + let is13 = hasMinVer && parseInt(node.literals[indexOf0point8 + 1].replaceAll(".","")) == 13; + let is14 = hasMinVer && parseInt(node.literals[indexOf0point8 + 1].replaceAll(".","")) == 14; + if ( + indexOf0point8 > -1 && ((is13 || is14) || ((indexOfFloating - 1 || indexOfSup - 1) && isLess15)) + ) { + instances.push(instanceFromSRC(file, node.src)); + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/L/sweepingRescuing.ts b/src/issues/L/sweepingRescuing.ts new file mode 100644 index 0000000..6e47f2f --- /dev/null +++ b/src/issues/L/sweepingRescuing.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: 'Sweeping may break accounting if tokens with multiple addresses are used', + description: "There have been [cases](https://blog.openzeppelin.com/compound-tusd-integration-issue-retrospective/) in the past where a token mistakenly had two addresses that could control its balance, and transfers using one address impacted the balance of the other. To protect against this potential scenario, sweep functions should ensure that the balance of the non-sweepable token does not change after the transfer of the swept tokens.", + regex: /sweep|recover(ERC|Token)|rescue(ERC|Token)/gi, +}; + +export default issue; diff --git a/src/issues/L/symbolNotERC20.ts b/src/issues/L/symbolNotERC20.ts new file mode 100644 index 0000000..18aad60 --- /dev/null +++ b/src/issues/L/symbolNotERC20.ts @@ -0,0 +1,13 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: '`symbol()` is not a part of the ERC-20 standard', + description: + "The `symbol()` function is not a part of the [ERC-20 standard](https://eips.ethereum.org/EIPS/eip-20), and was added later as an [optional extension](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/IERC20Metadata.sol). As such, some valid ERC20 tokens do not support this interface, so it is unsafe to blindly cast all tokens to this interface, and then call this function.", + + regex: /\.symbol\(\)/gi, +}; + +export default issue; diff --git a/src/issues/L/unsafeCasting.ts b/src/issues/L/unsafeCasting.ts new file mode 100644 index 0000000..b082f63 --- /dev/null +++ b/src/issues/L/unsafeCasting.ts @@ -0,0 +1,54 @@ +// regex: /(u| |-)int(?!256)\d?\d?\d?\(/g, +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.L, + title: + 'Consider using OpenZeppelin\'s SafeCast library to prevent unexpected overflows when downcasting', + description: + "Downcasting from `uint256`/`int256` in Solidity does not revert on overflow. This can result in undesired exploitation or bugs, since developers usually assume that overflows raise errors. [OpenZeppelin's SafeCast library](https://docs.openzeppelin.com/contracts/3.x/api/utils#SafeCast) restores this intuition by reverting the transaction when such an operation overflows. Using this library eliminates an entire class of bugs, so it's recommended to use it always. Some exceptions are acceptable like with the classic `uint256(uint160(address(variable)))`", + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const a of findAll('FunctionCall', file.ast)) { + if (a.kind != 'typeConversion') { + continue; + } + let shouldSkip = false; + for (const b of findAll('Identifier', a)) { + if (b.name.indexOf('block') > -1 || b.name.indexOf('time') > -1 || b.name.indexOf('Time') > -1) { + shouldSkip = true; + break; + } + } + if (shouldSkip) { + continue; + } + + for (const b of findAll('ElementaryTypeNameExpression', a)) { + if ( + b.argumentTypes && + b.argumentTypes[0] && + (b.argumentTypes[0].typeString == 'uint256' || b.argumentTypes[0].typeString == 'int256') + ) { + const inst = instanceFromSRC(file, a.src); + let str: string = inst.fileContent.split('\n')[inst.line - 1]; + const matches: any = [...str.matchAll(/(u| |-)int(?!256)\d\d?\d?\((?!.*(int256|block|time))/gi)]; + if (matches.length > 0) { + instances.push(inst); + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/L/unsafeReturnBomb.ts b/src/issues/L/unsafeReturnBomb.ts new file mode 100644 index 0000000..66b513f --- /dev/null +++ b/src/issues/L/unsafeReturnBomb.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: 'Unsafe solidity low-level call can cause gas grief attack', + description: "Using the low-level calls of a solidity address can leave the contract open to gas grief attacks. These attacks occur when the called contract returns a large amount of data.\n\nSo when calling an external contract, it is necessary to check the length of the return data before reading/copying it (using `returndatasize()`).", + regex: /bytes .+\.call\(/gi, +}; + +export default issue; diff --git a/src/issues/L/upgradeableMissingGap.ts b/src/issues/L/upgradeableMissingGap.ts new file mode 100644 index 0000000..9a770f9 --- /dev/null +++ b/src/issues/L/upgradeableMissingGap.ts @@ -0,0 +1,13 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: + 'Upgradeable contract is missing a `__gap[50]` storage variable to allow for new storage variables in later versions', + description: + 'See [this](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps) link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.', + regex: /Upgradeable/gi, +}; + +export default issue; diff --git a/src/issues/L/upgradeableNotInit.ts b/src/issues/L/upgradeableNotInit.ts new file mode 100644 index 0000000..b626617 --- /dev/null +++ b/src/issues/L/upgradeableNotInit.ts @@ -0,0 +1,14 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +//@audit-issue TODO - lots of false positives +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: 'Upgradeable contract not initialized', + description: + 'Upgradeable contracts are initialized via an initializer function rather than by a constructor. Leaving such a contract uninitialized may lead to it being taken over by a malicious user', + regexPreCondition: /import.+Upgradeable|/gi, + regex: /initialize|Upgradeable|_init\(/gi, +}; + +export default issue; diff --git a/src/issues/L/useOnlyInitializing.ts b/src/issues/L/useOnlyInitializing.ts new file mode 100644 index 0000000..e844413 --- /dev/null +++ b/src/issues/L/useOnlyInitializing.ts @@ -0,0 +1,33 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.L, + title: 'Use `initializer` for public-facing functions only. Replace with `onlyInitializing` on internal functions.', + description: + 'See [What\'s the difference between onlyInitializing and initializer](https://forum.openzeppelin.com/t/whats-the-difference-between-onlyinitializing-and-initialzer/25789) and https://docs.openzeppelin.com/contracts/4.x/api/proxy#Initializable-onlyInitializing--', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const cd of findAll('ContractDefinition', file.ast)) { + if (cd.contractKind == 'interface') { + continue; + } + for (const a of findAll('FunctionDefinition', file.ast)) { + let modifierIndex = a.modifiers && a.modifiers.findIndex(e => e.modifierName && e.modifierName.name && e.modifierName.name.indexOf("initializer") > -1); + if (a.visibility == 'internal' && modifierIndex > -1) { + instances.push(instanceFromSRC(file, a.modifiers[modifierIndex].src)); + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/L/year365.ts b/src/issues/L/year365.ts new file mode 100644 index 0000000..21d47e0 --- /dev/null +++ b/src/issues/L/year365.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: 'A year is not always 365 days', + description: + "On leap years, the number of days is 366, so calculations during those years will return the wrong value", + regex: /year.+=.+365/gi, +}; + +export default issue; diff --git a/src/issues/M/FoTTokens.ts b/src/issues/M/FoTTokens.ts new file mode 100644 index 0000000..1dad0d2 --- /dev/null +++ b/src/issues/M/FoTTokens.ts @@ -0,0 +1,57 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.M, + title: 'Contracts are vulnerable to fee-on-transfer accounting-related issues', + description: + "Consistently check account balance before and after transfers for Fee-On-Transfer discrepancies. As arbitrary ERC20 tokens can be used, the amount here should be calculated every time to take into consideration a possible fee-on-transfer or deflation.\nAlso, it's a good practice for the future of the solution.\n\nUse the balance before and after the transfer to calculate the received amount instead of assuming that it would be equal to the amount passed as a parameter. Or explicitly document that such tokens shouldn't be used and won't be supported", + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const a of findAll('FunctionCall', file.ast)) { + if ( + a.expression && + (a.expression['memberName'] == 'transferFrom' || + a.expression['memberName'] == 'safeTransferFrom') + ) { + for (const b of findAll('Identifier', a)) { + // Case of cast + if (b.name.indexOf('ERC20') > -1) { + const inst = instanceFromSRC(file, a.src); + let str: string = inst.fileContent.split('\n')[inst.line - 1]; + const matches: any = [...str.matchAll(/address\(this/gi)]; + if (matches.length > 0) { + instances.push(inst); + } + } + + // Case of real type + for (const c of findAll('VariableDeclaration', file.ast)) { + if (b.name == c.name) { + for (const d of findAll('IdentifierPath', c)) { + if (d.name.indexOf('ERC20') > -1) { + const inst = instanceFromSRC(file, a.src); + let str: string = inst.fileContent.split('\n')[inst.line - 1]; + const matches: any = [...str.matchAll(/address\(this/gi)]; + if (matches.length > 0) { + instances.push(inst); + } + } + } + } + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/M/NFTRedefinesMint.ts b/src/issues/M/NFTRedefinesMint.ts new file mode 100644 index 0000000..47c0c19 --- /dev/null +++ b/src/issues/M/NFTRedefinesMint.ts @@ -0,0 +1,42 @@ +import { ContractDefinition } from 'solidity-ast'; +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC, topLevelFiles, getStorageVariable } from '../../utils'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.L, + title: "NFT contract redefines `_mint()`/`_safeMint()`, but not both", + description: + "If one of the functions is re-implemented, or has new arguments, the other should be as well. The `_mint()` variant is supposed to skip `onERC721Received()` checks, whereas `_safeMint()` does not. Not having both points to a possible issue with spec-compatibility.", + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + + for (const file of files) { + if (!!file.ast) { + let isMint = 0; + let isSafeMint = 0; + for (const fd of findAll('FunctionDefinition', file.ast)) { + if (!!fd.overrides) { + if (fd.name == "_mint") { + isMint = 1; + } + if (fd.name == "_safeMint") { + isSafeMint = 1; + } + + if ((isMint + isSafeMint) == 1) { + let fdBodyEnd = fd.body && fd.body.statements && fd.body.statements; + if (!!fdBodyEnd) { + instances.push(instanceFromSRC(file, fd.src, fdBodyEnd[fdBodyEnd.length - 1].src)); + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/M/approve0first.ts b/src/issues/M/approve0first.ts new file mode 100644 index 0000000..6df546d --- /dev/null +++ b/src/issues/M/approve0first.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: "`approve()`/`safeApprove()` may revert if the current approval is not zero", + description: + '- Some tokens (like the *very popular* USDT) do not work when changing the allowance from an existing non-zero allowance value (it will revert if the current approval is not zero to protect against front-running changes of approvals). These tokens must first be approved for zero and then the actual allowance can be approved.\n- Furthermore, OZ\'s implementation of safeApprove would throw an error if an approve is attempted from a non-zero value (`"SafeERC20: approve from non-zero to non-zero allowance"`)\n\nSet the allowance to zero immediately before each of the existing allowance calls', + regex: /\.(safe)?approve\(/gi, +}; + +export default issue; \ No newline at end of file diff --git a/src/issues/M/avoidTx.origin.ts b/src/issues/M/avoidTx.origin.ts new file mode 100644 index 0000000..dc03020 --- /dev/null +++ b/src/issues/M/avoidTx.origin.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.L, + title: 'Use of `tx.origin` is unsafe in almost every context', + description: + 'According to [Vitalik Buterin](https://ethereum.stackexchange.com/questions/196/how-do-i-make-my-dapp-serenity-proof), contracts should _not_ `assume that tx.origin will continue to be usable or meaningful`. An example of this is [EIP-3074](https://eips.ethereum.org/EIPS/eip-3074#allowing-txorigin-as-signer-1) which explicitly mentions the intention to change its semantics when it\'s used with new op codes. There have also been calls to [remove](https://github.com/ethereum/solidity/issues/683) `tx.origin`, and there are [security issues](solidity.readthedocs.io/en/v0.4.24/security-considerations.html#tx-origin) associated with using it for authorization. For these reasons, it\'s best to completely avoid the feature.', + regex: /tx\.origin/gi, +}; + +export default issue; diff --git a/src/issues/M/blockNumberL2.ts b/src/issues/M/blockNumberL2.ts new file mode 100644 index 0000000..a19103d --- /dev/null +++ b/src/issues/M/blockNumberL2.ts @@ -0,0 +1,13 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.M, + title: + "`block.number` means different things on different L2s", + description: + "On Optimism, `block.number` is the L2 block number, but on Arbitrum, it's the L1 block number, and `ArbSys(address(100)).arbBlockNumber()` must be used. Furthermore, L2 block numbers often occur much more frequently than L1 block numbers (any may even occur on a per-transaction basis), so using block numbers for timing results in inconsistencies, especially when voting is involved across multiple chains. As of version 4.9, OpenZeppelin has [modified](https://blog.openzeppelin.com/introducing-openzeppelin-contracts-v4.9#governor) their governor code to use a clock rather than block numbers, to avoid these sorts of issues, but this still requires that the project [implement](https://docs.openzeppelin.com/contracts/4.x/governance#token_2) a [clock](https://eips.ethereum.org/EIPS/eip-6372) for each L2.", + regex: /block\.number/gi, +}; + +export default issue; diff --git a/src/issues/M/deprecatedTransfer.ts b/src/issues/M/deprecatedTransfer.ts new file mode 100644 index 0000000..4219157 --- /dev/null +++ b/src/issues/M/deprecatedTransfer.ts @@ -0,0 +1,27 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.M, + title: '`call()` should be used instead of `transfer()` on an `address payable`', + description: + 'The use of the deprecated `transfer()` function for an address may make the transaction fail due to the 2300 gas stipend', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const b of findAll('FunctionCall', file.ast)) { + if (b.expression && b.expression['memberName'] == 'transfer' && b.arguments.length == 1) { + instances.push(instanceFromSRC(file, b.src)); + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/M/erc721safeMint.ts b/src/issues/M/erc721safeMint.ts new file mode 100644 index 0000000..39ec7c0 --- /dev/null +++ b/src/issues/M/erc721safeMint.ts @@ -0,0 +1,13 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.M, + title: '`_safeMint()` should be used rather than `_mint()` wherever possible', + description: + "`_mint()` is [discouraged](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/d4d8d2ed9798cc3383912a23b5e8d5cb602f7d4b/contracts/token/ERC721/ERC721.sol#L271) in favor of `_safeMint()` which ensures that the recipient is either an EOA or implements `IERC721Receiver`. Both open [OpenZeppelin](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/d4d8d2ed9798cc3383912a23b5e8d5cb602f7d4b/contracts/token/ERC721/ERC721.sol#L238-L250) and [solmate](https://github.com/Rari-Capital/solmate/blob/4eaf6b68202e36f67cab379768ac6be304c8ebde/src/tokens/ERC721.sol#L180) have versions of this function so that NFTs aren't lost if they're minted to contracts that cannot transfer them back out.\n\nBe careful however to respect the CEI pattern or add a re-entrancy guard as `_safeMint` adds a callback-check (`_checkOnERC721Received`) and a malicious `onERC721Received` could be exploited if not careful.\n\nReading material:\n\n- \n- \n- ", + regexPreCondition: /ERC721/gi, + regex: /(? { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const cd of findAll('ContractDefinition', file.ast)) { + if (cd.contractKind == 'interface') { + continue; + } + for (const a of findAll('FunctionDefinition', file.ast)) { + if (a.kind == 'constructor' || a.virtual) { + continue; + } + if ( + ['view', 'pure'].indexOf(a.stateMutability) == -1 && + (a.name.indexOf('Fee') > -1 || a.name.indexOf('fee') > -1) + ) { + let isProtected = false; + for (const b of findAll('IfStatement', a)) { + isProtected = true; + } + + for (const b of findAll('Identifier', a)) { + if (b.name == 'require') { + isProtected = true; + } + } + if (!isProtected) { + let endLine = '' + (a.body && a.body.statements && a.body.statements[a.body.statements.length - 1].src); + instances.push(instanceFromSRC(file, a.src, endLine)); + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/M/increaseAllowanceUSDT.ts b/src/issues/M/increaseAllowanceUSDT.ts new file mode 100644 index 0000000..f4b4eab --- /dev/null +++ b/src/issues/M/increaseAllowanceUSDT.ts @@ -0,0 +1,15 @@ +import { ContractDefinition } from 'solidity-ast'; +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC, topLevelFiles, getStorageVariable } from '../../utils'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.M, + title: "`increaseAllowance/decreaseAllowance` won't work on mainnet for USDT", + description: + "On mainnet, the mitigation to be compatible with `increaseAllowance/decreaseAllowance` isn't applied: https://etherscan.io/token/0xdac17f958d2ee523a2206206994597c13d831ec7#code, meaning it reverts on setting a non-zero & non-max allowance, unless the allowance is already zero.", + regex: /increaseAllowance|decreaseAllowance/gi, +}; + +export default issue; diff --git a/src/issues/M/keccakOnStructOrArray.ts b/src/issues/M/keccakOnStructOrArray.ts new file mode 100644 index 0000000..ad44cc8 --- /dev/null +++ b/src/issues/M/keccakOnStructOrArray.ts @@ -0,0 +1,43 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.M, + title: 'Lack of EIP-712 compliance: using `keccak256()` directly on an array or struct variable', + description: + "Directly using the actual variable instead of encoding the array values goes against the EIP-712 specification https://github.com/ethereum/EIPs/blob/master/EIPS/eip-712.md#definition-of-encodedata. \n**Note**: OpenSea's [Seaport's example with offerHashes and considerationHashes](https://github.com/ProjectOpenSea/seaport/blob/a62c2f8f484784735025d7b03ccb37865bc39e5a/reference/lib/ReferenceGettersAndDerivers.sol#L130-L131) can be used as a reference to understand how array of structs should be encoded.", + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const a of findAll('FunctionCall', file.ast)) { + if ( + a.expression && + (a.expression['name'] == 'keccak256' || a.expression['memberName'] == 'keccak256') + ) { + for (const b of a.arguments) { + if (b.typeDescriptions && b.typeDescriptions.typeIdentifier && (b.typeDescriptions.typeIdentifier.indexOf('_struct') > -1 || b.typeDescriptions.typeIdentifier.indexOf('_array') > -1 || b.typeDescriptions.typeIdentifier.indexOf('_map') > -1)) { + const inst = instanceFromSRC(file, b.src); + instances.push(inst); + } + if (b["arguments"]) { + for (const c of b["arguments"]) { + if (c.typeDescriptions && c.typeDescriptions.typeIdentifier && (c.typeDescriptions.typeIdentifier.indexOf('_struct') > -1 || c.typeDescriptions.typeIdentifier.indexOf('_array') > -1 || c.typeDescriptions.typeIdentifier.indexOf('_map') > -1)) { + const inst = instanceFromSRC(file, c.src); + instances.push(inst); + } + } + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/M/libraryPublicFunction.ts b/src/issues/M/libraryPublicFunction.ts new file mode 100644 index 0000000..f39ff52 --- /dev/null +++ b/src/issues/M/libraryPublicFunction.ts @@ -0,0 +1,36 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.M, + title: 'Library function isn\'t `internal` or `private`', + description: + 'In a library, using an external or public visibility means that we won\'t be going through the library with a DELEGATECALL but with a CALL. This changes the context and should be done carefully.', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const cd of findAll('ContractDefinition', file.ast)) { + if (cd.contractKind != 'library') { + continue; + } + for (const a of findAll('FunctionDefinition', file.ast)) { + if (a.kind == 'constructor' || a.virtual) { + continue; + } + if ( + a.visibility == "external" || a.visibility == "public") { + instances.push(instanceFromSRC(file, a.src)); + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/M/staleOracleData.ts b/src/issues/M/staleOracleData.ts new file mode 100644 index 0000000..8e9a144 --- /dev/null +++ b/src/issues/M/staleOracleData.ts @@ -0,0 +1,39 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC, lineFromIndex } from '../../utils'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.M, + title: "Chainlink's `latestRoundData` might return stale or incorrect results", + description: + "- This is a common issue: https://github.com/code-423n4/2022-12-tigris-findings/issues/655, https://code4rena.com/reports/2022-10-inverse#m-17-chainlink-oracle-data-feed-is-not-sufficiently-validated-and-can-return-stale-price, https://app.sherlock.xyz/audits/contests/41#issue-m-12-chainlinks-latestrounddata--return-stale-or-incorrect-result and many more occurrences.\n\n`latestRoundData()` is used to fetch the asset price from a Chainlink aggregator, but it's missing additional validations to ensure that the round is complete. If there is a problem with Chainlink starting a new round and finding consensus on the new value for the oracle (e.g. Chainlink nodes abandon the oracle, chain congestion, vulnerability/attacks on the Chainlink system) consumers of this contract may continue using outdated stale data / stale prices.\n\nMore bugs related to chainlink here: [Chainlink Oracle Security Considerations](https://medium.com/cyfrin/chainlink-oracle-defi-attacks-93b6cb6541bf#99af)", + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const a of findAll('VariableDeclarationStatement', file.ast)) { + let isLatestRoundData = false; + for (const b of findAll('FunctionCall', a)) { + if (b.expression && b.expression['memberName'] == 'latestRoundData') { + isLatestRoundData = true; + for (const as of a.assignments) { + if (!as) { + let inst = instanceFromSRC(file, a.src, '' + (Number(a.src.split(':')[0]) + 120)); + instances.push(inst); + break; + } + } + } + if (isLatestRoundData) { + break; + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/M/staleOracleDataL2Sequencer.ts b/src/issues/M/staleOracleDataL2Sequencer.ts new file mode 100644 index 0000000..0522ad3 --- /dev/null +++ b/src/issues/M/staleOracleDataL2Sequencer.ts @@ -0,0 +1,35 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC, lineFromIndex } from '../../utils'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.M, + title: "Missing checks for whether the L2 Sequencer is active", + description: + "Chainlink recommends that users using price oracles, check whether the Arbitrum Sequencer is [active](https://docs.chain.link/data-feeds/l2-sequencer-feeds#arbitrum). If the sequencer goes down, the Chainlink oracles will have stale prices from before the downtime, until a new L2 OCR transaction goes through. Users who submit their transactions via the [L1 Dealyed Inbox](https://developer.arbitrum.io/tx-lifecycle#1b--or-from-l1-via-the-delayed-inbox) will be able to take advantage of these stale prices. Use a [Chainlink oracle](https://blog.chain.link/how-to-use-chainlink-price-feeds-on-arbitrum/#almost_done!_meet_the_l2_sequencer_health_flag) to determine whether the sequencer is offline or not, and don't allow operations to take place while the sequencer is offline.", + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const a of findAll('VariableDeclarationStatement', file.ast)) { + let isLatestRoundData = false; + for (const b of findAll('FunctionCall', a)) { + if (b.expression && b.expression['memberName'] == 'latestRoundData') { + isLatestRoundData = true; + let inst = instanceFromSRC(file, a.src, '' + (Number(a.src.split(':')[0]) + 120)); + instances.push(inst); + break; + } + if (isLatestRoundData) { + break; + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/M/supportInterface.ts b/src/issues/M/supportInterface.ts new file mode 100644 index 0000000..8b7809c --- /dev/null +++ b/src/issues/M/supportInterface.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.M, + title: 'Direct `supportsInterface()` calls may cause caller to revert', + description: + "Calling `supportsInterface()` on a contract that doesn't implement the ERC-165 standard will result in the call reverting. Even if the caller does support the function, the contract may be malicious and consume all of the transaction's available gas. Call it via a low-level [staticcall()](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/f959d7e4e6ee0b022b41e5b644c79369869d8411/contracts/utils/introspection/ERC165Checker.sol#L119), with a fixed amount of gas, and check the return code, or use OpenZeppelin's [`ERC165Checker.supportsInterface()`](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/f959d7e4e6ee0b022b41e5b644c79369869d8411/contracts/utils/introspection/ERC165Checker.sol#L36-L39).", + regex: /\.supportsInterface\(/gi, +}; + +export default issue; diff --git a/src/issues/M/uncheckedERC20Transfer.ts b/src/issues/M/uncheckedERC20Transfer.ts new file mode 100644 index 0000000..db64a0b --- /dev/null +++ b/src/issues/M/uncheckedERC20Transfer.ts @@ -0,0 +1,56 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.M, + title: 'Return values of `transfer()`/`transferFrom()` not checked', + description: + "Not all `IERC20` implementations `revert()` when there's a failure in `transfer()`/`transferFrom()`. The function signature has a `boolean` return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually making a payment", + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const a of findAll('FunctionCall', file.ast)) { + if ( + a.expression && + (a.expression['memberName'] == 'transfer' || a.expression['memberName'] == 'transferFrom') + ) { + for (const b of findAll('Identifier', a)) { + // Case of cast + if (b.name.indexOf('ERC20') > -1) { + const inst = instanceFromSRC(file, a.src); + let str: string = inst.fileContent.split('\n')[inst.line - 1]; + const matches: any = [...str.matchAll(/bool.+=.+transfer/gi)]; + if (matches.length == 0) { + instances.push(inst); + } + } + + // Case of real type + for (const c of findAll('VariableDeclaration', file.ast)) { + if (b.name == c.name) { + for (const d of findAll('IdentifierPath', c)) { + if (d.name.indexOf('ERC20') > -1) { + const inst = instanceFromSRC(file, a.src); + let str: string = inst.fileContent.split('\n')[inst.line - 1]; + const matches: any = [...str.matchAll(/bool.+=.+transfer/gi)]; + if (matches.length == 0) { + instances.push(inst); + } + } + } + } + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/M/unsafeERC20Transfer.ts b/src/issues/M/unsafeERC20Transfer.ts new file mode 100644 index 0000000..d04bfac --- /dev/null +++ b/src/issues/M/unsafeERC20Transfer.ts @@ -0,0 +1,47 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.M, + title: 'Unsafe use of `transfer()`/`transferFrom()` with `IERC20`', + description: + "Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s `transfer()` and `transferFrom()` functions on L1 do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to `IERC20`, their [function signatures](https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca) do not match and therefore the calls made, revert (see [this](https://gist.github.com/IllIllI000/2b00a32e8f0559e8f386ea4f1800abc5) link for a test case). Use OpenZeppelin's `SafeERC20`'s `safeTransfer()`/`safeTransferFrom()` instead", + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const a of findAll('FunctionCall', file.ast)) { + if ( + a.expression && + (a.expression['memberName'] == 'transfer' || a.expression['memberName'] == 'transferFrom') + ) { + for (const b of findAll('Identifier', a)) { + // Case of cast + if (b.name.indexOf('ERC20') > -1) { + instances.push(instanceFromSRC(file, a.src)); + } + + // Case of real type + for (const c of findAll('VariableDeclaration', file.ast)) { + if (b.name == c.name) { + for (const d of findAll('IdentifierPath', c)) { + // True only on real types, not cast + if (d.name.indexOf('ERC20') > -1) { + instances.push(instanceFromSRC(file, a.src)); + } + } + } + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/abiEncodeCall.ts b/src/issues/NC/abiEncodeCall.ts new file mode 100644 index 0000000..26fea38 --- /dev/null +++ b/src/issues/NC/abiEncodeCall.ts @@ -0,0 +1,13 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: + 'Replace `abi.encodeWithSignature` and `abi.encodeWithSelector` with `abi.encodeCall` which keeps the code typo/type safe', + description: + 'When using `abi.encodeWithSignature`, it is possible to include a typo for the correct function signature.\nWhen using `abi.encodeWithSignature` or `abi.encodeWithSelector`, it is also possible to provide parameters that are not of the correct type for the function.\n\nTo avoid these pitfalls, it would be best to use [`abi.encodeCall`](https://solidity-by-example.org/abi-encode/) instead.', + regex: /(encodeWithSignature|encodeWithSelector)/gi, +}; + +export default issue; diff --git a/src/issues/NC/abicoderv2.ts b/src/issues/NC/abicoderv2.ts new file mode 100644 index 0000000..2734ef9 --- /dev/null +++ b/src/issues/NC/abicoderv2.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'abicoder v2 is enabled by default', + description: + 'abicoder v2 is considered non-experimental as of Solidity 0.6.0 and it is enabled by default starting with Solidity 0.8.0. Therefore, there is no need to write.', + regex: /pragma.*abicoder.*v2/gi, +}; + +export default issue; diff --git a/src/issues/NC/assertRequire.ts b/src/issues/NC/assertRequire.ts new file mode 100644 index 0000000..4d33b96 --- /dev/null +++ b/src/issues/NC/assertRequire.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: '`require()` should be used instead of `assert()`', + description: + 'Prior to solidity version 0.8.0, hitting an assert consumes the **remainder of the transaction\'s available gas** rather than returning it, as `require()`/`revert()` do. `assert()` should be avoided even past solidity version 0.8.0 as its [documentation](https://docs.soliditylang.org/en/v0.8.14/control-structures.html#panic-via-assert-and-error-via-require) states that "The assert function creates an error of type Panic(uint256). ... Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix. Additionally, a require statement (or a custom error) are more friendly in terms of understanding what happened."', + regex: /assert\(/gi, +}; + +export default issue; diff --git a/src/issues/NC/concat.ts b/src/issues/NC/concat.ts new file mode 100644 index 0000000..f0d0735 --- /dev/null +++ b/src/issues/NC/concat.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Use `string.concat()` or `bytes.concat()` instead of `abi.encodePacked`', + description: + 'Solidity version 0.8.4 introduces `bytes.concat()` (vs `abi.encodePacked(,)`)\n\nSolidity version 0.8.12 introduces `string.concat()` (vs `abi.encodePacked(,), which catches concatenation errors (in the event of a `bytes` data mixed in the concatenation)`)', + regex: /abi\.encodePacked\(/gi, +}; + +export default issue; diff --git a/src/issues/NC/constantStyle.ts b/src/issues/NC/constantStyle.ts new file mode 100644 index 0000000..0a3bc51 --- /dev/null +++ b/src/issues/NC/constantStyle.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Constants should be in CONSTANT_CASE', + description: + 'For `constant` variable names, each word should use all capital letters, with underscores separating each word (CONSTANT_CASE)', + regex: / (constant|immutable) (?!override).*[a-z]+.* =/g, +}; + +export default issue; diff --git a/src/issues/NC/constantsInsteadMagic.ts b/src/issues/NC/constantsInsteadMagic.ts new file mode 100644 index 0000000..8eea778 --- /dev/null +++ b/src/issues/NC/constantsInsteadMagic.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: '`constant`s should be defined rather than using magic numbers', + description: + 'Even [assembly](https://github.com/code-423n4/2022-05-opensea-seaport/blob/9d7ce4d08bf3c3010304a0476a785c70c0e90ae7/contracts/lib/TokenTransferrer.sol#L35-L39) can benefit from using readable constants instead of hex/numeric literals', + regex: /(?\n\n**Recommended Mitigation Steps**\n\nLack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.', + regex: /function set.*address.*(owner|admin).*\)/gi, +}; + +export default issue; diff --git a/src/issues/NC/dangerWhileTrue.ts b/src/issues/NC/dangerWhileTrue.ts new file mode 100644 index 0000000..511a91a --- /dev/null +++ b/src/issues/NC/dangerWhileTrue.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Dangerous `while(true)` loop', + description: 'Consider using for-loops to avoid all risks of an infinite-loop situation', + regex: /while ?\( ?true ?\)/gi, +}; + +export default issue; diff --git a/src/issues/NC/defaultVisibility.ts b/src/issues/NC/defaultVisibility.ts new file mode 100644 index 0000000..481d4dc --- /dev/null +++ b/src/issues/NC/defaultVisibility.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Default Visibility for constants', + description: + 'Some constants are using the default visibility. For readability, consider explicitly declaring them as `internal`.', + regex: /(? { + let instances: Instance[] = []; + let affectedLines: any = {}; + for (const file of files) { + if (!!file.ast) { + for (const functionCall of findAll('FunctionCall', file.ast)) { + if (functionCall.expression.nodeType === 'Identifier' && functionCall.expression.name === 'require') { + let counter = 0; + let typeString = + functionCall.expression.argumentTypes && + functionCall.expression.argumentTypes[1] && + functionCall.expression.argumentTypes[1].typeString; + if (!typeString) { + continue; + } + let requires = new Set([instanceFromSRC(file, functionCall.src)]); + let lines = new Set(); + // instances.push(instanceFromSRC(file, functionCall.src)); + for (const node of findAll('FunctionCall', file.ast)) { + if (node.expression.nodeType === 'Identifier' && node.expression.name === 'require') { + let typeString2 = + node.expression.argumentTypes && + node.expression.argumentTypes[1] && + node.expression.argumentTypes[1].typeString; + if (typeString == typeString2) { + counter++; + if (counter > 1) { + requires.add(instanceFromSRC(file, node.src)); + } + } + } + } + if (counter > 1) { + for (let r of requires) { + if (!affectedLines[r.fileName] || affectedLines[r.fileName].indexOf(r.line) == -1) { + affectedLines[r.fileName] + ? affectedLines[r.fileName].push(r.line) + : (affectedLines[r.fileName] = [r.line]); + instances.push(r); + } + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/elseBlockOnReturn.ts b/src/issues/NC/elseBlockOnReturn.ts new file mode 100644 index 0000000..d3de8dd --- /dev/null +++ b/src/issues/NC/elseBlockOnReturn.ts @@ -0,0 +1,27 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.NC, + title: '`else`-block not required', + description: 'One level of nesting can be removed by not having an else block when the if-block returns', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const a of findAll('IfStatement', file.ast)) { + if (!a.condition) { + // if (a.trueBody.) + instances.push(instanceFromSRC(file, a.src)); + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/errorneverUsed.ts b/src/issues/NC/errorneverUsed.ts new file mode 100644 index 0000000..aac7428 --- /dev/null +++ b/src/issues/NC/errorneverUsed.ts @@ -0,0 +1,43 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC, lineFromIndex } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.NC, + title: 'Unused `error` definition', + description: + "Note that there may be cases where an error superficially appears to be used, but this is only because there are multiple definitions of the error in different files. In such cases, the error definition should be moved into a separate file. The instances below are the unused definitions.", + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const cd of findAll('ContractDefinition', file.ast)) { + if (cd.contractKind == 'interface' || cd.abstract) { + continue; + } + + for (const a of findAll('ErrorDefinition', cd)) { + let isUsed = false; + for (const file2 of files) { + if (!!file2.ast) { + for (const b of findAll('Identifier', file2.ast)) { + if (a.name == b.name) { + isUsed = true; + } + } + } + } + if (!isUsed) { + instances.push(instanceFromSRC(file, a.src)); + } + } + } + + } + } + return instances; + }, +}; +export default issue; diff --git a/src/issues/NC/eventNeverEmitted.ts b/src/issues/NC/eventNeverEmitted.ts new file mode 100644 index 0000000..289c131 --- /dev/null +++ b/src/issues/NC/eventNeverEmitted.ts @@ -0,0 +1,44 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.NC, + title: 'Event is never emitted', + description: 'The following are defined but never emitted. They can be removed to make the code cleaner.', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + + for (const file of files) { + if (!!file.ast) { + for (const cd of findAll('ContractDefinition', file.ast)) { + if (cd.contractKind == 'interface' || cd.abstract) { + continue; + } + for (const a of findAll('EventDefinition', cd)) { + let isCalled = false; + for (const b of findAll('EmitStatement', cd)) { + for (const b1 of findAll('Identifier', b)) { + if (a.name == b1.name) { + isCalled = true; + break; + } + } + if (isCalled) { + break; + } + } + if (!isCalled) { + instances.push(instanceFromSRC(file, a.src)); + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/eventNoArgs.ts b/src/issues/NC/eventNoArgs.ts new file mode 100644 index 0000000..fa41381 --- /dev/null +++ b/src/issues/NC/eventNoArgs.ts @@ -0,0 +1,26 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.NC, + title: 'Events should use parameters to convey information', + description: 'For example, rather than using `event Paused()` and `event Unpaused()`, use `event PauseState(address indexed whoChangedIt, bool wasPaused, bool isNowPaused)`', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + + for (const file of files) { + if (!!file.ast) { + for (const a of findAll('EventDefinition', file.ast)) { + if (!a.parameters || !a.parameters.parameters || a.parameters.parameters.length == 0) + instances.push(instanceFromSRC(file, a.src)); + } + } + } + return instances; + }, +}; + +export default issue; \ No newline at end of file diff --git a/src/issues/NC/eventNotProperlyIndexed.ts b/src/issues/NC/eventNotProperlyIndexed.ts new file mode 100644 index 0000000..8b10cd4 --- /dev/null +++ b/src/issues/NC/eventNotProperlyIndexed.ts @@ -0,0 +1,27 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.NC, + title: 'Event missing indexed field', + description: + "Index event fields make the field more quickly accessible [to off-chain tools](https://ethereum.stackexchange.com/questions/40396/can-somebody-please-explain-the-concept-of-event-indexing) that parse events. This is especially useful when it comes to filtering based on an address. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Where applicable, each `event` should use three `indexed` fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three applicable fields, all of the applicable fields should be indexed.", + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const a of findAll('EventDefinition', file.ast)) { + if (a.parameters.parameters.length > 0 && a.parameters.parameters.filter(e => e.indexed).length == 0) { + instances.push(instanceFromSRC(file, a.src)); + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/eventOldNew.ts b/src/issues/NC/eventOldNew.ts new file mode 100644 index 0000000..40df323 --- /dev/null +++ b/src/issues/NC/eventOldNew.ts @@ -0,0 +1,29 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.NC, + title: 'Events that mark critical parameter changes should contain both the old and the new value', + description: 'This should especially be done if the new value is not required to be different from the old value', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + + for (const file of files) { + if (!!file.ast) { + for (const a of findAll('FunctionDefinition', file.ast)) { + if (a.name.indexOf("set") == 0 || a.name.indexOf("update") == 0) { + for (const b of findAll('EmitStatement', a)) { + instances.push(instanceFromSRC(file, a.src, b.src)); + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/functionOrdering.ts b/src/issues/NC/functionOrdering.ts new file mode 100644 index 0000000..81a46e7 --- /dev/null +++ b/src/issues/NC/functionOrdering.ts @@ -0,0 +1,66 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.NC, + title: 'Function ordering does not follow the Solidity style guide', + description: + 'According to the [Solidity style guide](https://docs.soliditylang.org/en/v0.8.17/style-guide.html#order-of-functions), functions should be laid out in the following order :`constructor()`, `receive()`, `fallback()`, `external`, `public`, `internal`, `private`, but the cases below do not follow this pattern', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + let currentOrder: string[] = []; + let proposedOrder: string[] = []; + for (const a of findAll('FunctionDefinition', file.ast)) { + if (a.name) { + currentOrder.push(a.visibility + ' ' + a.name); + } + } + + currentOrder + .filter(e => e.indexOf('external') > -1) + .forEach(e => { + proposedOrder.push(e); + }); + currentOrder + .filter(e => e.indexOf('public') > -1) + .forEach(e => { + proposedOrder.push(e); + }); + currentOrder + .filter(e => e.indexOf('internal') > -1) + .forEach(e => { + proposedOrder.push(e); + }); + currentOrder + .filter(e => e.indexOf('private') > -1) + .forEach(e => { + proposedOrder.push(e); + }); + + let hasDifference = 0; + for (let i = 0; i < currentOrder.length; i++) { + if (currentOrder[i] != proposedOrder[i]) { + hasDifference = 1; + break; + } + } + if (hasDifference > 0) { + let concat = ['\nCurrent order:']; + concat = concat.concat(currentOrder); + concat.push(''); + concat.push('Suggested order:'); + concat = concat.concat(proposedOrder); + instances.push({ fileName: file.name, line: 1, endLine: concat.length + 1, fileContent: concat.join('\n') }); + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/functionsTooLong.ts b/src/issues/NC/functionsTooLong.ts new file mode 100644 index 0000000..d64080b --- /dev/null +++ b/src/issues/NC/functionsTooLong.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Functions should not be longer than 50 lines', + description: + 'Overly complex code can make understanding functionality more difficult, try to further modularize your code to ensure readability ', + regex: /function.{42,}/gi, +}; + +export default issue; diff --git a/src/issues/NC/implicitInt.ts b/src/issues/NC/implicitInt.ts new file mode 100644 index 0000000..1a8cc33 --- /dev/null +++ b/src/issues/NC/implicitInt.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Change int to int256', + description: + 'Throughout the code base, some variables are declared as `int`. To favor explicitness, consider changing all instances of `int` to `int256`', + regex: /(? { + let instances: Instance[] = []; + + for (const file of files) { + if (!!file.ast) { + for (const a of findAll('ContractDefinition', file.ast)) { + if (a.contractKind == 'interface' && file.name.indexOf(a.name) == -1) { + instances.push(instanceFromSRC(file, a.src)); + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/lackReasonableBounds.ts b/src/issues/NC/lackReasonableBounds.ts new file mode 100644 index 0000000..da1b344 --- /dev/null +++ b/src/issues/NC/lackReasonableBounds.ts @@ -0,0 +1,51 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.NC, + title: 'Lack of checks in setters', + description: + "Be it sanity checks (like checks against `0`-values) or initial setting checks: it's best for Setter functions to have them", + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const cd of findAll('ContractDefinition', file.ast)) { + if (cd.contractKind == 'interface') { + continue; + } + for (const a of findAll('FunctionDefinition', file.ast)) { + if (a.kind == 'constructor' || a.virtual) { + continue; + } + if ( + ['view', 'pure'].indexOf(a.stateMutability) == -1 && + (a.name.indexOf('set') > -1 || a.name.indexOf('update') > -1) + ) { + let isProtected = false; + for (const b of findAll('IfStatement', a)) { + isProtected = true; + } + + for (const b of findAll('Identifier', a)) { + if (b.name == 'require') { + isProtected = true; + } + } + if (!isProtected) { + let endLine = '' + (a.body && a.body.statements && a.body.statements.length > 0 && a.body.statements[a.body.statements.length - 1].src); + instances.push(instanceFromSRC(file, a.src, endLine)); + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/linesTooLong.ts b/src/issues/NC/linesTooLong.ts new file mode 100644 index 0000000..6e87248 --- /dev/null +++ b/src/issues/NC/linesTooLong.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Lines are too long', + description: + "Usually lines in source code are limited to [80](https://softwareengineering.stackexchange.com/questions/148677/why-is-80-characters-the-standard-limit-for-code-width) characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over [164](https://github.com/aizatto/character-length) characters, the lines below should be split when they reach that length", + regex: /.{164,}/gi, +}; + +export default issue; diff --git a/src/issues/NC/mappingStyle.ts b/src/issues/NC/mappingStyle.ts new file mode 100644 index 0000000..502f500 --- /dev/null +++ b/src/issues/NC/mappingStyle.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: '`mapping` definitions do not follow the Solidity Style Guide', + description: + "See the [mappings](https://docs.soliditylang.org/en/latest/style-guide.html#mappings) section of the Solidity Style Guide", + regex: /mapping \(|mapping\( /gi, +}; + +export default issue; diff --git a/src/issues/NC/maxUint.ts b/src/issues/NC/maxUint.ts new file mode 100644 index 0000000..0d2d4a1 --- /dev/null +++ b/src/issues/NC/maxUint.ts @@ -0,0 +1,10 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: '`type(uint).max` should be used instead of `uint(-1)`', + regex: /uint\d?\d?\d?\(-1\)/gi, +}; + +export default issue; diff --git a/src/issues/NC/maxUint256.ts b/src/issues/NC/maxUint256.ts new file mode 100644 index 0000000..dbbbc78 --- /dev/null +++ b/src/issues/NC/maxUint256.ts @@ -0,0 +1,10 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: '`type(uint256).max` should be used instead of `2 ** 256 - 1`', + regex: /2 ?\*\* ?256 ?- ?1/gi, +}; + +export default issue; diff --git a/src/issues/NC/missingEventSetters.ts b/src/issues/NC/missingEventSetters.ts new file mode 100644 index 0000000..a3fed1d --- /dev/null +++ b/src/issues/NC/missingEventSetters.ts @@ -0,0 +1,46 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.NC, + title: 'Missing Event for critical parameters change', + description: + 'Events help non-contract tools to track changes, and events prevent users from being surprised by changes.', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const cd of findAll('ContractDefinition', file.ast)) { + if (cd.contractKind == 'interface') { + continue; + } + for (const a of findAll('FunctionDefinition', file.ast)) { + if (a.kind == 'constructor' || a.virtual || a.visibility == 'internal' || a.visibility == 'private') { + continue; + } + if ( + ['view', 'pure'].indexOf(a.stateMutability) == -1 && + (a.name.indexOf('set') > -1 || a.name.indexOf('update') > -1) + ) { + let isEvented = false; + for (const b of findAll('EmitStatement', a)) { + isEvented = true; + } + + if (!isEvented) { + let endLine = '' + (a.body && a.body.statements && a.body.statements.length > 0 && a.body.statements[a.body.statements.length - 1].src); + instances.push(instanceFromSRC(file, a.src, endLine)); + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/missingNatspec.ts b/src/issues/NC/missingNatspec.ts new file mode 100644 index 0000000..7287db6 --- /dev/null +++ b/src/issues/NC/missingNatspec.ts @@ -0,0 +1,39 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.NC, + title: 'NatSpec is completely non-existent on functions that should have them', + description: "Public and external functions that aren't view or pure should have NatSpec comments", + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const cd of findAll('ContractDefinition', file.ast)) { + if (cd.contractKind == 'interface') { + continue; + } + for (const a of findAll('FunctionDefinition', file.ast)) { + if (a.kind == 'constructor' || a.virtual || a.kind == "fallback" || a.kind == "receive") { + continue; + } + if ( + ['public', 'external'].indexOf(a.visibility) > -1 && + ['view', 'pure'].indexOf(a.stateMutability) == -1 + ) { + if (!a.documentation || !a.documentation.text) { + instances.push(instanceFromSRC(file, a.src)); + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/missingNatspecParam.ts b/src/issues/NC/missingNatspecParam.ts new file mode 100644 index 0000000..8fa4fcc --- /dev/null +++ b/src/issues/NC/missingNatspecParam.ts @@ -0,0 +1,44 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.NC, + title: 'Incomplete NatSpec: `@param` is missing on actually documented functions', + description: 'The following functions are missing `@param` NatSpec comments.', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const cd of findAll('ContractDefinition', file.ast)) { + if (cd.contractKind == 'interface') { + continue; + } + for (const a of findAll('FunctionDefinition', file.ast)) { + if (a.kind == 'constructor' || a.virtual) { + continue; + } + if ( + ['public', 'external'].indexOf(a.visibility) > -1 && + ['view', 'pure'].indexOf(a.stateMutability) == -1 + ) { + if (!!a.documentation && !!a.documentation.text && a.documentation.text.indexOf('@inheritdoc') == -1) { + let params = a.documentation?.text.split('\n').filter(e => e.indexOf('@param') > -1) || []; + let parameterss = a.parameters.parameters; + if (params.length < parameterss.length) { + let srcStart = (a.documentation && a.documentation.src) || a.src; + instances.push(instanceFromSRC(file, srcStart, parameterss[parameterss.length - 1].src)); + } + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/missingNatspecReturn.ts b/src/issues/NC/missingNatspecReturn.ts new file mode 100644 index 0000000..91d0152 --- /dev/null +++ b/src/issues/NC/missingNatspecReturn.ts @@ -0,0 +1,45 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.NC, + title: 'Incomplete NatSpec: `@return` is missing on actually documented functions', + description: 'The following functions are missing `@return` NatSpec comments.', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const cd of findAll('ContractDefinition', file.ast)) { + if (cd.contractKind == 'interface') { + continue; + } + for (const a of findAll('FunctionDefinition', file.ast)) { + if (a.kind == 'constructor' || a.virtual) { + continue; + } + + if ( + ['public', 'external'].indexOf(a.visibility) > -1 && + ['view', 'pure'].indexOf(a.stateMutability) == -1 + ) { + if (!!a.documentation && !!a.documentation.text && a.documentation.text.indexOf('@inheritdoc') == -1) { + let returns = a.documentation?.text.split('\n').filter(e => e.indexOf('@return') > -1) || []; + let returnss = a.returnParameters.parameters; + if (returns.length < returnss.length) { + let srcStart = (a.documentation && a.documentation.src) || a.src; + instances.push(instanceFromSRC(file, srcStart, returnss[returnss.length - 1].src)); + } + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/missingSPDX.ts b/src/issues/NC/missingSPDX.ts new file mode 100644 index 0000000..8bc91e6 --- /dev/null +++ b/src/issues/NC/missingSPDX.ts @@ -0,0 +1,25 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.NC, + title: 'File\'s first line is not an SPDX Identifier', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + const content = file.content; + const split_lines = content.split('\n'); + if (split_lines[0].indexOf('SPDX') == -1) { + instances.push({ fileName: file.name, line: 1, fileContent: content }); + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/modifierForMsgSender.ts b/src/issues/NC/modifierForMsgSender.ts new file mode 100644 index 0000000..d30e471 --- /dev/null +++ b/src/issues/NC/modifierForMsgSender.ts @@ -0,0 +1,13 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +// Also catches the checks in modifiers and should be an AST detector +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Use a `modifier` instead of a `require/if` statement for a special `msg.sender` actor', + description: + 'If a function is supposed to be access-controlled, a `modifier` should be used instead of a `require/if` statement for more readability.', + regex: /(require|if|assert).*(msg\.sender)/gi, +}; + +export default issue; diff --git a/src/issues/NC/multipleConstantDeclaration.ts b/src/issues/NC/multipleConstantDeclaration.ts new file mode 100644 index 0000000..cf4aff3 --- /dev/null +++ b/src/issues/NC/multipleConstantDeclaration.ts @@ -0,0 +1,38 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.NC, + title: 'Constant state variables defined more than once', + description: + 'Rather than redefining state variable constant, consider using a library to store all constants as this will prevent data redundancy', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const a of findAll('VariableDeclaration', file.ast)) { + if (a.mutability != 'constant') { + continue; + } + for (const file2 of files) { + if (!!file2.ast && file.ast.absolutePath != file2.ast.absolutePath) { + for (const b of findAll('VariableDeclaration', file2.ast)) { + if (b.mutability != 'constant') { + continue; + } else if (b.name == a.name) { + instances.push(instanceFromSRC(file, a.src)); + } + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/namedMappings.ts b/src/issues/NC/namedMappings.ts new file mode 100644 index 0000000..db3ea3b --- /dev/null +++ b/src/issues/NC/namedMappings.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Consider using named mappings', + description: + 'Consider moving to solidity version 0.8.18 or later, and using [named mappings](https://ethereum.stackexchange.com/questions/51629/how-to-name-the-arguments-in-mapping/145555#145555) to make it easier to understand the purpose of each mapping', + regex: /mapping\(\S+ => /g, +}; + +export default issue; diff --git a/src/issues/NC/noAddressHardcode.ts b/src/issues/NC/noAddressHardcode.ts new file mode 100644 index 0000000..07e165c --- /dev/null +++ b/src/issues/NC/noAddressHardcode.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: '`address`s shouldn\'t be hard-coded', + description: 'It is often better to declare `address`es as `immutable`, and assign them via constructor arguments. This allows the code to remain the same across deployments on different networks, and avoids recompilation when addresses need to change.', + regex: /0x\S{40}(\)|;)/gi, +}; + +export default issue; diff --git a/src/issues/NC/nonReentrantBeforeModifiers.ts b/src/issues/NC/nonReentrantBeforeModifiers.ts index 0f2c45b..0a47d49 100644 --- a/src/issues/NC/nonReentrantBeforeModifiers.ts +++ b/src/issues/NC/nonReentrantBeforeModifiers.ts @@ -6,7 +6,7 @@ const issue: RegexIssue = { title: 'The `nonReentrant` `modifier` should occur before all other modifiers', description: 'This is a best-practice to protect against reentrancy in other modifiers', regex: - /function.?\([a-zA-Z]*\)[^\}]*[[:space:]]((?!(external[[:space:]]|override[[:space:]]|view[[:space:]]|pure[[:space:]]|internal[[:space:]]|private[[:space:]]))[a-zA-Z]+[[:space:]])+[^\}]*nonReentrant/g, + /function.?\([a-zA-Z]*\)[^\}]*[[:space:]]((?!(external[[:space:]]|override[[:space:]]|view[[:space:]]|pure[[:space:]]|internal[[:space:]]|private[[:space:]]))[a-zA-Z]+[[:space:]])+[^\}]*nonReentrant/gi, }; export default issue; diff --git a/src/issues/NC/numericTimeDays.ts b/src/issues/NC/numericTimeDays.ts new file mode 100644 index 0000000..a6d30ca --- /dev/null +++ b/src/issues/NC/numericTimeDays.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Numeric values having to do with time should use time units for readability', + description: + "There are [units](https://docs.soliditylang.org/en/latest/units-and-global-variables.html#time-units) for seconds, minutes, hours, days, and weeks, and since they're defined, they should be used", + regex: /uint.+(epoch|expiry|period|warmup|time|delay).+ = \d*;/gi, +}; + +export default issue; diff --git a/src/issues/NC/onlyConstantsInUppercase.ts b/src/issues/NC/onlyConstantsInUppercase.ts new file mode 100644 index 0000000..d66b92b --- /dev/null +++ b/src/issues/NC/onlyConstantsInUppercase.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Variable names that consist of all capital letters should be reserved for `constant`/`immutable` variables', + description: + 'If the variable needs to be different based on which class it comes from, a `view`/`pure` *function* should be used instead (e.g. like [this](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/76eee35971c2541585e05cbf258510dda7b2fbc6/contracts/token/ERC20/extensions/draft-IERC20Permit.sol#L59)).', + regex: /^(?!.*constant).*[A-Z]{2,}/gi, +}; + +export default issue; diff --git a/src/issues/NC/ownerCanRenounceWhilePaused.ts b/src/issues/NC/ownerCanRenounceWhilePaused.ts new file mode 100644 index 0000000..d1a5aa6 --- /dev/null +++ b/src/issues/NC/ownerCanRenounceWhilePaused.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Owner can renounce while system is paused', + description: + 'The contract owner or single user with a role is not prevented from renouncing the role/ownership while the contract is paused, which would cause any user assets stored in the protocol, to be locked indefinitely.', + regex: /function.*pause.*onlyOwner/gi, +}; + +export default issue; diff --git a/src/issues/NC/redundantNamedReturn.ts b/src/issues/NC/redundantNamedReturn.ts new file mode 100644 index 0000000..fd7534c --- /dev/null +++ b/src/issues/NC/redundantNamedReturn.ts @@ -0,0 +1,38 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import util from 'util'; +import { instanceFromSRC } from '../../utils'; + +//@note trop fier de moi +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.NC, + title: 'Adding a `return` statement when the function defines a named return variable, is redundant', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + + for (const file of files) { + if (!!file.ast) { + for (const a of findAll('FunctionDefinition', file.ast)) { + if ( + a.returnParameters && + a.returnParameters.parameters && + a.returnParameters.parameters[0] && + a.returnParameters.parameters[0].name + ) { + for (const b of findAll('Return', a)) { + if (a.documentation && a.documentation.src) { + instances.push(instanceFromSRC(file, a.documentation && a.documentation.src, b.src)); + } else { + instances.push(instanceFromSRC(file, a.src, b.src)); + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/requireWithString.ts b/src/issues/NC/requireWithString.ts index 3d89372..5a5c795 100644 --- a/src/issues/NC/requireWithString.ts +++ b/src/issues/NC/requireWithString.ts @@ -5,7 +5,7 @@ import { instanceFromSRC } from '../../utils'; const issue: ASTIssue = { regexOrAST: 'AST', type: IssueTypes.NC, - title: ' `require()` / `revert()` statements should have descriptive reason strings', + title: '`require()` / `revert()` statements should have descriptive reason strings', detector: (files: InputType): Instance[] => { let instances: Instance[] = []; diff --git a/src/issues/NC/returnValueOfApprove.ts b/src/issues/NC/returnValueOfApprove.ts deleted file mode 100644 index aaf680e..0000000 --- a/src/issues/NC/returnValueOfApprove.ts +++ /dev/null @@ -1,13 +0,0 @@ -import { IssueTypes, RegexIssue } from '../../types'; - -const issue: RegexIssue = { - regexOrAST: 'Regex', - type: IssueTypes.NC, - title: 'Return values of `approve()` not checked', - description: - "Not all IERC20 implementations `revert()` when there's a failure in `approve()`. The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually approving anything", - regex: /\n((?![^=\n]*function)[^=\n]*)approve.?\(/g, - startLineModifier: 1, -}; - -export default issue; diff --git a/src/issues/NC/revertWithoutArgument.ts b/src/issues/NC/revertWithoutArgument.ts new file mode 100644 index 0000000..c86738c --- /dev/null +++ b/src/issues/NC/revertWithoutArgument.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: "Take advantage of Custom Error's return value property", + description: + 'An important feature of Custom Error is that values such as address, tokenID, msg.value can be written inside the () sign, this kind of approach provides a serious advantage in debugging and examining the revert details of dapps such as tenderly.', + regex: /revert.*\(\)/gi, +}; + +export default issue; diff --git a/src/issues/NC/safeMath08.ts b/src/issues/NC/safeMath08.ts new file mode 100644 index 0000000..7eb1228 --- /dev/null +++ b/src/issues/NC/safeMath08.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Deprecated library used for Solidity `>= 0.8` : SafeMath', + regexPreCondition: /pragma.+0\.8/gi, + regex: /SafeMath/gi, +}; + +export default issue; diff --git a/src/issues/NC/scientificNotationExponent.ts b/src/issues/NC/scientificNotationExponent.ts new file mode 100644 index 0000000..7d2bfdf --- /dev/null +++ b/src/issues/NC/scientificNotationExponent.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Use scientific notation (e.g. `1e18`) rather than exponentiation (e.g. `10**18`)', + description: + "While this won't save gas in the recent solidity versions, this is shorter and more readable (this is especially true in calculations).", + regex: /10 ?\*\* ?\d/gi, +}; + +export default issue; diff --git a/src/issues/NC/scientificNotationZeros.ts b/src/issues/NC/scientificNotationZeros.ts new file mode 100644 index 0000000..9b3e97c --- /dev/null +++ b/src/issues/NC/scientificNotationZeros.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Use scientific notation for readability reasons for large multiples of ten', + description: + "The more a number has zeros, the harder it becomes to see with the eyes if it's the intended value. To ease auditing and bug bounty hunting, consider using the scientific notation", + regex: /( |\()\d00000/gi, +}; + +export default issue; diff --git a/src/issues/NC/sensitiveTerms.ts b/src/issues/NC/sensitiveTerms.ts new file mode 100644 index 0000000..75495b5 --- /dev/null +++ b/src/issues/NC/sensitiveTerms.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Avoid the use of sensitive terms', + description: + 'Use [alternative variants](https://www.zdnet.com/article/mysql-drops-master-slave-and-blacklist-whitelist-terminology/), e.g. allowlist/denylist instead of whitelist/blacklist', + regex: /(whitelist|blacklist)/gi, +}; + +export default issue; diff --git a/src/issues/NC/stringQuote.ts b/src/issues/NC/stringQuote.ts new file mode 100644 index 0000000..6b233d5 --- /dev/null +++ b/src/issues/NC/stringQuote.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Strings should use double quotes rather than single quotes', + description: + 'See the Solidity Style Guide: https://docs.soliditylang.org/en/v0.8.20/style-guide.html#other-recommendations', + regex: /(?\n\nFunctions should be grouped according to their visibility and ordered:\n\n- constructor\n- receive function (if exists)\n- fallback function (if exists)\n- external\n- public\n- internal\n- private\n- within a grouping, place the view and pure functions last', + regex: /^contract .+ \{/gi, +}; + +export default issue; diff --git a/src/issues/NC/styleGuide.ts b/src/issues/NC/styleGuide.ts new file mode 100644 index 0000000..7d060b6 --- /dev/null +++ b/src/issues/NC/styleGuide.ts @@ -0,0 +1,96 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.NC, + title: "Contract does not follow the Solidity style guide's suggested layout ordering", + description: + 'The [style guide](https://docs.soliditylang.org/en/v0.8.16/style-guide.html#order-of-layout) says that, within a contract, the ordering should be:\n\n1) Type declarations\n2) State variables\n3) Events\n4) Modifiers\n5) Functions\n\nHowever, the contract(s) below do not follow this ordering', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + let currentOrder: string[] = []; + let proposedOrder: string[] = []; + for (const a of findAll('ContractDefinition', file.ast)) { + for (const b of a.nodes) { + let currentString = b.nodeType + '.'; + if (!!b['name']) { + currentString += b['name']; + } else if (!!b['typeName'] && !!b['typeName']['pathNode'] && !!b['typeName']['pathNode']['name']) { + currentString += b['name'] || b['typeName']['pathNode']['name']; + } else { + currentString += b['kind'] || (b['libraryName'] && b['libraryName'].name); + } + currentOrder.push(currentString); + } + } + + currentOrder + .filter(e => e.indexOf('UsingForDirective') > -1) + .forEach(e => { + proposedOrder.push(e); + }); + currentOrder + .filter(e => e.indexOf('VariableDeclaration') > -1) + .forEach(e => { + proposedOrder.push(e); + }); + currentOrder + .filter(e => e.indexOf('EnumDefinition') > -1) + .forEach(e => { + proposedOrder.push(e); + }); + currentOrder + .filter(e => e.indexOf('StructDefinition') > -1) + .forEach(e => { + proposedOrder.push(e); + }); + + currentOrder + .filter(e => e.indexOf('ErrorDefinition') > -1) + .forEach(e => { + proposedOrder.push(e); + }); + + currentOrder + .filter(e => e.indexOf('EventDefinition') > -1) + .forEach(e => { + proposedOrder.push(e); + }); + currentOrder + .filter(e => e.indexOf('ModifierDefinition') > -1) + .forEach(e => { + proposedOrder.push(e); + }); + currentOrder + .filter(e => e.indexOf('FunctionDefinition') > -1) + .forEach(e => { + proposedOrder.push(e); + }); + + let hasDifference = 0; + for (let i = 0; i < currentOrder.length; i++) { + if (currentOrder[i] != proposedOrder[i]) { + hasDifference = 1; + break; + } + } + if (hasDifference > 0) { + let concat = ['\nCurrent order:']; + concat = concat.concat(currentOrder); + concat.push(''); + concat.push('Suggested order:'); + concat = concat.concat(proposedOrder); + instances.push({ fileName: file.name, line: 1, endLine: concat.length + 1, fileContent: concat.join('\n') }); + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/unclearRequire.ts b/src/issues/NC/unclearRequire.ts new file mode 100644 index 0000000..cebb67b --- /dev/null +++ b/src/issues/NC/unclearRequire.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Some require descriptions are not clear', + description: + '1. It does not comply with the general require error description model of the project (Either all of them should be debugged in this way, or all of them should be explained with a string not exceeding 32 bytes.)\n2. For debug dapps like Tenderly, these debug messages are important, this allows the user to see the reasons for revert practically.', + regex: /require.*".{0,3}"/gi, +}; + +export default issue; diff --git a/src/issues/NC/underscoreDecimals.ts b/src/issues/NC/underscoreDecimals.ts new file mode 100644 index 0000000..e1e01ab --- /dev/null +++ b/src/issues/NC/underscoreDecimals.ts @@ -0,0 +1,10 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Use Underscores for Number Literals (add an underscore every 3 digits)', + regex: / \d+\d\d\d+/gi, +}; + +export default issue; diff --git a/src/issues/NC/underscoreInternal.ts b/src/issues/NC/underscoreInternal.ts new file mode 100644 index 0000000..a2a4243 --- /dev/null +++ b/src/issues/NC/underscoreInternal.ts @@ -0,0 +1,44 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.NC, + title: 'Internal and private variables and functions names should begin with an underscore', + description: + 'According to the Solidity Style Guide, Non-`external` variable and function names should begin with an [underscore](https://docs.soliditylang.org/en/latest/style-guide.html#underscore-prefix-for-non-external-functions-and-variables)', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + for (const file of files) { + if (!!file.ast) { + for (const a of findAll('VariableDeclaration', file.ast)) { + if ( + !a.constant && + a.stateVariable && + (a.visibility == 'internal' || a.visibility == 'private') && + !a.constant && + a.mutability != 'immutable' && + a.name.charAt(0) != '_' + ) { + instances.push(instanceFromSRC(file, a.src)); + } + } + + for (const a of findAll('FunctionDefinition', file.ast)) { + if (a.kind === 'constructor') { + continue; + } + + if ((a.visibility == 'internal' || a.visibility == 'private') && a.name.charAt(0) != '_') { + instances.push(instanceFromSRC(file, a.src)); + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/uselessOverride.ts b/src/issues/NC/uselessOverride.ts new file mode 100644 index 0000000..aea1929 --- /dev/null +++ b/src/issues/NC/uselessOverride.ts @@ -0,0 +1,61 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +import util from 'util'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.NC, + title: + '`override` function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + + for (const file of files) { + for (const cd of findAll('ContractDefinition', file.ast)) { + if (cd.contractKind == 'interface' || cd.abstract) { + continue; + } + for (const a of findAll('FunctionDefinition', file.ast)) { + if (a.kind == 'constructor' || a.virtual || a.kind == 'fallback' || a.kind == 'receive') { + continue; + } + if (!a.body) { + continue; + } + if (!!a.overrides) { + for (const b of findAll('ParameterList', a)) { + if (a.returnParameters && a.returnParameters.id == b.id) { + continue; + } + for (const b1 of findAll('VariableDeclaration', b)) { + let usesParam = false; + if (!b1.name) { + continue; + } + for (const c of findAll('Block', a)) { + for (const d of findAll('Identifier', c)) { + if (b1.name == d.name) { + usesParam = true; + break; + } + } + if (usesParam) { + break; + } + } + + if (!usesParam) { + instances.push(instanceFromSRC(file, b1.src)); + } + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/uselessPublic.ts b/src/issues/NC/uselessPublic.ts index 73b9165..7156da3 100644 --- a/src/issues/NC/uselessPublic.ts +++ b/src/issues/NC/uselessPublic.ts @@ -6,13 +6,16 @@ import util from 'util'; const issue: ASTIssue = { regexOrAST: 'AST', type: IssueTypes.NC, - title: 'Functions not used internally could be marked external', + title: '`public` functions not called by the contract should be declared `external` instead', detector: (files: InputType): Instance[] => { let instances: Instance[] = []; for (const file of files) { if (!!file.ast) { for (const node of findAll('FunctionDefinition', file.ast)) { + if(node.overrides){ + continue; + } if (node.visibility === 'public' && !node.virtual && node.name !== '') { const functionName = node.name; let usedInternally = false; diff --git a/src/issues/NC/uselessZeroInit.ts b/src/issues/NC/uselessZeroInit.ts new file mode 100644 index 0000000..7017397 --- /dev/null +++ b/src/issues/NC/uselessZeroInit.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Variables need not be initialized to zero', + description: 'The default value for variables is zero, so initializing them to zero is superfluous.', + regex: /(uint|address|byte).*(? The Yellow Paper makes it look like nothing is checked.\n\n> Digging into the clients, it turns out the precompile actually checks if the value is 27 or 28. No need to do this on the caller side!\n\n> The change has been merged into the Yellow Paper', + regex: /v.*(!|=)=.*27.*v.*(!|=)=.*28/g, +}; + +export default issue; diff --git a/src/utils.ts b/src/utils.ts index 8660c70..3cde76b 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -46,7 +46,7 @@ export async function sh(cmd: string) { * @dev Works with a queue, could be done with a recursive function */ export const recursiveExploration = (basePath: string, extension = '.sol'): string[] => { - let fileNames = []; + let fileNames: string[] = []; let directoryQueue = ['']; while (directoryQueue.length > 0) { let dir = directoryQueue.pop(); @@ -93,7 +93,7 @@ export const topLevelFiles = (contractId: number, files: InputType): InputType = export const getStorageVariable = (contract: ContractDefinition): string[] => { /** Build list of storage variables */ let storageVariables = [...findAll('VariableDeclaration', contract)] - .filter(e => (e.storageLocation === 'default' || e.storageLocation === 'storage') && e.mutability === 'mutable') + .filter(e => (e.stateVariable || e.storageLocation === 'storage') && !e.constant) .map(e => e.name); /** Remove function variables */ for (const func of findAll('FunctionDefinition', contract)) { diff --git a/tsconfig.json b/tsconfig.json index 6a93367..e9c8ff5 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -2,5 +2,8 @@ "extends": "ts-node/node16/tsconfig.json", "ts-node": { "files": true + }, + "compilerOptions": { + "noImplicitAny": false } }