search

Consensys / truffle-security

MythX smart contract security verification plugin for Truffle Framework
https://mythx.io
124 stars 28 forks source link

Different results reported when running analysis on all contracts as opposed to individually #222

Closed elenadimitrova closed 5 years ago

elenadimitrova commented 5 years ago

Different results reported when running analysis on all contracts as opposed to individually. Below is an extract of the truffle run verify --mode quick --style json run against all contracts, showing the errors reported on the IColonyNetwork contract.

{"errorCount":0,"warningCount":4,"fixableErrorCount":0,"fixableWarningCount":0,"filePath":"/Users/Elena/colonyNetwork/contracts/IColonyNetwork.sol",
    "messages":[
      {"fatal":false,"ruleId":"SWC-103","message":"A floating pragma is set. It is recommended to make a conscious choice on what version of Solidity is used for compilation. Currently multiple versions \">=0.5.6\" are allowed.","severity":1,"mythXseverity":"Low","line":18,"column":0,"endLine":18,"endCol":24},
      {"fatal":false,"ruleId":"SWC-107","message":"The contract executes an external message call. An external function call to a fixed contract address is executed. Make sure that the callee contract has been reviewed carefully.","severity":1,"mythXseverity":"Low","line":118,"column":37,"endLine":119,"endCol":17},
      {"fatal":false,"ruleId":"SWC-107","message":"The contract executes an external message call. An external function call to a fixed contract address is executed. Make sure that the callee contract has been reviewed carefully.","severity":1,"mythXseverity":"Low","line":134,"column":35,"endLine":134,"endCol":60},
      {"fatal":false,"ruleId":"SWC-107","message":"The contract executes an external message call. An external function call to a fixed contract address is executed. Make sure that the callee contract has been reviewed carefully.","severity":1,"mythXseverity":"Low","line":140,"column":44,"endLine":140,"endCol":84}]},

When analysing the contract on its own via truffle run verify --mode quick --style json IColonyNetwork.json the three SWC-107 messages aren't present.

