Merge pull request #115 from habdelra/114-mark-callable-contracts-fal…

…se-positive Fixes #114 not firing false positive on mark-callable-contracts for structs
protofire · May 14, 2019 · 0d322c7 · 0d322c7
2 parents ca9d6e5 + ccdda9b
commit 0d322c7
Show file tree

Hide file tree

Showing 2 changed files with 64 additions and 3 deletions.
diff --git a/lib/rules/security/mark-callable-contracts.js b/lib/rules/security/mark-callable-contracts.js
@@ -25,15 +25,25 @@ class MarkCallableContractsChecker {
     this.reporter = reporter
     this.ruleId = ruleId
     this.meta = meta
+    this.structNames = []
+  }
+
+  enterStructDefinition(ctx) {
+    this.gatherStructNames(ctx)
   }
 
   exitIdentifier(ctx) {
     const identifier = ctx.getText()
     const isFirstCharUpper = /[A-Z]/.test(identifier[0])
-    const isContainsTrustInfo = identifier.toLowerCase().includes('trust')
+    const containsTrustInfo = identifier.toLowerCase().includes('trust')
     const isStatement = traversing.findParentType(ctx, 'StatementContext')
 
-    if (isFirstCharUpper && !isContainsTrustInfo && isStatement) {
+    if (
+      isFirstCharUpper &&
+      !containsTrustInfo &&
+      isStatement &&
+      !this.structNames.includes(identifier)
+    ) {
       this.reporter.addMessage(
         ctx.getSourceInterval(),
         SEVERITY.WARN,
@@ -42,6 +52,15 @@ class MarkCallableContractsChecker {
       )
     }
   }
+
+  gatherStructNames(ctx) {
+    const identifier = ctx.children[1]
+    const structName = identifier.getText()
+
+    if (structName) {
+      this.structNames.push(structName)
+    }
+  }
 }
 
 module.exports = MarkCallableContractsChecker
diff --git a/test/rules/security/mark-callable-contracts.js b/test/rules/security/mark-callable-contracts.js
@@ -1,6 +1,6 @@
 const assert = require('assert')
 const linter = require('./../../../lib/index')
-const funcWith = require('./../../common/contract-builder').funcWith
+const { funcWith, contractWith } = require('./../../common/contract-builder')
 
 describe('Linter - mark-callable-contracts', () => {
   it('should return error that external contract is not marked as trusted / untrusted', () => {
@@ -13,4 +13,46 @@ describe('Linter - mark-callable-contracts', () => {
     assert.equal(report.warningCount, 1)
     assert.ok(report.reports[0].message.includes('trusted'))
   })
+
+  it('should not return error for external contract that is marked as trusted', () => {
+    const code = funcWith('TrustedBank.withdraw(100);')
+
+    const report = linter.processStr(code, {
+      rules: { 'mark-callable-contracts': 'warn' }
+    })
+
+    assert.equal(report.warningCount, 0)
+  })
+
+  it('should not return error for external contract that is marked as untrusted', () => {
+    const code = funcWith('UntrustedBank.withdraw(100);')
+
+    const report = linter.processStr(code, {
+      rules: { 'mark-callable-contracts': 'warn' }
+    })
+
+    assert.equal(report.warningCount, 0)
+  })
+
+  it('should not return error for a struct', () => {
+    const code = contractWith(`
+  struct Token {
+    address tokenAddress;
+    string tokenSymbol;
+  }
+
+  mapping(address => Token) public acceptedTokens;
+
+  function b(address tokenAddress, string memory tokenSymbol) public {
+    Token memory token = Token(tokenAddress, tokenSymbol);
+    acceptedTokens[tokenAddress] = token;
+  }
+    `)
+
+    const report = linter.processStr(code, {
+      rules: { 'mark-callable-contracts': 'warn' }
+    })
+
+    assert.equal(report.warningCount, 0)
+  })
 })