From 7d690065a1b271c0b02ad37e01fe1580a5512efa Mon Sep 17 00:00:00 2001 From: DHARMIK GANGANI Date: Thu, 2 Nov 2023 11:28:42 +0530 Subject: [PATCH] Add files via upload --- src/issues/GAS/0.8.19.ts | 10 +++ src/issues/GAS/DeleteUse.ts | 12 +++ src/issues/GAS/ERC721A.ts | 11 +++ src/issues/GAS/SafeMath.ts | 11 +++ src/issues/GAS/abiEncodePacked.ts | 12 +++ src/issues/GAS/addressBalance.ts | 33 +++++++ src/issues/GAS/addressZero.ts | 39 +++++++++ src/issues/GAS/assemblyCall.ts | 12 +++ src/issues/GAS/assemblyEmit.ts | 24 +++++ src/issues/GAS/assemblySmallHashes.ts | 12 +++ src/issues/GAS/assignUpdateArray.ts | 64 ++++++++++++++ src/issues/GAS/blockTimestampEmit.ts | 14 +++ src/issues/GAS/boolIncursOverhead.ts | 13 +++ src/issues/GAS/cacheArrayLength.ts | 13 +++ src/issues/GAS/cacheVariable.ts | 92 ++++++++++++++++++++ src/issues/GAS/calldataViewFunctions.ts | 56 ++++++++++++ src/issues/GAS/canUseUnchecked.ts | 12 +++ src/issues/GAS/compareBoolean.ts | 13 +++ src/issues/GAS/customErrors.ts | 13 +++ src/issues/GAS/divisionMultiplicationBy2.ts | 12 +++ src/issues/GAS/emitInsideForLoop.ts | 13 +++ src/issues/GAS/emptyBlock.ts | 13 +++ src/issues/GAS/forgeStd.ts | 11 +++ src/issues/GAS/gthanlThan.ts | 13 +++ src/issues/GAS/hardCodeAddress.ts | 11 +++ src/issues/GAS/initializeDefaultValue.ts | 10 +++ src/issues/GAS/longRevertString.ts | 11 +++ src/issues/GAS/msgSender().ts | 12 +++ src/issues/GAS/nestedIf.ts | 12 +++ src/issues/GAS/payableConstractor .ts | 12 +++ src/issues/GAS/payableFunctions.ts | 13 +++ src/issues/GAS/postIncrement.ts | 12 +++ src/issues/GAS/privateForConstants.ts | 13 +++ src/issues/GAS/publicNotUsedInternally.ts | 12 +++ src/issues/GAS/shiftInsteadOfDiv.ts | 10 +++ src/issues/GAS/smallerUint.ts | 14 +++ src/issues/GAS/splitRequireStatement.ts | 10 +++ src/issues/GAS/stringTobyte32Constant.ts | 12 +++ src/issues/GAS/this.ts | 12 +++ src/issues/GAS/unchecked++i.ts | 12 +++ src/issues/GAS/unsignedComparison.ts | 11 +++ src/issues/GAS/uselessInternal.ts | 53 +++++++++++ src/issues/NC/2MultipleRewrite.ts | 11 +++ src/issues/NC/ComplexMaths.ts | 12 +++ src/issues/NC/EIP712Domain.ts | 12 +++ src/issues/NC/ElseReturn.ts | 12 +++ src/issues/NC/EmptyReciverFallback.ts | 11 +++ src/issues/NC/ExternalPublicInterface.ts | 12 +++ src/issues/NC/FunctionLong.ts | 12 +++ src/issues/NC/IfElseTarnery.ts | 14 +++ src/issues/NC/IncosisitSpacingComment.ts | 12 +++ src/issues/NC/LineLong.ts | 12 +++ src/issues/NC/ModifierMsgSender.ts | 12 +++ src/issues/NC/MultilpleConstant.ts | 12 +++ src/issues/NC/TransferInstaedOfCall.ts | 11 +++ src/issues/NC/abiEncodSignatureSelector.ts | 12 +++ src/issues/NC/address0Check.ts | 48 ++++++++++ src/issues/NC/arrayPush.ts | 12 +++ src/issues/NC/customErrorNoString.ts | 12 +++ src/issues/NC/customErrors.ts | 12 +++ src/issues/NC/dontUseReurire.ts | 11 +++ src/issues/NC/duplicateRequire.ts | 10 +++ src/issues/NC/emptyEvent.ts | 11 +++ src/issues/NC/experimentalPragma.ts | 11 +++ src/issues/NC/initializeDefaultValue.ts | 11 +++ src/issues/NC/largeMultipleOfTen.ts | 12 +++ src/issues/NC/lowerCaseConstant.ts | 12 +++ src/issues/NC/modernImport.ts | 11 +++ src/issues/NC/nameMapping.ts | 11 +++ src/issues/NC/nonReentrantBeforeModifiers.ts | 12 +++ src/issues/NC/pragmaInconsist.ts | 12 +++ src/issues/NC/requireWithString.ts | 33 +++++++ src/issues/NC/returnValueOfApprove.ts | 13 +++ src/issues/NC/stringSingleQuote.ts | 11 +++ src/issues/NC/todoLeftInTheCode.ts | 13 +++ src/issues/NC/unindexedEvent.ts | 37 ++++++++ src/issues/NC/upperCaseForNonConsatnt.ts | 12 +++ src/issues/NC/useConstants.ts | 10 +++ src/issues/NC/useScientificNotation.ts | 12 +++ src/issues/NC/uselessPublic.ts | 35 ++++++++ 80 files changed, 1331 insertions(+) create mode 100644 src/issues/GAS/0.8.19.ts create mode 100644 src/issues/GAS/DeleteUse.ts create mode 100644 src/issues/GAS/ERC721A.ts create mode 100644 src/issues/GAS/SafeMath.ts create mode 100644 src/issues/GAS/abiEncodePacked.ts create mode 100644 src/issues/GAS/addressBalance.ts create mode 100644 src/issues/GAS/addressZero.ts create mode 100644 src/issues/GAS/assemblyCall.ts create mode 100644 src/issues/GAS/assemblyEmit.ts create mode 100644 src/issues/GAS/assemblySmallHashes.ts create mode 100644 src/issues/GAS/assignUpdateArray.ts create mode 100644 src/issues/GAS/blockTimestampEmit.ts create mode 100644 src/issues/GAS/boolIncursOverhead.ts create mode 100644 src/issues/GAS/cacheArrayLength.ts create mode 100644 src/issues/GAS/cacheVariable.ts create mode 100644 src/issues/GAS/calldataViewFunctions.ts create mode 100644 src/issues/GAS/canUseUnchecked.ts create mode 100644 src/issues/GAS/compareBoolean.ts create mode 100644 src/issues/GAS/customErrors.ts create mode 100644 src/issues/GAS/divisionMultiplicationBy2.ts create mode 100644 src/issues/GAS/emitInsideForLoop.ts create mode 100644 src/issues/GAS/emptyBlock.ts create mode 100644 src/issues/GAS/forgeStd.ts create mode 100644 src/issues/GAS/gthanlThan.ts create mode 100644 src/issues/GAS/hardCodeAddress.ts create mode 100644 src/issues/GAS/initializeDefaultValue.ts create mode 100644 src/issues/GAS/longRevertString.ts create mode 100644 src/issues/GAS/msgSender().ts create mode 100644 src/issues/GAS/nestedIf.ts create mode 100644 src/issues/GAS/payableConstractor .ts create mode 100644 src/issues/GAS/payableFunctions.ts create mode 100644 src/issues/GAS/postIncrement.ts create mode 100644 src/issues/GAS/privateForConstants.ts create mode 100644 src/issues/GAS/publicNotUsedInternally.ts create mode 100644 src/issues/GAS/shiftInsteadOfDiv.ts create mode 100644 src/issues/GAS/smallerUint.ts create mode 100644 src/issues/GAS/splitRequireStatement.ts create mode 100644 src/issues/GAS/stringTobyte32Constant.ts create mode 100644 src/issues/GAS/this.ts create mode 100644 src/issues/GAS/unchecked++i.ts create mode 100644 src/issues/GAS/unsignedComparison.ts create mode 100644 src/issues/GAS/uselessInternal.ts create mode 100644 src/issues/NC/2MultipleRewrite.ts create mode 100644 src/issues/NC/ComplexMaths.ts create mode 100644 src/issues/NC/EIP712Domain.ts create mode 100644 src/issues/NC/ElseReturn.ts create mode 100644 src/issues/NC/EmptyReciverFallback.ts create mode 100644 src/issues/NC/ExternalPublicInterface.ts create mode 100644 src/issues/NC/FunctionLong.ts create mode 100644 src/issues/NC/IfElseTarnery.ts create mode 100644 src/issues/NC/IncosisitSpacingComment.ts create mode 100644 src/issues/NC/LineLong.ts create mode 100644 src/issues/NC/ModifierMsgSender.ts create mode 100644 src/issues/NC/MultilpleConstant.ts create mode 100644 src/issues/NC/TransferInstaedOfCall.ts create mode 100644 src/issues/NC/abiEncodSignatureSelector.ts create mode 100644 src/issues/NC/address0Check.ts create mode 100644 src/issues/NC/arrayPush.ts create mode 100644 src/issues/NC/customErrorNoString.ts create mode 100644 src/issues/NC/customErrors.ts create mode 100644 src/issues/NC/dontUseReurire.ts create mode 100644 src/issues/NC/duplicateRequire.ts create mode 100644 src/issues/NC/emptyEvent.ts create mode 100644 src/issues/NC/experimentalPragma.ts create mode 100644 src/issues/NC/initializeDefaultValue.ts create mode 100644 src/issues/NC/largeMultipleOfTen.ts create mode 100644 src/issues/NC/lowerCaseConstant.ts create mode 100644 src/issues/NC/modernImport.ts create mode 100644 src/issues/NC/nameMapping.ts create mode 100644 src/issues/NC/nonReentrantBeforeModifiers.ts create mode 100644 src/issues/NC/pragmaInconsist.ts create mode 100644 src/issues/NC/requireWithString.ts create mode 100644 src/issues/NC/returnValueOfApprove.ts create mode 100644 src/issues/NC/stringSingleQuote.ts create mode 100644 src/issues/NC/todoLeftInTheCode.ts create mode 100644 src/issues/NC/unindexedEvent.ts create mode 100644 src/issues/NC/upperCaseForNonConsatnt.ts create mode 100644 src/issues/NC/useConstants.ts create mode 100644 src/issues/NC/useScientificNotation.ts create mode 100644 src/issues/NC/uselessPublic.ts diff --git a/src/issues/GAS/0.8.19.ts b/src/issues/GAS/0.8.19.ts new file mode 100644 index 0000000..5151d0c --- /dev/null +++ b/src/issues/GAS/0.8.19.ts @@ -0,0 +1,10 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Reduce gas usage by moving to Solidity 0.8.19 or later', + regex: /pragma solidity 0.8.1[1-8];/g, +}; + +export default issue; diff --git a/src/issues/GAS/DeleteUse.ts b/src/issues/GAS/DeleteUse.ts new file mode 100644 index 0000000..b9f365f --- /dev/null +++ b/src/issues/GAS/DeleteUse.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Using delete instead of setting mapping/struct to 0 saves gas', + regex: /^\s*\b(?:\w+|(\w+\.\w+))\b\s*=\s*0;/g, + description:"Avoid an assignment by deleting the value instead of setting it to zero.", + gasCost:5, +}; + +export default issue; diff --git a/src/issues/GAS/ERC721A.ts b/src/issues/GAS/ERC721A.ts new file mode 100644 index 0000000..c1efab3 --- /dev/null +++ b/src/issues/GAS/ERC721A.ts @@ -0,0 +1,11 @@ +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. [Reffrence](https://nextrope.com/erc721-vs-erc721a-2/)", + regex: /ERC721\.sol/g, +}; + +export default issue; diff --git a/src/issues/GAS/SafeMath.ts b/src/issues/GAS/SafeMath.ts new file mode 100644 index 0000000..c107af8 --- /dev/null +++ b/src/issues/GAS/SafeMath.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: "Don't use SafeMath once the solidity version is 0.8.0 or greater", + description:" 0.8.0 introduces internal overflow checks, so using SafeMath is redundant and adds overhead", + regex: /import\s+.*from\s+".*\/SafeMath.sol";/g, +}; + +export default issue; diff --git a/src/issues/GAS/abiEncodePacked.ts b/src/issues/GAS/abiEncodePacked.ts new file mode 100644 index 0000000..9a92a63 --- /dev/null +++ b/src/issues/GAS/abiEncodePacked.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: '`abi.encode` is more efficient than `abi.encodePacked`', + description:'`abi.encode` uses less gas than `abi.encodePacked`: the gas saved depends on the number of arguments, with an average of ~90 per argument. Test available here.', + gasCost:5, + regex: /\babi\.encodePacked\b/g, +}; + +export default issue; diff --git a/src/issues/GAS/addressBalance.ts b/src/issues/GAS/addressBalance.ts new file mode 100644 index 0000000..6729e6a --- /dev/null +++ b/src/issues/GAS/addressBalance.ts @@ -0,0 +1,33 @@ +import { InputType, IssueTypes, Instance, ASTIssue } from '../../types'; +import { findAll } from 'solidity-ast/utils'; +import { instanceFromSRC } from '../../utils'; +//@audit-issue +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/addressZero.ts b/src/issues/GAS/addressZero.ts new file mode 100644 index 0000000..53c24ee --- /dev/null +++ b/src/issues/GAS/addressZero.ts @@ -0,0 +1,39 @@ +import { InputType, IssueTypes, Instance, ASTIssue } from '../../types'; +import { findAll } from 'solidity-ast/utils'; +import { instanceFromSRC } from '../../utils'; +import { Expression, SourceUnit } from 'solidity-ast'; +//@audit-issue +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.GAS, + title: 'Use assembly to check for `address(0)`', + description: '*Saves 6 gas per instance*', + gasCost:6, + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + + const addressZero = (node: Expression): boolean => { + return ( + node.nodeType === 'FunctionCall' && + node.kind === 'typeConversion' && + node.arguments?.[0].nodeType === 'Literal' && + node.arguments?.[0].value === '0' + ); + }; + + for (const file of files) { + if (!!file.ast) { + for (const node of findAll('BinaryOperation', file.ast)) { + if (node.operator === '!=' || node.operator === '==') { + if (addressZero(node.rightExpression) || addressZero(node.leftExpression)) { + instances.push(instanceFromSRC(file, node.src)); + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/GAS/assemblyCall.ts b/src/issues/GAS/assemblyCall.ts new file mode 100644 index 0000000..8aee513 --- /dev/null +++ b/src/issues/GAS/assemblyCall.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, +title:'With assembly, .call (bool success) transfer can be done gas-optimized', +gasCost:248, +description:"When using assembly language, it is possible to call the transfer function of an Ethereum contract in a gas-optimized way by using the .call function with specific input parameters. The .call function takes a number of input parameters, including the address of the contract to call, the amount of Ether to transfer, and a specification of the gas limit for the call. By specifying a lower gas limit than the default, it is possible to reduce the gas cost of the transfer.", +regex: /\.call/g, +}; + +export default issue; diff --git a/src/issues/GAS/assemblyEmit.ts b/src/issues/GAS/assemblyEmit.ts new file mode 100644 index 0000000..44f850f --- /dev/null +++ b/src/issues/GAS/assemblyEmit.ts @@ -0,0 +1,24 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, +title:'Use assembly to emit events', +description:"We can use assembly to emit events efficiently by utilizing `scratch space` and the `free memory pointer`. This will allow us to potentially avoid memory expansion costs. Note: In order to do this optimization safely, we will need to cache and restore the free memory pointer.", +gasCost:38, +regex: /emit/g, +}; + +export default issue; +/** +// uint256 id, uint256 value, uint256 amount +emit eventSentAmountExample(id, value, amount); + + assembly { + let memptr := mload(0x40) + mstore(0x00, calldataload(0x44)) + mstore(0x20, calldataload(0xa4)) + mstore(0x40, amount) log1( 0x00, 0x60, // keccak256("eventSentAmountExample(uint256,uint256,uint256)") 0xa622cf392588fbf2cd020ff96b2f4ebd9c76d7a4bc7f3e6b2f18012312e76bc3 ) + mstore(0x40, memptr) } + +*/ \ No newline at end of file diff --git a/src/issues/GAS/assemblySmallHashes.ts b/src/issues/GAS/assemblySmallHashes.ts new file mode 100644 index 0000000..f93dcfb --- /dev/null +++ b/src/issues/GAS/assemblySmallHashes.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Use assembly for small keccak256 hashes, in order to save gas', + description:'Use assembly for small keccak256 hashes, in order to save gas', + regex: /keccak256\s*\(\s*abi\.encode\s*\(\s*[^)]{1,40}\)\s*\)/gm, + gasCost:120, +}; + +export default issue; diff --git a/src/issues/GAS/assignUpdateArray.ts b/src/issues/GAS/assignUpdateArray.ts new file mode 100644 index 0000000..9c8bc1e --- /dev/null +++ b/src/issues/GAS/assignUpdateArray.ts @@ -0,0 +1,64 @@ +import { InputType, IssueTypes, Instance, ASTIssue } from '../../types'; +import { findAll } from 'solidity-ast/utils'; +import { instanceFromSRC } from '../../utils'; +//@audit-issue +const issue: ASTIssue = { + regexOrAST: 'AST', + 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*', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + + for (const file of files) { + if (!!file.ast) { + for (const node of findAll('Assignment', file.ast)) { + if ( + node.operator === '=' && + node.leftHandSide.nodeType === 'IndexAccess' && + node.rightHandSide.nodeType === 'BinaryOperation' + // (node.leftHandSide.typeDescriptions.typeString?.includes('[] storage') || + // node.leftHandSide.typeDescriptions.typeString?.includes('[] memory')) + ) { + let array; + if (node.leftHandSide.baseExpression.nodeType === 'Identifier') { + array = node.leftHandSide.baseExpression.name; + } + let index; + if (node.leftHandSide.indexExpression?.nodeType === 'Literal') { + index = node.leftHandSide.indexExpression.value; + } + + if (node.rightHandSide.rightExpression.nodeType === 'IndexAccess') { + if ( + (!array || + (node.rightHandSide.rightExpression.baseExpression.nodeType === 'Identifier' && + array === node.rightHandSide.rightExpression.baseExpression.name)) && + (!index || + (node.rightHandSide.rightExpression.indexExpression?.nodeType === 'Literal' && + index === node.rightHandSide.rightExpression.indexExpression?.value)) + ) { + instances.push(instanceFromSRC(file, node.src)); + } + } else if (node.rightHandSide.leftExpression.nodeType === 'IndexAccess') { + if ( + (!array || + (node.rightHandSide.leftExpression.baseExpression.nodeType === 'Identifier' && + array === node.rightHandSide.leftExpression.baseExpression.name)) && + (!index || + (node.rightHandSide.leftExpression.indexExpression?.nodeType === 'Literal' && + index === node.rightHandSide.leftExpression.indexExpression?.value)) + ) { + instances.push(instanceFromSRC(file, node.src)); + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/GAS/blockTimestampEmit.ts b/src/issues/GAS/blockTimestampEmit.ts new file mode 100644 index 0000000..a369e3b --- /dev/null +++ b/src/issues/GAS/blockTimestampEmit.ts @@ -0,0 +1,14 @@ + +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, +title:'Redundant event fields can be removed', +description:"Some parameters (block.timestamp and block.number) are added to event information by default so re-adding them wastes gas, as they are already included.", +regex: /(emit.*block\.timestamp|block\.timestamp.*emit)/gm, +gasCost:358, +}; + +export default issue; + diff --git a/src/issues/GAS/boolIncursOverhead.ts b/src/issues/GAS/boolIncursOverhead.ts new file mode 100644 index 0000000..f8187ec --- /dev/null +++ b/src/issues/GAS/boolIncursOverhead.ts @@ -0,0 +1,13 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Use uint256(1)/uint256(2) instead for true and false boolean states', + gasCost:17100, + description: + 'Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past. See [source](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27).', + regex: /(bool.?(public|private|internal).?.?[a-zA-Z]*|\=\>.*bool|struct?.{[^}]*bool)/g, // storage variable | part of mapping | struct +}; + +export default issue; diff --git a/src/issues/GAS/cacheArrayLength.ts b/src/issues/GAS/cacheArrayLength.ts new file mode 100644 index 0000000..82948fa --- /dev/null +++ b/src/issues/GAS/cacheArrayLength.ts @@ -0,0 +1,13 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: '.length should not be looked up in every loop of a for-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, + gasCost:3, +}; + +export default issue; diff --git a/src/issues/GAS/cacheVariable.ts b/src/issues/GAS/cacheVariable.ts new file mode 100644 index 0000000..bb21b66 --- /dev/null +++ b/src/issues/GAS/cacheVariable.ts @@ -0,0 +1,92 @@ +import { InputType, IssueTypes, Instance, ASTIssue } from '../../types'; +import { findAll } from 'solidity-ast/utils'; +import { getStorageVariable, instanceFromSRC } from '../../utils'; +import { Identifier } from 'solidity-ast'; +//@audit-issue +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.GAS, + title: 'State variables should be cached in stack variables rather than re-reading them from storage', + gasCost:248, + description: + 'The instances below point to the second+ access of a state variable within a function. Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.\n\n*Saves 100 gas per instance*', + 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)) { + /** Check all storage reads */ + let identifiers: Identifier[] = []; + + /** Could be an expression */ + for (const expr of findAll('ExpressionStatement', func)) { + for (const op of [...findAll('Assignment', expr)]) { + if (op.rightHandSide.nodeType === 'Identifier' && storageVariables.includes(op.rightHandSide.name)) { + identifiers.push(op.rightHandSide); + } + if (op.rightHandSide.nodeType === 'BinaryOperation') { + const bin = op.rightHandSide; + if ( + bin.rightExpression.nodeType === 'Identifier' && + storageVariables.includes(bin.rightExpression.name) + ) { + identifiers.push(bin.rightExpression); + } + + if ( + bin.leftExpression.nodeType === 'Identifier' && + storageVariables.includes(bin.leftExpression.name) + ) { + identifiers.push(bin.leftExpression); + } + } + } + } + + /** Could be a declaration */ + for (const decl of findAll('VariableDeclarationStatement', func)) { + if (decl.initialValue?.nodeType === 'Identifier') { + identifiers.push(decl.initialValue); + } + } + + /** Could be an Call */ + for (const decl of findAll('FunctionCall', func)) { + for (const id of decl.arguments) { + if (id.nodeType === 'Identifier') { + identifiers.push(id); + } + } + } + + /** Sort */ + identifiers.sort((a, b) => (a.src < b.src ? -1 : 1)); + + for (const variableName of storageVariables) { + /** Check variable isn't local */ + let isStorage = true; + for (const decl of findAll('VariableDeclaration', func)) { + if (decl.name === variableName) { + isStorage = false; + } + } + /** Check that there is more than 2 reads */ + const occurences = identifiers.filter(r => r.name === variableName); + if (isStorage && occurences?.length > 1) { + instances.push(instanceFromSRC(file, occurences[1].src)); + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/GAS/calldataViewFunctions.ts b/src/issues/GAS/calldataViewFunctions.ts new file mode 100644 index 0000000..02a86a5 --- /dev/null +++ b/src/issues/GAS/calldataViewFunctions.ts @@ -0,0 +1,56 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC } from '../../utils'; +//@audit-issue +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.GAS, + title: 'Use calldata instead of memory for function arguments that do not get mutated', + gasCost:360, + 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.', + 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.visibility === 'external' || node.visibility === 'public') { + for (const param of Object.values(node.parameters.parameters)) { + if (param.storageLocation === 'memory') { + /** Now we have a memory variable, let's check if it is modified during the call */ + const variableName = param.name; + let modified = false; + for (const assign of findAll('Assignment', node)) { + const variable = assign.leftHandSide; + + /** Array */ + if ( + variable.nodeType === 'IndexAccess' && + variable.baseExpression.nodeType === 'Identifier' && + variable.baseExpression.name === variableName + ) { + modified = true; + } + if ( + variable.nodeType === 'MemberAccess' && + variable.expression.nodeType === 'Identifier' && + variable.expression.name === variableName + ) { + modified = true; + } + } + if (!modified) { + instances.push(instanceFromSRC(file, param.src)); + } + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/GAS/canUseUnchecked.ts b/src/issues/GAS/canUseUnchecked.ts new file mode 100644 index 0000000..c17fd67 --- /dev/null +++ b/src/issues/GAS/canUseUnchecked.ts @@ -0,0 +1,12 @@ +//@audit import also take +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'For Operations that will not overflow, you could use unchecked', + regex: /([^"]*)\s*-\s*([^"]*);/gm, + gasCost:85, +}; + +export default issue; diff --git a/src/issues/GAS/compareBoolean.ts b/src/issues/GAS/compareBoolean.ts new file mode 100644 index 0000000..1767a80 --- /dev/null +++ b/src/issues/GAS/compareBoolean.ts @@ -0,0 +1,13 @@ + +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: "Don't compare boolean expressions to boolean literals", + description:'true and false are constants and it is more expensive comparing a boolean against them than directly checking the returned boolean value. `if ( == true)` => `if ()`, `if ( == false)` => `if (!)`', + regex: /==+.(true|false)/g, + gasCost:9, +}; + +export default issue; diff --git a/src/issues/GAS/customErrors.ts b/src/issues/GAS/customErrors.ts new file mode 100644 index 0000000..1df79f2 --- /dev/null +++ b/src/issues/GAS/customErrors.ts @@ -0,0 +1,13 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Use Custom Errors', + gasCost:16, + 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, +}; + +export default issue; diff --git a/src/issues/GAS/divisionMultiplicationBy2.ts b/src/issues/GAS/divisionMultiplicationBy2.ts new file mode 100644 index 0000000..2c5fd57 --- /dev/null +++ b/src/issues/GAS/divisionMultiplicationBy2.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Multiplication/division by two should use bit shifting', + description:"X * 2 is equivalent to X << 1 and X / 2 is the same as X >> 1.\n The MUL and DIV opcodes cost 5 gas, whereas SHL and SHR only cost 3 gas.", + regex: /([\w\.\[\]\(\)]+)\s*([*/])\s*(2|4|8|16|32|64|128|256|512|1024|2048)\b/g, + gasCost:20, +}; + +export default issue; diff --git a/src/issues/GAS/emitInsideForLoop.ts b/src/issues/GAS/emitInsideForLoop.ts new file mode 100644 index 0000000..279b138 --- /dev/null +++ b/src/issues/GAS/emitInsideForLoop.ts @@ -0,0 +1,13 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Use of emit inside a loop', + gasCost:1026, + description:'Emitting an event inside a loop performs a LOG op N times, where N is the loop length. Consider refactoring the code to emit the event only once at the end of loop. Gas savings should be multiplied by the average loop length.', + regex: /for\s*\([^)]*\)\s*\{[^}]*\bemit\b[^}]*\}/g, +}; + +export default issue; +//1026 \ No newline at end of file diff --git a/src/issues/GAS/emptyBlock.ts b/src/issues/GAS/emptyBlock.ts new file mode 100644 index 0000000..2447294 --- /dev/null +++ b/src/issues/GAS/emptyBlock.ts @@ -0,0 +1,13 @@ + +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, +title:'Empty Blocks Should Be Removed Or Emit Something', +description:"The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified(if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}})", +regex: /\{\};/g, +}; + +export default issue; + diff --git a/src/issues/GAS/forgeStd.ts b/src/issues/GAS/forgeStd.ts new file mode 100644 index 0000000..0ad2a47 --- /dev/null +++ b/src/issues/GAS/forgeStd.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Remove import forge-std', + description:"It's used to print the values of variables while running tests to help debug and see what's happening inside your contracts but since it's a development tool, it serves no purpose on mainnet. Also, the remember to remove the usage of calls that use forge-std when removing of the import of forge-std", + regex: /import\s+"forge-std/g, +}; + +export default issue; diff --git a/src/issues/GAS/gthanlThan.ts b/src/issues/GAS/gthanlThan.ts new file mode 100644 index 0000000..3062ebf --- /dev/null +++ b/src/issues/GAS/gthanlThan.ts @@ -0,0 +1,13 @@ +//@audit regex left mapping also include +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: '>=/ <= costs less gas than >/<', + description:'The compiler uses opcodes GT and ISZERO for solidity code that uses >, but only requires LT for >=,which saves 3 gas', + regex: /\b\s*(\w+)\s*>/g, + gasCost:3, +}; + +export default issue; diff --git a/src/issues/GAS/hardCodeAddress.ts b/src/issues/GAS/hardCodeAddress.ts new file mode 100644 index 0000000..6aafa22 --- /dev/null +++ b/src/issues/GAS/hardCodeAddress.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Use hardcode address instead of address(this) ', + description:'Instead of using address(this), it is more gas-efficient to pre-calculate and use the hardcoded address. Foundry’s script.sol and solmate’s LibRlp.sol contracts can help achieve this. References: https://book.getfoundry.sh/reference/forge-std/compute-create-address', + regex: /(address.*\(this\))./g, +}; + +export default issue; diff --git a/src/issues/GAS/initializeDefaultValue.ts b/src/issues/GAS/initializeDefaultValue.ts new file mode 100644 index 0000000..5cef73c --- /dev/null +++ b/src/issues/GAS/initializeDefaultValue.ts @@ -0,0 +1,10 @@ +import { IssueTypes, RegexIssue } from '../../types'; +//@audit Transfer into Non critical issue +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, +}; + +export default issue; diff --git a/src/issues/GAS/longRevertString.ts b/src/issues/GAS/longRevertString.ts new file mode 100644 index 0000000..d656730 --- /dev/null +++ b/src/issues/GAS/longRevertString.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Long revert strings', + regex: /(revert|require)\(.*,?.(\"|\').{33,}(\"|\')\)/g, + gasCost:18, +}; + +export default issue; diff --git a/src/issues/GAS/msgSender().ts b/src/issues/GAS/msgSender().ts new file mode 100644 index 0000000..b50c93d --- /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\(\)/g, + gasCost:16, +}; + +export default issue; diff --git a/src/issues/GAS/nestedIf.ts b/src/issues/GAS/nestedIf.ts new file mode 100644 index 0000000..817afa7 --- /dev/null +++ b/src/issues/GAS/nestedIf.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: "The use of a logical AND (&&) instead of double if is slightly less gas efficient in instances where there isn't a corresponding else statement for the given if statement", + description:'Using a double if statement instead of logical AND (&&) can provide similar short-circuiting behavior whereas double if is slightly more efficient.', + regex: /if\(.*&&.*\);/g, + gasCost:2100, +}; + +export default issue; \ No newline at end of file diff --git a/src/issues/GAS/payableConstractor .ts b/src/issues/GAS/payableConstractor .ts new file mode 100644 index 0000000..2d80075 --- /dev/null +++ b/src/issues/GAS/payableConstractor .ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Constructors can be marked payable', + description:' Payable functions cost less gas to execute, since the compiler does not have to add extra checks to ensure that a payment was not provided. A constructor can safely be marked as payable, since only the deployer would be able to pass funds, and the project itself would not pass any funds.', + regex: /constructor((?!payable).)/g, + gasCost:21, +}; +//per instance 21 gas +export default issue; diff --git a/src/issues/GAS/payableFunctions.ts b/src/issues/GAS/payableFunctions.ts new file mode 100644 index 0000000..772d5b4 --- /dev/null +++ b/src/issues/GAS/payableFunctions.ts @@ -0,0 +1,13 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Functions guaranteed to revert when called by normal users can be marked `payable`', + description: + 'If a function modifier such as `onlyOwner` is used, the function will revert if a normal user tries to pay the function. Marking the function as `payable` will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.', + regex: /function((?!payable).)*only/g, + gasCost:21, +}; + +export default issue; diff --git a/src/issues/GAS/postIncrement.ts b/src/issues/GAS/postIncrement.ts new file mode 100644 index 0000000..33f7580 --- /dev/null +++ b/src/issues/GAS/postIncrement.ts @@ -0,0 +1,12 @@ +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*', + gasCost:5, + regex: /[^ \t]+\+\+/g, +}; + +export default issue; diff --git a/src/issues/GAS/privateForConstants.ts b/src/issues/GAS/privateForConstants.ts new file mode 100644 index 0000000..f227525 --- /dev/null +++ b/src/issues/GAS/privateForConstants.ts @@ -0,0 +1,13 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Using `private` rather than `public` for constants, saves gas', + description: + "If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that [returns a tuple](https://github.com/code-423n4/2022-08-frax/blob/90f55a9ce4e25bceed3a74290b854341d8de6afa/src/contracts/FraxlendPair.sol#L156-L178) of the values of all currently-public constants. Saves **3406-3606 gas** in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table", + regex: /(public.?constant.?|constant.?public.?)[^=\n\(]*(=|;)/g, + gasCost:8406, +}; + +export default issue; diff --git a/src/issues/GAS/publicNotUsedInternally.ts b/src/issues/GAS/publicNotUsedInternally.ts new file mode 100644 index 0000000..84fecd6 --- /dev/null +++ b/src/issues/GAS/publicNotUsedInternally.ts @@ -0,0 +1,12 @@ +//@audit +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Public functions not used internally can be marked as external to save gas', + description:"Public functions that aren't used internally in Solidity contracts should be made external to optimize gas usage and improve contract efficiency. External functions can only be called from outside the contract, and their arguments are directly read from the calldata, which is more gas-efficient than loading them into memory, as is the case for public functions. By using external visibility, developers can reduce gas consumption for external calls and ensure that the contract operates more cost-effectively for users. Moreover, setting the appropriate visibility level for functions also enhances code readability and maintainability, promoting a more secure and well-structured contract design.", + regex: /^\s*function\s+public\s+\w+\s*\(.*\)\s*(?:internal|external|payable)?\s*\{/g, +}; + +export default issue; diff --git a/src/issues/GAS/shiftInsteadOfDiv.ts b/src/issues/GAS/shiftInsteadOfDiv.ts new file mode 100644 index 0000000..39e2b58 --- /dev/null +++ b/src/issues/GAS/shiftInsteadOfDiv.ts @@ -0,0 +1,10 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Use shift Right/Left instead of division/multiplication if possible', + regex: /\n[^\/\n]*\/[^\/]?[248]+/g, + gasCost:20, +}; +export default issue; diff --git a/src/issues/GAS/smallerUint.ts b/src/issues/GAS/smallerUint.ts new file mode 100644 index 0000000..b070750 --- /dev/null +++ b/src/issues/GAS/smallerUint.ts @@ -0,0 +1,14 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Usage of smaller uint/int types causes overhead', + description: + "When using a smaller int/uint type it first needs to be converted to it's 258 bit counterpart to be operated, this increases the gass cost and thus should be avoided. However it does make sense to use smaller int/uint values within structs provided you pack the struct properly.", + regex: /\b(uint|int)(8|16|32|64|128)\b\s+\w+\s*(?![^()]*event[^()]*;)/gm, + gasCost:7, +}; + +export default issue; +// 22-28 gas save per instances \ No newline at end of file diff --git a/src/issues/GAS/splitRequireStatement.ts b/src/issues/GAS/splitRequireStatement.ts new file mode 100644 index 0000000..b0f705e --- /dev/null +++ b/src/issues/GAS/splitRequireStatement.ts @@ -0,0 +1,10 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Splitting require() statements that use && saves gas', + regex: /require\(.*&&.*\);/g, +}; + +export default issue; \ No newline at end of file diff --git a/src/issues/GAS/stringTobyte32Constant.ts b/src/issues/GAS/stringTobyte32Constant.ts new file mode 100644 index 0000000..e61be9d --- /dev/null +++ b/src/issues/GAS/stringTobyte32Constant.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Bytes constants are more efficient than string constants', + description:'Using the `bytes` types for fixed-length strings is more efficient than having the EVM have to incur the overhead of string processing. Consider whether the value _needs_ to be a `string`. A good reason to keep it as a `string` would be if the variable is defined in an interface that this project does not own.', + regex: /^.*\bstring\b.*\bconstant\b.*/g, + gasCost:378, +}; + +export default issue; \ No newline at end of file diff --git a/src/issues/GAS/this.ts b/src/issues/GAS/this.ts new file mode 100644 index 0000000..2b9aaf7 --- /dev/null +++ b/src/issues/GAS/this.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Using this to access functions results in an external call, wasting gas', + description:"External calls have an overhead of 100 gas, which can be avoided by not referencing the function using this. Contracts are allowed to override their parents' functions and change the visibility from external to public, so make this change if it's required in order to call the function internally.", + regex: /this\./g, + gasCost:100, +}; + +export default issue; diff --git a/src/issues/GAS/unchecked++i.ts b/src/issues/GAS/unchecked++i.ts new file mode 100644 index 0000000..47a8694 --- /dev/null +++ b/src/issues/GAS/unchecked++i.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: "`++i/i++` should be `unchecked{++i}/unchecked{i++}` when it is not possible for them to overflow, as is the case when used in forand whileloops", + description: 'The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop', + regex: /[^ \t]+\+\+/g, + gasCost:60, +}; + +export default issue; diff --git a/src/issues/GAS/unsignedComparison.ts b/src/issues/GAS/unsignedComparison.ts new file mode 100644 index 0000000..d8c1349 --- /dev/null +++ b/src/issues/GAS/unsignedComparison.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.GAS, + title: 'Use != 0 instead of > 0 for unsigned integer comparison', + regex: /([a-z,A-Z,0-9]*>.?0|0.?<.?[a-z,A-Z,0-9]*)/g, + gasCost:6, +}; + +export default issue; diff --git a/src/issues/GAS/uselessInternal.ts b/src/issues/GAS/uselessInternal.ts new file mode 100644 index 0000000..57b3182 --- /dev/null +++ b/src/issues/GAS/uselessInternal.ts @@ -0,0 +1,53 @@ +import { ContractDefinition } from 'solidity-ast'; +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import { instanceFromSRC, topLevelFiles } from '../../utils'; +//@audit-issue +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.GAS, + title: '`internal` functions not called by the contract should be removed', + description: + 'If the functions are required by an interface, the contract should inherit from that interface and use the `override` keyword', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + + const containsCallTo = (name: string, ast: ContractDefinition): boolean => { + let res = false; + for (const functionCall of findAll('FunctionCall', ast)) { + if (functionCall.expression.nodeType === 'Identifier' && functionCall.expression.name === name) { + res = true; + } + } + return res; + }; + + for (const file of files) { + if (!!file.ast) { + for (const contract of findAll('ContractDefinition', file.ast)) { + for (const func of findAll('FunctionDefinition', contract)) { + if (func.visibility === 'internal' && !func.virtual && func.name !== '' && !func.overrides) { + const functionName = func.name; + let usedInternally = containsCallTo(functionName, contract); + + if (!usedInternally) { + /** Extract contracts extending this one */ + for (const topLevelFile of topLevelFiles(contract.id, files)) { + for (const topContract of findAll('ContractDefinition', topLevelFile.ast)) { + usedInternally = usedInternally || containsCallTo(functionName, topContract); + } + } + if (!usedInternally) { + instances.push(instanceFromSRC(file, func.src)); + } + } + } + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/2MultipleRewrite.ts b/src/issues/NC/2MultipleRewrite.ts new file mode 100644 index 0000000..bc2e0ed --- /dev/null +++ b/src/issues/NC/2MultipleRewrite.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: "`2** - 1` should be re-written as `type(uint).max`", + description:"Earlier versions of solidity can use uint(-1) instead. Expressions not including the - 1 can often be re-written to accomodate the change (e.g. by using a > rather than a >=, which will also save some gas).", + regex: /(2 \*\* (\d+)\s*-\s*1|2\*\*(\d+)\s*-\s*1)/g, +}; + +export default issue; diff --git a/src/issues/NC/ComplexMaths.ts b/src/issues/NC/ComplexMaths.ts new file mode 100644 index 0000000..f00ea0b --- /dev/null +++ b/src/issues/NC/ComplexMaths.ts @@ -0,0 +1,12 @@ + +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: "Complex math should be split into multiple steps", + description:"Consider splitting long arithmetic calculations into multiple steps to improve the code readability.", + regex: /(\([^()]+\)|\d+)\s*[-+*/]\s*(\([^()]+\)|\d+)/g, +}; + +export default issue; diff --git a/src/issues/NC/EIP712Domain.ts b/src/issues/NC/EIP712Domain.ts new file mode 100644 index 0000000..c6781a1 --- /dev/null +++ b/src/issues/NC/EIP712Domain.ts @@ -0,0 +1,12 @@ + +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: "Use EIP-5627 to describe EIP-712 domains", + description:"EIP-5267 is a standard which allows for the retrieval and description of EIP-712 hash domains. This enable external tools to allow users to view the fields and values that describe their domain.This is especially useful when a project may exist on multiple chains and or in multiple contracts, and allows users/tools to verify that the signature is for the right fork, chain, version, contract, etc.", + regex: /EIP712Domain/g, +}; + +export default issue; diff --git a/src/issues/NC/ElseReturn.ts b/src/issues/NC/ElseReturn.ts new file mode 100644 index 0000000..ddd8299 --- /dev/null +++ b/src/issues/NC/ElseReturn.ts @@ -0,0 +1,12 @@ + +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: "else block is not required", + description:"Consider moving the logic outside the else block, and then removing it (it's not required, as the function is returning a value). There will be one less level of nesting, which will improve the quality of the codebase.", + regex: /\s*else\s*{[^{}]*return[^;]*;}?/g, +}; + +export default issue; diff --git a/src/issues/NC/EmptyReciverFallback.ts b/src/issues/NC/EmptyReciverFallback.ts new file mode 100644 index 0000000..eaace8e --- /dev/null +++ b/src/issues/NC/EmptyReciverFallback.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Unused/empty receive/fallback', + regex: /fallback|receive\(\)\s+\w+(?:\s+\w+)*\s+\{[^\}]*\}/g, + description:"If the intention is for the ETH to be used, the function should call another function, otherwise it should revert `(e.g. require(msg.sender == address(weth)))`.", +}; + +export default issue; diff --git a/src/issues/NC/ExternalPublicInterface.ts b/src/issues/NC/ExternalPublicInterface.ts new file mode 100644 index 0000000..d1a2be9 --- /dev/null +++ b/src/issues/NC/ExternalPublicInterface.ts @@ -0,0 +1,12 @@ + +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: "Contract functions should use an interface", + description:"All external/public functions should extend an interface. This is useful to make sure that the whole API is extracted.", + regex: /\bfunction\s+\w+\s*\(.*\)\s+(?:external|public)/g, +}; + +export default issue; diff --git a/src/issues/NC/FunctionLong.ts b/src/issues/NC/FunctionLong.ts new file mode 100644 index 0000000..544ec8f --- /dev/null +++ b/src/issues/NC/FunctionLong.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Long functions should be refactored into multiple functions', + description: + 'Consider splitting long functions into multiple, smaller functions to improve the code readability.', + regex: /^function.{60,}/g, +}; + +export default issue; diff --git a/src/issues/NC/IfElseTarnery.ts b/src/issues/NC/IfElseTarnery.ts new file mode 100644 index 0000000..bc4a327 --- /dev/null +++ b/src/issues/NC/IfElseTarnery.ts @@ -0,0 +1,14 @@ + +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: "Use a ternary statement instead of if/else when appropriate", +description:"The if/else statement can be written in a shorthand way using the ternary operator, as it increases readability and reduces the number of lines of code.", + regex: /if\s*\([^)]+\)\s*{[^{}]*}\s*else\s*{[^{}]*}/g, +}; + +export default issue; + +//need optimization all if else take \ No newline at end of file diff --git a/src/issues/NC/IncosisitSpacingComment.ts b/src/issues/NC/IncosisitSpacingComment.ts new file mode 100644 index 0000000..2bea645 --- /dev/null +++ b/src/issues/NC/IncosisitSpacingComment.ts @@ -0,0 +1,12 @@ + +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Inconsistent spacing in comments', + regex: /\/\/\/?.*?\b\w+\s+\w+\b/gm, + description:"Some lines use // x and some use //x. The instances below point out the usages that don't follow the majority, within each file", +}; + +export default issue; diff --git a/src/issues/NC/LineLong.ts b/src/issues/NC/LineLong.ts new file mode 100644 index 0000000..058da13 --- /dev/null +++ b/src/issues/NC/LineLong.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Lines are too long', + description: + 'Maximum suggested line length is 120 characters according to the [documentation](https://docs.soliditylang.org/en/latest/style-guide.html#maximum-line-length).', + regex: /^.{121,}$/gm, +}; + +export default issue; diff --git a/src/issues/NC/ModifierMsgSender.ts b/src/issues/NC/ModifierMsgSender.ts new file mode 100644 index 0000000..ff7f303 --- /dev/null +++ b/src/issues/NC/ModifierMsgSender.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Use a modifier instead of require to check for msg.sender', + description: + 'If some functions are only allowed to be called by some specific users, consider using a modifier instead of checking with a require statement, especially if this check is done in multiple functions.', + regex: /require\s*\(\s*([^=,]+)\s*==\s*msg\.sender\s*,\s*"([^"]*)"\s*\);/g, +}; + +export default issue; diff --git a/src/issues/NC/MultilpleConstant.ts b/src/issues/NC/MultilpleConstant.ts new file mode 100644 index 0000000..bff9d06 --- /dev/null +++ b/src/issues/NC/MultilpleConstant.ts @@ -0,0 +1,12 @@ + +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: "Use a single file for system wide constants", + description:"Consider grouping all the system constants under a single file. This finding shows only the first constant for each file, for brevity.", + regex: /(?:^|\s)(constant|immutable)\s+(?:\w+\s+)*\w+(?:\s*=\s*[^;]+)?(?:\s*;)/g, +}; + +export default issue; diff --git a/src/issues/NC/TransferInstaedOfCall.ts b/src/issues/NC/TransferInstaedOfCall.ts new file mode 100644 index 0000000..379e366 --- /dev/null +++ b/src/issues/NC/TransferInstaedOfCall.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Use transfer libraries instead of low level calls', + regex: /\.call\{/g, + description:"Consider using SafeTransferLib.safeTransferETH or Address.sendValue for clearer semantic meaning instead of using a low level call.", +}; + +export default issue; diff --git a/src/issues/NC/abiEncodSignatureSelector.ts b/src/issues/NC/abiEncodSignatureSelector.ts new file mode 100644 index 0000000..4af4b32 --- /dev/null +++ b/src/issues/NC/abiEncodSignatureSelector.ts @@ -0,0 +1,12 @@ + +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: "Use abi.encodeCall() instead of abi.encodeWithSignature()/abi.encodeWithSelector()", + description:"abi.encodeCall() has compiler [type safety](https://github.com/OpenZeppelin/openzeppelin-contracts/issues/3693), whereas the other two functions do not", + regex: /abi\.encodeWith(Signature|Selector)\(/gm, +}; + +export default issue; diff --git a/src/issues/NC/address0Check.ts b/src/issues/NC/address0Check.ts new file mode 100644 index 0000000..187a76f --- /dev/null +++ b/src/issues/NC/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.NC, + 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/NC/arrayPush.ts b/src/issues/NC/arrayPush.ts new file mode 100644 index 0000000..9e0b0b8 --- /dev/null +++ b/src/issues/NC/arrayPush.ts @@ -0,0 +1,12 @@ + +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: "Array is push()ed but not pop()ed", + description:"Array entries are added but are never removed. Consider whether this should be the case, or whether there should be a maximum, or whether old entries should be removed. Cases where there are specific potential problems will be flagged separately under a different issue.", + regex: /\.push\(/gm, +}; + +export default issue; diff --git a/src/issues/NC/customErrorNoString.ts b/src/issues/NC/customErrorNoString.ts new file mode 100644 index 0000000..93d94d1 --- /dev/null +++ b/src/issues/NC/customErrorNoString.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Custom error without details', + description: + 'Consider adding some parameters to the error to indicate which user or values caused the failure.', + regex: /error\s+(\w+_\w|\w)+\s*\(\s*\);/g, +}; + +export default issue; diff --git a/src/issues/NC/customErrors.ts b/src/issues/NC/customErrors.ts new file mode 100644 index 0000000..14ae783 --- /dev/null +++ b/src/issues/NC/customErrors.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Use Custom Errors', + description: + 'Custom errors are available from solidity version 0.8.4. Custom errors are more easily processed in try-catch blocks, and are easier to re-use and maintain.', + regex: /(require|revert)\(.*,?".*"\)/g, +}; + +export default issue; diff --git a/src/issues/NC/dontUseReurire.ts b/src/issues/NC/dontUseReurire.ts new file mode 100644 index 0000000..a13bc56 --- /dev/null +++ b/src/issues/NC/dontUseReurire.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: "Custom errors should be used rather than `revert()/require()`", + description:"Custom errors are available from solidity version 0.8.4. Custom errors are more easily processed in try-catch blocks, and are easier to re-use and maintain", + regex: /((?![^\n]*(uint|int|public))[^\n]*)([[:blank:]]|\()((?!(10|1e|32|256|128))[0-9e]{2,})/g, +}; + +export default issue; diff --git a/src/issues/NC/duplicateRequire.ts b/src/issues/NC/duplicateRequire.ts new file mode 100644 index 0000000..7dbbd1e --- /dev/null +++ b/src/issues/NC/duplicateRequire.ts @@ -0,0 +1,10 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Duplicated `require()/revert()` checks chould be refactored to a modifier Or function to save gas', + regex: /((require|revert)\s*\(\s*([^)]+)\s*\)\s*;(?=.*\1\s*\(\s*\2\s*\)\s*;)|(if\s*\([^)]+\))[\s\S]*\1)/gs, +}; + +export default issue; diff --git a/src/issues/NC/emptyEvent.ts b/src/issues/NC/emptyEvent.ts new file mode 100644 index 0000000..8f38640 --- /dev/null +++ b/src/issues/NC/emptyEvent.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Event emit should emit a parameter', + description:'Some emitted events do not have any emitted parameters. It is recommended to add some parameter such as state changes or value changes when events are emitted', + regex: /\bevent\s+(\w+)\s*\(\s*\)\s*;/gm, +}; + +export default issue; diff --git a/src/issues/NC/experimentalPragma.ts b/src/issues/NC/experimentalPragma.ts new file mode 100644 index 0000000..672904a --- /dev/null +++ b/src/issues/NC/experimentalPragma.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, +title:'experimental pragmas may be dangerous/deprecated', +description:'experimental pragmas may be dangerous/deprecated', +regex: /pragma experimental/g, +}; + +export default issue; diff --git a/src/issues/NC/initializeDefaultValue.ts b/src/issues/NC/initializeDefaultValue.ts new file mode 100644 index 0000000..fb2c510 --- /dev/null +++ b/src/issues/NC/initializeDefaultValue.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: "Don't initialize variables with default value", + description:"It's not necessary to initialize a variable with its default value, and it's actually worse in gas terms as it adds an overhead.", + regex: /((uint|int)[0-9]*?.*[a-z,A-Z,0-9]+.?=.?0;)|(bool.[a-z,A-Z,0-9]+.?=.?false;)/g, +}; + +export default issue; diff --git a/src/issues/NC/largeMultipleOfTen.ts b/src/issues/NC/largeMultipleOfTen.ts new file mode 100644 index 0000000..6365d22 --- /dev/null +++ b/src/issues/NC/largeMultipleOfTen.ts @@ -0,0 +1,12 @@ + +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: "Large multiples of ten should use scientific notation", + description:"Use a scientific notation rather than decimal literals (e.g. 1e6 instead of 1000000), for better code readability.", + regex: /\b[1-9]\d{4,}(?=\b)/g, +}; + +export default issue; diff --git a/src/issues/NC/lowerCaseConstant.ts b/src/issues/NC/lowerCaseConstant.ts new file mode 100644 index 0000000..3a65dcf --- /dev/null +++ b/src/issues/NC/lowerCaseConstant.ts @@ -0,0 +1,12 @@ + +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: "Constant names should be UPPERCASE", + description:"Constants should be named with all capital letters with underscores separating words. Examples: MAX_BLOCKS, TOKEN_NAME. Documentation.", + regex: /constant\s[a-z]+/g, +}; + +export default issue; diff --git a/src/issues/NC/modernImport.ts b/src/issues/NC/modernImport.ts new file mode 100644 index 0000000..affe5c7 --- /dev/null +++ b/src/issues/NC/modernImport.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Import declarations should import specific identifiers, rather than the whole file', + description:'Using import declarations of the form import {} from "some/file.sol" avoids polluting the symbol namespace making flattened files smaller, and speeds up compilation (but does not save any gas)', + regex: /import\s+("\.\/|"@)/gm, +}; + +export default issue; diff --git a/src/issues/NC/nameMapping.ts b/src/issues/NC/nameMapping.ts new file mode 100644 index 0000000..2cd11be --- /dev/null +++ b/src/issues/NC/nameMapping.ts @@ -0,0 +1,11 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: "Consider using named mappings.", + description:"Consider moving t.o solidity version 0.8.18 or later, and using named mappings to make it easier to understand the purpose of each mapping", + regex: /mapping\(address[^a-zA-Z0-9]*$/gm, +}; + +export default issue; diff --git a/src/issues/NC/nonReentrantBeforeModifiers.ts b/src/issues/NC/nonReentrantBeforeModifiers.ts new file mode 100644 index 0000000..0f2c45b --- /dev/null +++ b/src/issues/NC/nonReentrantBeforeModifiers.ts @@ -0,0 +1,12 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + 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, +}; + +export default issue; diff --git a/src/issues/NC/pragmaInconsist.ts b/src/issues/NC/pragmaInconsist.ts new file mode 100644 index 0000000..e4a39c2 --- /dev/null +++ b/src/issues/NC/pragmaInconsist.ts @@ -0,0 +1,12 @@ + +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: "Inconsistent method of specifying a floating pragma", + description:"Some files use >=, while others use ^. The instances below are examples of the method that has the fewest instances for a specific version.", + regex: /pragma\s+solidity\s+(([>=<]|\^)=?\s*[0-9]*\.[0-9]+\.[0-9]+)(?:\s*;|$)/g, +}; + +export default issue; diff --git a/src/issues/NC/requireWithString.ts b/src/issues/NC/requireWithString.ts new file mode 100644 index 0000000..3d89372 --- /dev/null +++ b/src/issues/NC/requireWithString.ts @@ -0,0 +1,33 @@ +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.NC, + title: ' `require()` / `revert()` statements should have descriptive reason strings', + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + + for (const file of files) { + if (!!file.ast) { + // Revert + for (const node of findAll('RevertStatement', file.ast)) { + if (node.errorCall.expression.nodeType !== 'Identifier' || !node.errorCall.expression.name) { + instances.push(instanceFromSRC(file, node.src)); + } + } + + // Require + for (const node of findAll('FunctionCall', file.ast)) { + if (node.expression.nodeType === 'Identifier' && node.expression.name === 'require' && !node.arguments?.[1]) { + instances.push(instanceFromSRC(file, node.src)); + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/returnValueOfApprove.ts b/src/issues/NC/returnValueOfApprove.ts new file mode 100644 index 0000000..aaf680e --- /dev/null +++ b/src/issues/NC/returnValueOfApprove.ts @@ -0,0 +1,13 @@ +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/stringSingleQuote.ts b/src/issues/NC/stringSingleQuote.ts new file mode 100644 index 0000000..25023ee --- /dev/null +++ b/src/issues/NC/stringSingleQuote.ts @@ -0,0 +1,11 @@ +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: /'[^']*'/g, +}; + +export default issue; diff --git a/src/issues/NC/todoLeftInTheCode.ts b/src/issues/NC/todoLeftInTheCode.ts new file mode 100644 index 0000000..e72ae96 --- /dev/null +++ b/src/issues/NC/todoLeftInTheCode.ts @@ -0,0 +1,13 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +// TODO: This finding won't work until we add a flag to specify findings +// that either also search in commentsOr only search in comments +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'TODO Left in the code', + description: "TODOs may signal that a feature is missing or not ready for audit, consider resolving the issue and removing the TODO comment", + regex: /(TODO)/g, +}; + +export default issue; diff --git a/src/issues/NC/unindexedEvent.ts b/src/issues/NC/unindexedEvent.ts new file mode 100644 index 0000000..01f1799 --- /dev/null +++ b/src/issues/NC/unindexedEvent.ts @@ -0,0 +1,37 @@ +import { findAll } from 'solidity-ast/utils'; +import { ASTIssue, InputType, Instance, IssueTypes, RegexIssue } from '../../types'; +import util from 'util'; +import { instanceFromSRC } from '../../utils'; + +const issue: ASTIssue = { + regexOrAST: 'AST', + type: IssueTypes.NC, + title: 'Event is missing `indexed` fields', + description: + "Index event fields make the field more quickly accessible to off-chain tools that parse events. 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). 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 fields, all of the fields should be indexed.", + detector: (files: InputType): Instance[] => { + let instances: Instance[] = []; + + for (const file of files) { + if (!!file.ast) { + for (const event of findAll('EventDefinition', file.ast)) { + let indexedCount = 0; + let nonIndexed = false; + for (const param of event.parameters.parameters) { + if (param.indexed) { + indexedCount += 1; + } else { + nonIndexed = true; + } + } + if (nonIndexed && indexedCount < 3) { + instances.push(instanceFromSRC(file, event.src)); + } + } + } + } + return instances; + }, +}; + +export default issue; diff --git a/src/issues/NC/upperCaseForNonConsatnt.ts b/src/issues/NC/upperCaseForNonConsatnt.ts new file mode 100644 index 0000000..9b7852e --- /dev/null +++ b/src/issues/NC/upperCaseForNonConsatnt.ts @@ -0,0 +1,12 @@ +//@audit +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: "Don't use uppercase for non constant/immutable variables", + description:"Variables which are not constants or immutable should not be declared in all uppercase.", + regex: /constant\s[a-z]+/g, +}; + +export default issue; diff --git a/src/issues/NC/useConstants.ts b/src/issues/NC/useConstants.ts new file mode 100644 index 0000000..4b31a57 --- /dev/null +++ b/src/issues/NC/useConstants.ts @@ -0,0 +1,10 @@ +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: 'Constants should be defined rather than using magic numbers', + regex: /((?![^\n]*(uint|int|public))[^\n]*)([[:blank:]]|\()((?!(10|1e|32|256|128))[0-9e]{2,})/g, +}; + +export default issue; diff --git a/src/issues/NC/useScientificNotation.ts b/src/issues/NC/useScientificNotation.ts new file mode 100644 index 0000000..bcf4920 --- /dev/null +++ b/src/issues/NC/useScientificNotation.ts @@ -0,0 +1,12 @@ + +import { IssueTypes, RegexIssue } from '../../types'; + +const issue: RegexIssue = { + regexOrAST: 'Regex', + type: IssueTypes.NC, + title: "Use of exponentiation instead of scientific notation", + description:"Use a scientific notation rather than exponentiation (e.g. 1e18 instead of 10**18): although the compiler is capable of optimizing it, it is considered good coding practice to utilize idioms that don't rely on compiler optimization, whenever possible.", + regex: /10\*\*\d+/gm, +}; + +export default issue; diff --git a/src/issues/NC/uselessPublic.ts b/src/issues/NC/uselessPublic.ts new file mode 100644 index 0000000..73b9165 --- /dev/null +++ b/src/issues/NC/uselessPublic.ts @@ -0,0 +1,35 @@ +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: 'Functions not used internally could be marked external', + 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.visibility === 'public' && !node.virtual && node.name !== '') { + const functionName = node.name; + let usedInternally = false; + for (const functionCall of findAll('FunctionCall', file.ast)) { + if (functionCall.expression.nodeType === 'Identifier' && functionCall.expression.name === functionName) { + usedInternally = true; + } + } + if (!usedInternally) { + instances.push(instanceFromSRC(file, node.src)); + } + } + } + } + } + return instances; + }, +}; + +export default issue;