Merge pull request #394 from protofire/fix/avoid-low-level-calls

fix: transfers with .call excluded from warning as low level code
protofire · Feb 8, 2023 · 2970288 · 2970288
2 parents 021c960 + bc437e3
commit 2970288
Show file tree

Hide file tree

Showing 5 changed files with 76 additions and 10 deletions.
diff --git 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
@@ -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/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/test/fixtures/security/low-level-calls.js b/test/fixtures/security/low-level-calls.js
@@ -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
@@ -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)
+    })
+  )
 })