Skip to content

Commit

Permalink
Dravee giving back some love (Picodes#43)
Browse files Browse the repository at this point in the history
* Removing dups

* Adding solc versions

* Compilation fixes

* Giving back some love

* This was better before

* More flexibility

---------

Co-authored-by: Picodes <41673773+Picodes@users.noreply.github.com>
  • Loading branch information
JustDravee and Picodes authored Feb 16, 2024
1 parent 098627c commit 8a9d1eb
Show file tree
Hide file tree
Showing 150 changed files with 3,142 additions and 102 deletions.
24 changes: 16 additions & 8 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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"
},
Expand Down
18 changes: 17 additions & 1 deletion src/compile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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-'))
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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)}`);
Expand Down Expand Up @@ -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}`);
Expand Down
25 changes: 8 additions & 17 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 <string>', '.txt file containing the contest scope')
.option('-g, --github <string>', 'github url to generate links to code')
.option('-o, --out <string>', '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);
12 changes: 12 additions & 0 deletions src/issues/GAS/ERC721A.ts
Original file line number Diff line number Diff line change
@@ -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;
12 changes: 12 additions & 0 deletions src/issues/GAS/_msgSender.ts
Original file line number Diff line number Diff line change
@@ -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;
12 changes: 12 additions & 0 deletions src/issues/GAS/addPlusEqual.ts
Original file line number Diff line number Diff line change
@@ -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;
33 changes: 0 additions & 33 deletions src/issues/GAS/addressBalance.ts

This file was deleted.

2 changes: 1 addition & 1 deletion src/issues/GAS/assignUpdateArray.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = [];

Expand Down
12 changes: 12 additions & 0 deletions src/issues/GAS/boolCompare.ts
Original file line number Diff line number Diff line change
@@ -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;
2 changes: 1 addition & 1 deletion src/issues/GAS/cacheArrayLength.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
5 changes: 4 additions & 1 deletion src/issues/GAS/calldataViewFunctions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 * <mem_array>.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') {
Expand Down
6 changes: 3 additions & 3 deletions src/issues/GAS/customErrors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: <https://blog.soliditylang.org/2021/04/21/custom-errors/>:\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;
12 changes: 12 additions & 0 deletions src/issues/GAS/delegatecallAddressCheck.ts
Original file line number Diff line number Diff line change
@@ -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;
52 changes: 52 additions & 0 deletions src/issues/GAS/dontCacheIfUsedOnce.ts
Original file line number Diff line number Diff line change
@@ -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;
64 changes: 64 additions & 0 deletions src/issues/GAS/immutableConstructor.ts
Original file line number Diff line number Diff line change
@@ -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<string, string>();
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;
Loading

0 comments on commit 8a9d1eb

Please sign in to comment.