Repository: protofire/solhint (js)

Issue:
mark-callable-contracts rule is firing a false positive on structs
The `mark-callable-contracts` rule is being triggered against `structs`, as they are instantiated very similarly to contracts. We should update this rule so that it does not get triggered by structs. Take for instance this example:

```sol
contract ReservePool {

  struct Token {
    address tokenAddress;
    string tokenSymbol;
    uint256 currentRate;
  }

  mapping(address => Token) public acceptedTokens;
.
.
.
  function addAcceptedToken(string memory tokenSymbol, address tokenAddress, uint256 rate) public onlyAdmins {
    // solhint-disable-next-line mark-callable-contracts, solhint is confusing structs for contracts
    Token memory token = Token({ tokenAddress: tokenAddress, tokenSymbol: tokenSymbol, currentRate: rate });
    acceptedTokens[tokenAddress] = token;
  }
.
.
.
}
```

Note that `solhint-disable-next-line` had to be used for this to pass solhint as the `struct` was being confused for a contract.

Pull request description:
Fixes #114 not firing false positive on mark-callable-contracts for structs
Fixes #114 

Patch:
diff --git a/lib/rules/security/mark-callable-contracts.js b/lib/rules/security/mark-callable-contracts.js
index c074e40..b9ec42f 100644
--- 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


Test patch:
diff --git a/test/rules/security/mark-callable-contracts.js b/test/rules/security/mark-callable-contracts.js
index 6860381..0a5c6f9 100644
--- 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)
+  })
 })