From 75e200c833da3b1cbda45490a84448a77829ee57 Mon Sep 17 00:00:00 2001 From: dbale-altoros Date: Tue, 12 Mar 2024 15:59:20 -0300 Subject: [PATCH] fix: payable fallback --- conf/rulesets/solhint-all.js | 2 +- docs/rules.md | 26 ++++----- docs/rules/best-practises/payable-fallback.md | 29 +++++----- docs/rules/naming/interface-starts-with-i.md | 6 +- e2e/08-autofix/payable-fallback/Foo1.sol | 58 ++++++++++++------- .../payable-fallback/Foo1AfterFix.sol | 58 ++++++++++++------- .../payable-fallback/Foo1BeforeFix.sol | 58 ++++++++++++------- .../best-practises/interface-starts-with-i.js | 2 +- lib/rules/best-practises/payable-fallback.js | 56 +++++++++++++++--- ...t-payable.js => --fallback-not-payable.js} | 0 ...lback-payable.js => --fallback-payable.js} | 0 test/rules/best-practises/payable-fallback.js | 29 ++++++---- .../gas-named-return-values.js | 1 - test/rules/naming/func-named-parameters.js | 1 - 14 files changed, 209 insertions(+), 117 deletions(-) rename test/fixtures/best-practises/{fallback-not-payable.js => --fallback-not-payable.js} (100%) rename test/fixtures/best-practises/{fallback-payable.js => --fallback-payable.js} (100%) diff --git a/conf/rulesets/solhint-all.js b/conf/rulesets/solhint-all.js index 2344912c..b10c1e1a 100644 --- a/conf/rulesets/solhint-all.js +++ b/conf/rulesets/solhint-all.js @@ -8,7 +8,7 @@ module.exports = Object.freeze({ 'code-complexity': ['warn', 7], 'explicit-types': ['warn', 'explicit'], 'function-max-lines': ['warn', 50], - 'interface-starts-with-i': 'warning', + 'interface-starts-with-i': 'warn', 'max-line-length': ['error', 120], 'max-states-count': ['warn', 15], 'no-console': 'error', diff --git a/docs/rules.md b/docs/rules.md index f1ed4375..61bc0c8b 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -19,7 +19,7 @@ title: "Rule Index of Solhint" | [no-unused-import](./rules/best-practises/no-unused-import.md) | Imported object name is not being used by the contract. | $~~~~~~~~$✔️ | | | [no-unused-vars](./rules/best-practises/no-unused-vars.md) | Variable "name" is unused. | $~~~~~~~~$✔️ | | | [one-contract-per-file](./rules/best-practises/one-contract-per-file.md) | Enforces the use of ONE Contract per file see [here](https://docs.soliditylang.org/en/v0.8.21/style-guide.html#contract-and-library-names) | $~~~~~~~~$✔️ | | -| [payable-fallback](./rules/best-practises/payable-fallback.md) | When fallback is not payable you will not be able to receive ethers. | $~~~~~~~~$✔️ | | +| [payable-fallback](./rules/best-practises/payable-fallback.md) | When fallback is not payable and there is no receive function you will not be able to receive currency. | $~~~~~~~~$✔️ | | | [reason-string](./rules/best-practises/reason-string.md) | Require or revert statement must have a reason string and check that each reason string is at most N characters long. | $~~~~~~~~$✔️ | | | [constructor-syntax](./rules/best-practises/constructor-syntax.md) | Constructors should use the new constructor keyword. | | | @@ -50,18 +50,18 @@ title: "Rule Index of Solhint" ## Gas Consumption Rules -| Rule Id | Error | Recommended | Deprecated | -| ----------------------------------------------------------------------------- | -------------------------------------------------------------------- | ------------ | ---------- | -| [gas-calldata-parameters](./rules/gas-consumption/gas-calldata-parameters.md) | Suggest calldata keyword on function arguments when read only | | | -| [gas-custom-errors](./rules/gas-consumption/gas-custom-errors.md) | Enforces the use of Custom Errors over Require and Revert statements | $~~~~~~~~$✔️ | | -| [gas-increment-by-one](./rules/gas-consumption/gas-increment-by-one.md) | Suggest incrementation by one like this ++i instead of other type | | | -| [gas-indexed-events](./rules/gas-consumption/gas-indexed-events.md) | Suggest indexed arguments on events for uint, bool and address | | | -| [gas-multitoken1155](./rules/gas-consumption/gas-multitoken1155.md) | ERC1155 is a cheaper non-fungible token than ERC721 | | | -| [gas-named-return-values](./rules/gas-consumption/gas-named-return-values.md) | Enforce the return values of a function to be named | | | -| [gas-small-strings](./rules/gas-consumption/gas-small-strings.md) | Keep strings smaller than 32 bytes | | | -| [gas-strict-inequalities](./rules/gas-consumption/gas-strict-inequalities.md) | Suggest Strict Inequalities over non Strict ones | | | -| [gas-struct-packing](./rules/gas-consumption/gas-struct-packing.md) | Suggest to re-arrange struct packing order when it is inefficient | | | -| [gas-length-in-loops](./rules/gas-consumption/gas-length-in-loops.md) | Suggest replacing object.length in a loop condition to avoid calculation on each lap | | | +| Rule Id | Error | Recommended | Deprecated | +| ----------------------------------------------------------------------------- | ------------------------------------------------------------------------------------ | ------------ | ---------- | +| [gas-calldata-parameters](./rules/gas-consumption/gas-calldata-parameters.md) | Suggest calldata keyword on function arguments when read only | | | +| [gas-custom-errors](./rules/gas-consumption/gas-custom-errors.md) | Enforces the use of Custom Errors over Require and Revert statements | $~~~~~~~~$✔️ | | +| [gas-increment-by-one](./rules/gas-consumption/gas-increment-by-one.md) | Suggest incrementation by one like this ++i instead of other type | | | +| [gas-indexed-events](./rules/gas-consumption/gas-indexed-events.md) | Suggest indexed arguments on events for uint, bool and address | | | +| [gas-length-in-loops](./rules/gas-consumption/gas-length-in-loops.md) | Suggest replacing object.length in a loop condition to avoid calculation on each lap | | | +| [gas-multitoken1155](./rules/gas-consumption/gas-multitoken1155.md) | ERC1155 is a cheaper non-fungible token than ERC721 | | | +| [gas-named-return-values](./rules/gas-consumption/gas-named-return-values.md) | Enforce the return values of a function to be named | | | +| [gas-small-strings](./rules/gas-consumption/gas-small-strings.md) | Keep strings smaller than 32 bytes | | | +| [gas-strict-inequalities](./rules/gas-consumption/gas-strict-inequalities.md) | Suggest Strict Inequalities over non Strict ones | | | +| [gas-struct-packing](./rules/gas-consumption/gas-struct-packing.md) | Suggest to re-arrange struct packing order when it is inefficient | | | ## Miscellaneous diff --git a/docs/rules/best-practises/payable-fallback.md b/docs/rules/best-practises/payable-fallback.md index be0f61fc..ca8d6b5c 100644 --- a/docs/rules/best-practises/payable-fallback.md +++ b/docs/rules/best-practises/payable-fallback.md @@ -12,7 +12,7 @@ title: "payable-fallback | Solhint" ## Description -When fallback is not payable you will not be able to receive ethers. +When fallback is not payable and there is no receive function you will not be able to receive currency. ## Options This rule accepts a string option of rule severity. Must be one of "error", "warn", "off". Default to warn. @@ -28,6 +28,7 @@ This rule accepts a string option of rule severity. Must be one of "error", "war ### Notes - Solhint allows this rule to automatically fix the code with `--fix` option +- Instead of having a fallback function to receive native currency it is recommended to code a receive() function [[here]](https://docs.soliditylang.org/en/v0.8.24/contracts.html#fallback-function) ## Examples ### 👍 Examples of **correct** code for this rule @@ -35,14 +36,13 @@ This rule accepts a string option of rule severity. Must be one of "error", "war #### Fallback is payable ```solidity +function() public payable {} +``` + +#### Fallback is payable - pragma solidity 0.4.4; - - - contract A { - function () public payable {} - } - +```solidity +fallback() external payable {} ``` ### 👎 Examples of **incorrect** code for this rule @@ -50,14 +50,13 @@ This rule accepts a string option of rule severity. Must be one of "error", "war #### Fallback is not payable ```solidity +function() {} function g() payable {} +``` + +#### Fallback is not payable - pragma solidity 0.4.4; - - - contract A { - function () public {} - } - +```solidity +fallback() {} function g() payable {} ``` ## Version diff --git a/docs/rules/naming/interface-starts-with-i.md b/docs/rules/naming/interface-starts-with-i.md index 89a468bc..f8148240 100644 --- a/docs/rules/naming/interface-starts-with-i.md +++ b/docs/rules/naming/interface-starts-with-i.md @@ -6,19 +6,19 @@ title: "interface-starts-with-i | Solhint" # interface-starts-with-i ![Category Badge](https://img.shields.io/badge/-Style%20Guide%20Rules-informational) -![Default Severity Badge warning](https://img.shields.io/badge/Default%20Severity-warning-undefined) +![Default Severity Badge warn](https://img.shields.io/badge/Default%20Severity-warn-yellow) ## Description Solidity Interfaces names should start with an `I` ## Options -This rule accepts a string option of rule severity. Must be one of "error", "warn", "off". Default to warning. +This rule accepts a string option of rule severity. Must be one of "error", "warn", "off". Default to warn. ### Example Config ```json { "rules": { - "interface-starts-with-i": "warning" + "interface-starts-with-i": "warn" } } ``` diff --git a/e2e/08-autofix/payable-fallback/Foo1.sol b/e2e/08-autofix/payable-fallback/Foo1.sol index eb1cfdaf..035b0353 100644 --- a/e2e/08-autofix/payable-fallback/Foo1.sol +++ b/e2e/08-autofix/payable-fallback/Foo1.sol @@ -1,22 +1,11 @@ // SPDX-License-Identifier: Apache-2.0 pragma solidity ^0.8.0; -contract Generic { - - constructor() {} +contract FallbackNoReceive1 { + constructor() {} function anyFunction() {} - //// fallback with receive - receive() public {} - - receive() external onlyOwner {} - - receive() external virtual { - uint256 anUintToFillSpace; - } - - //// fallback with no name function() public {} function() { @@ -29,22 +18,49 @@ contract Generic { uint256 anUintToFillSpace; } - //// fallback explicit - fallback() {} + fallback() external {} - fallback() { + fallback() external { uint256 anUintToFillSpace; } - fallback() external onlyOwner{ + fallback() external onlyOwner { uint256 anUintToFillSpace; } - fallback() virtual {} - + fallback() external virtual {} fallback() external payable {} - function() external payable {} - receive() public virtual payable {} + function() external payable {} +} + +contract FallbackWithReceive { + constructor() {} + + function() { + uint256 anUintToFillSpace; + } + + function() external onlyOwner {} + + fallback() external { + uint256 anUintToFillSpace; + } + + receive() external payable onlyOwner {} +} + +contract FallbackNoReceive2 { + constructor() {} + + function() { + uint256 anUintToFillSpace; + } + + function() external onlyOwner {} + + fallback() external { + uint256 anUintToFillSpace; + } } diff --git a/e2e/08-autofix/payable-fallback/Foo1AfterFix.sol b/e2e/08-autofix/payable-fallback/Foo1AfterFix.sol index f32e5c55..9bdfe87b 100644 --- a/e2e/08-autofix/payable-fallback/Foo1AfterFix.sol +++ b/e2e/08-autofix/payable-fallback/Foo1AfterFix.sol @@ -1,22 +1,11 @@ // SPDX-License-Identifier: Apache-2.0 pragma solidity ^0.8.0; -contract Generic { - - constructor() {} +contract FallbackNoReceive1 { + constructor() {} function anyFunction() {} - //// fallback with receive - receive() payable public {} - - receive() payable external onlyOwner {} - - receive() payable external virtual { - uint256 anUintToFillSpace; - } - - //// fallback with no name function() payable public {} function() payable { @@ -29,22 +18,49 @@ contract Generic { uint256 anUintToFillSpace; } - //// fallback explicit - fallback() payable {} + fallback() payable external {} - fallback() payable { + fallback() payable external { uint256 anUintToFillSpace; } - fallback() payable external onlyOwner{ + fallback() payable external onlyOwner { uint256 anUintToFillSpace; } - fallback() payable virtual {} - + fallback() payable external virtual {} fallback() external payable {} - function() external payable {} - receive() public virtual payable {} + function() external payable {} +} + +contract FallbackWithReceive { + constructor() {} + + function() { + uint256 anUintToFillSpace; + } + + function() external onlyOwner {} + + fallback() external { + uint256 anUintToFillSpace; + } + + receive() external payable onlyOwner {} +} + +contract FallbackNoReceive2 { + constructor() {} + + function() payable { + uint256 anUintToFillSpace; + } + + function() payable external onlyOwner {} + + fallback() payable external { + uint256 anUintToFillSpace; + } } diff --git a/e2e/08-autofix/payable-fallback/Foo1BeforeFix.sol b/e2e/08-autofix/payable-fallback/Foo1BeforeFix.sol index eb1cfdaf..035b0353 100644 --- a/e2e/08-autofix/payable-fallback/Foo1BeforeFix.sol +++ b/e2e/08-autofix/payable-fallback/Foo1BeforeFix.sol @@ -1,22 +1,11 @@ // SPDX-License-Identifier: Apache-2.0 pragma solidity ^0.8.0; -contract Generic { - - constructor() {} +contract FallbackNoReceive1 { + constructor() {} function anyFunction() {} - //// fallback with receive - receive() public {} - - receive() external onlyOwner {} - - receive() external virtual { - uint256 anUintToFillSpace; - } - - //// fallback with no name function() public {} function() { @@ -29,22 +18,49 @@ contract Generic { uint256 anUintToFillSpace; } - //// fallback explicit - fallback() {} + fallback() external {} - fallback() { + fallback() external { uint256 anUintToFillSpace; } - fallback() external onlyOwner{ + fallback() external onlyOwner { uint256 anUintToFillSpace; } - fallback() virtual {} - + fallback() external virtual {} fallback() external payable {} - function() external payable {} - receive() public virtual payable {} + function() external payable {} +} + +contract FallbackWithReceive { + constructor() {} + + function() { + uint256 anUintToFillSpace; + } + + function() external onlyOwner {} + + fallback() external { + uint256 anUintToFillSpace; + } + + receive() external payable onlyOwner {} +} + +contract FallbackNoReceive2 { + constructor() {} + + function() { + uint256 anUintToFillSpace; + } + + function() external onlyOwner {} + + fallback() external { + uint256 anUintToFillSpace; + } } diff --git a/lib/rules/best-practises/interface-starts-with-i.js b/lib/rules/best-practises/interface-starts-with-i.js index 9c7fafe3..f47db110 100644 --- a/lib/rules/best-practises/interface-starts-with-i.js +++ b/lib/rules/best-practises/interface-starts-with-i.js @@ -23,7 +23,7 @@ const meta = { }, isDefault: false, recommended: false, - defaultSetup: 'warning', + defaultSetup: 'warn', schema: [], } diff --git a/lib/rules/best-practises/payable-fallback.js b/lib/rules/best-practises/payable-fallback.js index 473c7c67..6655bace 100644 --- a/lib/rules/best-practises/payable-fallback.js +++ b/lib/rules/best-practises/payable-fallback.js @@ -1,24 +1,32 @@ const BaseChecker = require('../base-checker') -const { isFallbackFunction } = require('../../common/ast-types') const ruleId = 'payable-fallback' const meta = { type: 'best-practises', docs: { - description: 'When fallback is not payable you will not be able to receive ethers.', + description: + 'When fallback is not payable and there is no receive function you will not be able to receive currency.', category: 'Best Practise Rules', examples: { good: [ { description: 'Fallback is payable', - code: require('../../../test/fixtures/best-practises/fallback-payable'), + code: 'function() public payable {}', + }, + { + description: 'Fallback is payable', + code: 'fallback() external payable {}', }, ], bad: [ { description: 'Fallback is not payable', - code: require('../../../test/fixtures/best-practises/fallback-not-payable'), + code: 'function() {} function g() payable {}', + }, + { + description: 'Fallback is not payable', + code: 'fallback() {} function g() payable {}', }, ], }, @@ -26,6 +34,9 @@ const meta = { { note: 'Solhint allows this rule to automatically fix the code with `--fix` option', }, + { + note: 'Instead of having a fallback function to receive native currency it is recommended to code a receive() function [[here]](https://docs.soliditylang.org/en/v0.8.24/contracts.html#fallback-function)', + }, ], }, @@ -41,13 +52,40 @@ class PayableFallbackChecker extends BaseChecker { super(reporter, ruleId, meta) } + ContractDefinition(node) { + if (node.kind !== 'contract') return + this.contractName = node.name + + this.receiveFunctionPresent = false + this.fallbackFunctionPresentAndNotPayable = false + this.nodesError = [] + } + + 'ContractDefinition:exit'(node) { + if (node.kind !== 'contract') return + this.outputReport() + this.contractName = '' + } + FunctionDefinition(node) { - if (isFallbackFunction(node)) { - if (node.stateMutability !== 'payable') { + if (node.isReceiveEther) { + this.receiveFunctionPresent = true + return + } + + if (node.isFallback && node.stateMutability !== 'payable') { + this.fallbackFunctionPresentAndNotPayable = true + this.nodesError.push(node) + } + } + + outputReport() { + if (!this.receiveFunctionPresent && this.fallbackFunctionPresentAndNotPayable) { + for (let i = 0; i < this.nodesError.length; i++) { this.warn( - node, - 'Fallback should be external and payable to accept native currency', - this.fixStatement(node) + this.nodesError[i], + `Contract [${this.contractName}] Fallback should be payable and external (code a receive() function is recommended!)`, + this.fixStatement(this.nodesError[i]) ) } } diff --git a/test/fixtures/best-practises/fallback-not-payable.js b/test/fixtures/best-practises/--fallback-not-payable.js similarity index 100% rename from test/fixtures/best-practises/fallback-not-payable.js rename to test/fixtures/best-practises/--fallback-not-payable.js diff --git a/test/fixtures/best-practises/fallback-payable.js b/test/fixtures/best-practises/--fallback-payable.js similarity index 100% rename from test/fixtures/best-practises/fallback-payable.js rename to test/fixtures/best-practises/--fallback-payable.js diff --git a/test/rules/best-practises/payable-fallback.js b/test/rules/best-practises/payable-fallback.js index 281b7768..9dae2912 100644 --- a/test/rules/best-practises/payable-fallback.js +++ b/test/rules/best-practises/payable-fallback.js @@ -3,8 +3,8 @@ const linter = require('../../../lib/index') const { contractWith } = require('../../common/contract-builder') describe('Linter - payable-fallback', () => { - it('should raise warn when fallback is not payable', () => { - const code = require('../../fixtures/best-practises/fallback-not-payable') + it('should raise warn when fallback is not payable and no receive function is present', () => { + const code = contractWith('function() public {}') const report = linter.processStr(code, { rules: { 'payable-fallback': 'warn' }, @@ -14,8 +14,8 @@ describe('Linter - payable-fallback', () => { assertErrorMessage(report, 'payable') }) - it('should not raise warn when fallback is payable', () => { - const code = require('../../fixtures/best-practises/fallback-payable') + it('should not raise warn when fallback is payable and no receive function is present', () => { + const code = contractWith('fallback() public payable {}') const report = linter.processStr(code, { rules: { 'payable-fallback': 'warn' }, @@ -24,8 +24,8 @@ describe('Linter - payable-fallback', () => { assertNoWarnings(report) }) - it('should ignore non-fallback functions', () => { - const code = contractWith('function f() {} function g() payable {}') + it('should not raise warn when fallback is not payable but receive function is present', () => { + const code = contractWith('fallback() external {} receive() {}') const report = linter.processStr(code, { rules: { 'payable-fallback': 'warn' }, @@ -34,14 +34,23 @@ describe('Linter - payable-fallback', () => { assertNoWarnings(report) }) - it('should raise for other fallback types when are not payable', () => { - const code = contractWith('fallback() external {} receive() onlyOwner {}') + it('should not raise warn when fallback is payable and receive function is present', () => { + const code = contractWith('fallback() external payable {} receive() {}') const report = linter.processStr(code, { rules: { 'payable-fallback': 'warn' }, }) - assertWarnsCount(report, 2) - assertErrorMessage(report, 'payable') + assertNoWarnings(report) + }) + + it('should ignore non-fallback functions', () => { + const code = contractWith('function f() {} function g() payable {}') + + const report = linter.processStr(code, { + rules: { 'payable-fallback': 'warn' }, + }) + + assertNoWarnings(report) }) }) diff --git a/test/rules/gas-consumption/gas-named-return-values.js b/test/rules/gas-consumption/gas-named-return-values.js index 3aa73ff0..f20e4fdd 100644 --- a/test/rules/gas-consumption/gas-named-return-values.js +++ b/test/rules/gas-consumption/gas-named-return-values.js @@ -88,7 +88,6 @@ describe('Linter - gas-named-return-values', () => { 'compiler-version': 'off', 'comprehensive-interface': 'off', 'foundry-test-functions': 'off', - 'non-state-vars-leading-underscore': 'off', }, }) diff --git a/test/rules/naming/func-named-parameters.js b/test/rules/naming/func-named-parameters.js index 7aeef066..c1102d2d 100644 --- a/test/rules/naming/func-named-parameters.js +++ b/test/rules/naming/func-named-parameters.js @@ -83,7 +83,6 @@ describe('Linter - func-named-parameters', () => { 'compiler-version': 'off', 'comprehensive-interface': 'off', 'foundry-test-functions': 'off', - 'non-state-vars-leading-underscore': 'off', }, })