fix: transfers with .call excluded from warning as low level code

protofire · Jan 30, 2023 · 254b120 · 254b120
1 parent a2b8ea3
commit 254b120
Show file tree

Hide file tree

Showing 4 changed files with 73 additions and 9 deletions.
diff --git a/docs/rules/security/avoid-low-level-calls.md b/docs/rules/security/avoid-low-level-calls.md
@@ -28,12 +28,21 @@ 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}("");
+anyAddress.call.value(code)();
+```
+
 ### 👎 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);
 ```

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,30 @@ 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 (hasMethodCalls(node.expression, ['call', 'callcode', 'delegatecall'])) {
+          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,11 @@
-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);',
+]
+const ALLOWED_LOW_LEVEL_CODES = [
+  'anyAddress.call{value: 1 ether}("");',
+  'anyAddress.call.value(code)();',
+]
+
+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 low level calls', () => {
+      const report = linter.processStr(curCode, {
+        rules: { 'avoid-low-level-calls': 'warn' },
+      })
+
+      assertWarnsCount(report, 0)
+    })
+  )
 })