[
  {"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0,"filePath":"/Users/Elena/colonyNetwork/contracts/ColonyNetworkDataTypes.sol","messages":[]},
  {"errorCount":0,"warningCount":1,"fixableErrorCount":0,"fixableWarningCount":0,"filePath":"/Users/Elena/colonyNetwork/contracts/IColonyNetwork.sol",
    "messages":[
      {"fatal":false,"ruleId":"SWC-103","message":"A floating pragma is set. It is recommended to make a conscious choice on what version of Solidity is used for compilation. Currently multiple versions \">=0.5.6\" are allowed.","severity":1,"mythXseverity":"Low","line":18,"column":0,"endLine":18,"endCol":24}
      ]
    },
    {"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0,"filePath":"/Users/Elena/colonyNetwork/contracts/IRecovery.sol","messages":[]}
]
nbanmp commented 5 years ago

Can you check what version of truffle-security you are using?

elenadimitrova commented 5 years ago

I've tested this on both 1.5.0 and 1.5.1

elenadimitrova commented 5 years ago

I've just confirmed this is not an issue in v1.4.2

daniyarchambylov commented 5 years ago

Running truffle run verify --mode quick --style json, truffle run verify --mode quick --style json IColonyNetwork and truffle run verify --mode quick --style json contracts/IColonyNetwork.sol all return only SWC-103

 {
    "errorCount": 0,
    "warningCount": 1,
    "fixableErrorCount": 0,
    "fixableWarningCount": 0,
    "filePath": "/usr/src/node_app/contracts/IColonyNetwork.sol",
    "messages": [
      {
        "fatal": false,
        "ruleId": "SWC-103",
        "message": "A floating pragma is set. It is recommended to make a conscious choice on what version of Solidity is used for compilation. Currently multiple versions \">=0.5.6\" are allowed.",
        "severity": 1,
        "mythXseverity": "Low",
        "line": 18,
        "column": 0,
        "endLine": 18,
        "endCol": 24
      }
    ]
  },
$ truffle run verify --version
truffle-security 1.5.1
api: v1.4.21, harvey: 0.0.27, hash: b3db8db7026d1cd31c136a9108f2fdfb, maestro: 1.2.16, maru: 0.4.8, mythril: 0.20.8, url: https://api.mythx.io
elenadimitrova commented 5 years ago

I just reproduced it here https://circleci.com/gh/JoinColony/colonyNetwork/8796

The result for /home/circleci/colonyNetwork/contracts/IColonyNetwork.sol includes all 4 warnings:

║ Line     │ Column   │ Type     │ Message                                                │ Rule ID              ║
╟──────────┼──────────┼──────────┼────────────────────────────────────────────────────────┼──────────────────────╢
║ 18       │ 0        │ warning  │ A floating pragma is set.                              │ SWC-103              ║
║ 118      │ 37       │ warning  │ The contract executes an external message call.        │ SWC-107              ║
║ 134      │ 35       │ warning  │ The contract executes an external message call.        │ SWC-107              ║
║ 140      │ 44       │ warning  │ The contract executes an external message call.        │ SWC-107              ║
elenadimitrova commented 5 years ago

Essentially the problem is that when you run just a single contract analysis it returns the one SWC-103 warning, whereas when you run the entire set of contracts, it returns these additional 3 SWC-107 instances. These instances are also false positives as the interface contract executes nothing, yet all three warnings refer to an external message call syntax somewhere.

nbanmp commented 5 years ago

@elenadimitrova I've been having trouble reproducing the issue as well. I'll continue to look and compare against your ci log. If you could run truffle run verify --style json --debug=2 and upload the contents somewhere (GitHub gists or something), that would be helpful.

Also, was the error occuring in quick mode, or only in full mode?

Also, a side note: you can add the --no-progress arg to your ci runs, and you won't get all the extraneous attempts at progress bars.

elenadimitrova commented 5 years ago

Just as a side note here you can see three consecutive runs (the last 2 of which is close succession) showing significantly different results for the same analysis. https://circleci.com/gh/JoinColony/colonyNetwork/8796 https://circleci.com/gh/JoinColony/colonyNetwork/8948 https://circleci.com/gh/JoinColony/colonyNetwork/8949

elenadimitrova commented 5 years ago

I've run build with the debug options you asked for @nbanmp and you can download the output directly from CI here https://circleci.com/gh/JoinColony/colonyNetwork/8950

nbanmp commented 5 years ago

Thanks for that output. The error seems to be that the order of the sourceList sent does not match the sourceMaps in the ast sent.

The source list in your debug output is not sorted correctly, even though the ast and id of the sources are correct.

We had this same issue earlier, and fixed it by adding this line here, ensuring that the ordering of the sourceList was correct: https://github.com/ConsenSys/truffle-security/blob/v1.5.1/lib/mythx.js#L47

Now that we know exactly what the issue is we get the pleasure of figuring out why it's happening again and why our previous fix is not working...

nbanmp commented 5 years ago

That fix was introduced in v1.4.3, which is making me really confused. Because it is working for you before that version, but not working after? I would expect the exact opposite results. :cry:

Can you triple check that circleci is running the latest truffle security version?

elenadimitrova commented 5 years ago

Definitely running on v1.5.1. Outputting that explicitly now, see Getting truffle-security version number step in CI reporting:

truffle-security 1.5.1
api: v1.4.23, harvey: 0.0.27, hash: fd386b323440d99ad205a941d8a46bb5, maestro: 1.2.16, maru: 0.4.8, mythril: 0.21.2, url: https://api.mythx.io

I am going to explicitly step down through each version from 1.5.1 -> 1.5.0 -> 1.4.4 -> 1.4.3 on this branch to triple make sure it breaks on 1.5.x. You can follow the builds here https://circleci.com/gh/JoinColony/colonyNetwork/tree/repro%2Ftruffle-security-issue-222

nbanmp commented 5 years ago

Yes! Thanks for all that work! I finally managed to reproduce the issue on my machine.

══ /home/nat/Dev/mythx/test-cases/colonyNetwork
━━┫ truffle run verify contracts/ColonyNetwork.sol

ColonyNetwork |***************| 100% || Elapsed: 9.7s ✓ completed

/home/nat/Dev/mythx/test-cases/colonyNetwork/contracts/IMetaColony.sol
  53:1664  error    persistent state read after call  SWC-107
  53:1686  error    persistent state read after call  SWC-107
  53:1731  warning  multiple external calls           SWC-113

✖ 3 problems (2 errors, 1 warning)
nbanmp commented 5 years ago

@elenadimitrova The bug should be fixed now. Can you try truffle-security v1.5.2 and close this issue if it works?

elenadimitrova commented 5 years ago

🎉 https://circleci.com/gh/JoinColony/colonyNetwork/8987