Repository: protofire/solhint
Pull request #561 (2024-03-12 19:02:19)

fix: payable fallback
Modified behavior:
Search for a `receive()` function
If not present, search for `fallback()` function
If present, suggest making it `payable` to receive native currency (if it is not already)

This rule suggests to code a `receive()` function in the warnings


Patch:
diff --git a/conf/rulesets/solhint-all.js b/conf/rulesets/solhint-all.js
index 2344912..b10c1e1 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 f1ed437..61bc0c8 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 be0f61f..ca8d6b5 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 89a468b..f814824 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/lib/rules/best-practises/interface-starts-with-i.js b/lib/rules/best-practises/interface-starts-with-i.js
index 9c7fafe..f47db11 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 473c7c6..6655bac 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])
         )
       }
     }


Test patch:
diff --git a/e2e/08-autofix/payable-fallback/Foo1.sol b/e2e/08-autofix/payable-fallback/Foo1.sol
index eb1cfda..035b035 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 f32e5c5..9bdfe87 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 eb1cfda..035b035 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/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 281b776..9dae291 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 3aa73ff..f20e4fd 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 7aeef06..c1102d2 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',
       },
     })