Repository: protofire/solhint
Pull request #394 (2023-01-30 14:54:45)

fix: transfers with .call excluded from warning as low level code
This PR comes from 
https://github.com/protofire/solhint/pull/329
All credits to @potomak


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 85a318f..beb7849 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,17 +1,3 @@
-## [3.3.9] - TBD
-
-### Updated
-- Update solhint dependencies to support newer versions [#380](https://github.com/protofire/solhint/pull/380)
-- Linter fixed to get clearer source code [#381](https://github.com/protofire/solhint/pull/381)
-- Updated E2E, added formatters into repo, updated CI [#385](https://github.com/protofire/solhint/pull/385)
-
-### Added
-- Lint rule for banning "console.sol" and "import hardhat or foundry console.sol" [#372](https://github.com/protofire/solhint/pull/372)
-
-### Fixed
-- Directories with .sol in the name path treated as files [#352](https://github.com/protofire/solhint/pull/388)
-
-<br><br>
 ## [3.3.8] - 2023-01-17
 ### Fixed Docs and Typos
 - [#292](https://github.com/protofire/solhint/pull/292)
@@ -19,6 +5,8 @@
 - [#355](https://github.com/protofire/solhint/pull/355)
 - [#285](https://github.com/protofire/solhint/pull/285)
   
+### Added
+- Lint rule for banning "console.sol" and "import hardhat/console.sol" [#372](https://github.com/protofire/solhint/pull/372)
 
 ### Updated
 - Solidity Parser to 0.14.5 [#378](https://github.com/protofire/solhint/pull/378)
diff --git a/docs/rules/security/avoid-low-level-calls.md b/docs/rules/security/avoid-low-level-calls.md
index b543327..eea9d15 100644
--- a/docs/rules/security/avoid-low-level-calls.md
+++ b/docs/rules/security/avoid-low-level-calls.md
@@ -28,14 +28,23 @@ This rule accepts a string option of rule severity. Must be one of "error", "war
 
 
 ## Examples
+### 👍 Examples of **correct** code for this rule
+
+#### Using low level calls to transfer funds
+
+```solidity
+anyAddress.call{value: 1 ether}("");
+```
+
 ### 👎 Examples of **incorrect** code for this rule
 
 #### Using low level calls
 
 ```solidity
-msg.sender.call(code);
+anyAddress.call(code);
 a.callcode(test1);
 a.delegatecall(test1);
+anyAddress.call.value(code)();
 ```
 
 ## Version
diff --git a/lib/common/tree-traversing.js b/lib/common/tree-traversing.js
index 77b74c0..fc0155b 100644
--- a/lib/common/tree-traversing.js
+++ b/lib/common/tree-traversing.js
@@ -77,7 +77,6 @@ TreeTraversing.typeOf = function typeOf(ctx) {
 
 TreeTraversing.hasMethodCalls = function hasMethodCalls(node, methodNames) {
   const text = node.memberName
-
   return methodNames.includes(text)
 }
 
diff --git a/lib/index.js b/lib/index.js
index 1c10e66..6c522eb 100644
--- a/lib/index.js
+++ b/lib/index.js
@@ -61,7 +61,7 @@ function processFile(file, config) {
 function processPath(path, config) {
   const ignoreFilter = ignore({ allowRelativePaths: true }).add(config.excludedFiles)
 
-  const allFiles = glob.sync(path, { nodir: true })
+  const allFiles = glob.sync(path, {})
   const files = ignoreFilter.filter(allFiles)
 
   return files.map((curFile) => processFile(curFile, config))
diff --git a/lib/rules/security/avoid-low-level-calls.js b/lib/rules/security/avoid-low-level-calls.js
index 21bc1b0..8056de2 100644
--- a/lib/rules/security/avoid-low-level-calls.js
+++ b/lib/rules/security/avoid-low-level-calls.js
@@ -1,5 +1,11 @@
+/* eslint-disable no-fallthrough */
+/* eslint-disable default-case */
 const BaseChecker = require('../base-checker')
 const { hasMethodCalls } = require('../../common/tree-traversing')
+const LOW_LEVEL_CALLS = require('../../../test/fixtures/security/low-level-calls')
+
+const WARN_LOW_LEVEL_CALLS = LOW_LEVEL_CALLS[0]
+const ALLOWED_LOW_LEVEL_CALLS = LOW_LEVEL_CALLS[1]
 
 const ruleId = 'avoid-low-level-calls'
 const meta = {
@@ -9,10 +15,16 @@ const meta = {
     description: `Avoid to use low level calls.`,
     category: 'Security Rules',
     examples: {
+      good: [
+        {
+          description: 'Using low level calls to transfer funds',
+          code: ALLOWED_LOW_LEVEL_CALLS.join('\n'),
+        },
+      ],
       bad: [
         {
           description: 'Using low level calls',
-          code: require('../../../test/fixtures/security/low-level-calls').join('\n'),
+          code: WARN_LOW_LEVEL_CALLS.join('\n'),
         },
       ],
     },
@@ -30,9 +42,35 @@ class AvoidLowLevelCallsChecker extends BaseChecker {
     super(reporter, ruleId, meta)
   }
 
-  MemberAccess(node) {
-    if (hasMethodCalls(node, ['call', 'callcode', 'delegatecall'])) {
-      this._warn(node)
+  FunctionCall(node) {
+    switch (node.expression.type) {
+      case 'MemberAccess':
+        if (
+          // can add 'value' in the array of hasMethodCalls() and remove the || condition but seems out of place
+          hasMethodCalls(node.expression, ['call', 'callcode', 'delegatecall']) ||
+          (node.expression.expression.memberName === 'call' && // this is for anyAddress.call.value(code)()
+            node.expression.memberName === 'value')
+        ) {
+          return this._warn(node)
+        }
+      case 'NameValueExpression':
+        // Allow low level method calls for transferring funds
+        //
+        // See:
+        //
+        // - https://solidity-by-example.org/sending-ether/
+        // - https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/
+        if (
+          node.expression.expression.memberName === 'call' &&
+          node.expression.arguments.type === 'NameValueList' &&
+          node.expression.arguments.names.includes('value')
+        ) {
+          return
+        }
+
+        if (hasMethodCalls(node.expression.expression, ['call', 'callcode', 'delegatecall'])) {
+          return this._warn(node)
+        }
     }
   }
 
diff --git a/solhint.js b/solhint.js
index 8e2d619..383eed6 100644
--- a/solhint.js
+++ b/solhint.js
@@ -87,9 +87,7 @@ function execMainAction() {
 
   if (program.opts().quiet) {
     // filter the list of reports, to set errors only.
-    reports.forEach((reporter) => {
-      reporter.reports = reporter.reports.filter((i) => i.severity === 2)
-    })
+    reports[0].reports = reports[0].reports.filter((i) => i.severity === 2)
   }
 
   if (printReports(reports, formatterFn)) {


Test patch:
diff --git a/e2e/04-dotSol-on-path/.solhint.json b/e2e/04-dotSol-on-path/.solhint.json
deleted file mode 100644
index 1ee2c69..0000000
--- a/e2e/04-dotSol-on-path/.solhint.json
+++ /dev/null
@@ -1,5 +0,0 @@
-{
-  "rules": {
-    "no-empty-blocks": "off"
-  }
-}
diff --git a/e2e/04-dotSol-on-path/contracts/ERC20.sol/ERC20.dbg.js b/e2e/04-dotSol-on-path/contracts/ERC20.sol/ERC20.dbg.js
deleted file mode 100644
index e69de29..0000000
diff --git a/e2e/04-dotSol-on-path/contracts/ERC20.sol/Foo.sol b/e2e/04-dotSol-on-path/contracts/ERC20.sol/Foo.sol
deleted file mode 100644
index 00cbe4c..0000000
--- a/e2e/04-dotSol-on-path/contracts/ERC20.sol/Foo.sol
+++ /dev/null
@@ -1,4 +0,0 @@
-pragma solidity >=0.5.0;
-
-contract Foo {
-}
diff --git a/e2e/test.js b/e2e/test.js
index b4b9399..c614a8e 100644
--- a/e2e/test.js
+++ b/e2e/test.js
@@ -111,14 +111,4 @@ describe('e2e', function() {
       expect(stdout.trim()).to.contain('Code contains empty blocks')
     })
   })
-
-  describe('.sol on path', function() {
-    useFixture('04-dotSol-on-path')
-
-    it('should handle directory names that end with .sol', function() {
-      const { code } = shell.exec('solhint contracts/**/*.sol')
-
-      expect(code).to.equal(0)
-    })
-  })
 })
diff --git a/test/fixtures/security/low-level-calls.js b/test/fixtures/security/low-level-calls.js
index 194d468..0c9ddaa 100644
--- a/test/fixtures/security/low-level-calls.js
+++ b/test/fixtures/security/low-level-calls.js
@@ -1,1 +1,9 @@
-module.exports = ['msg.sender.call(code);', 'a.callcode(test1);', 'a.delegatecall(test1);']
+const WARN_LOW_LEVEL_CODES = [
+  'anyAddress.call(code);',
+  'a.callcode(test1);',
+  'a.delegatecall(test1);',
+  'anyAddress.call.value(code)();',
+]
+const ALLOWED_LOW_LEVEL_CODES = ['anyAddress.call{value: 1 ether}("");']
+
+module.exports = [WARN_LOW_LEVEL_CODES, ALLOWED_LOW_LEVEL_CODES]
diff --git a/test/rules/security/avoid-low-level-calls.js b/test/rules/security/avoid-low-level-calls.js
index 386ab09..7e4be38 100644
--- a/test/rules/security/avoid-low-level-calls.js
+++ b/test/rules/security/avoid-low-level-calls.js
@@ -3,10 +3,12 @@ const funcWith = require('../../common/contract-builder').funcWith
 const { assertWarnsCount, assertErrorMessage } = require('../../common/asserts')
 
 describe('Linter - avoid-low-level-calls', () => {
-  const LOW_LEVEL_CALLS = require('../../fixtures/security/low-level-calls').map(funcWith)
+  const LOW_LEVEL_CALLS = require('../../fixtures/security/low-level-calls')
+  const WARN_LOW_LEVEL_CALLS = LOW_LEVEL_CALLS[0].map(funcWith)
+  const ALLOWED_LOW_LEVEL_CALLS = LOW_LEVEL_CALLS[1].map(funcWith)
 
-  LOW_LEVEL_CALLS.forEach((curCode) =>
-    it('should return warn when code contains possible reentrancy', () => {
+  WARN_LOW_LEVEL_CALLS.forEach((curCode) =>
+    it('should return warn when code contains low level calls', () => {
       const report = linter.processStr(curCode, {
         rules: { 'avoid-low-level-calls': 'warn' },
       })
@@ -15,4 +17,14 @@ describe('Linter - avoid-low-level-calls', () => {
       assertErrorMessage(report, 'low level')
     })
   )
+
+  ALLOWED_LOW_LEVEL_CALLS.forEach((curCode) =>
+    it('should not return warn when code contains allowed low level calls', () => {
+      const report = linter.processStr(curCode, {
+        rules: { 'avoid-low-level-calls': 'warn' },
+      })
+
+      assertWarnsCount(report, 0)
+    })
+  )
 })