Consensys / mythril

Security analysis tool for EVM bytecode. Supports smart contracts built for Ethereum, Hedera, Quorum, Vechain, Rootstock, Tron and other EVM-compatible blockchains.
https://mythx.io/
MIT License
3.88k stars 741 forks source link

False positives for some of the issues Mythril detects? #454

Closed shashank-srikant closed 6 years ago

shashank-srikant commented 6 years ago

I've been trying to understand codes for the different issues which Mythril flags. I'm having a hard time tracking these issues in the actual source. Not sure if these are false-positives. Will be great if your team can help spot these issues in the code.

Description

Unchecked CALL return value

Mythril detects this issue for the following source codes - https://etherscan.io/address/0xf7a6e15dfd5cdd9ef12711bd757a9b6021abf643#code https://etherscan.io/address/0xc3424fe8fb83002b35f96c9618a15d06ddec0c68#code

(Aside - In the particular commit I use, i get an error when trying to run Mythril on 0xc34.. above. I strongly suspect this is a commit-related issue. Because i've managed to run this code right on the previous Mythril version i had. This is the error i see.

Traceback (most recent call last):
  File "/usr/local/bin/myth", line 11, in <module>
    load_entry_point('mythril==0.18.11', 'console_scripts', 'myth')()
  File "/usr/local/lib/python3.5/dist-packages/mythril/interfaces/cli.py", line 209, in main
    max_depth=args.max_depth, execution_timeout=args.execution_timeout)
  File "/usr/local/lib/python3.5/dist-packages/mythril/mythril.py", line 385, in fire_lasers
    issue.add_code_info(contract)
  File "/usr/local/lib/python3.5/dist-packages/mythril/analysis/report.py", line 38, in add_code_info
    codeinfo = contract.get_source_info(self.address)
  File "/usr/local/lib/python3.5/dist-packages/mythril/ether/soliditycontract.py", line 105, in get_source_info
    solidity_file = self.solidity_files[self.mappings[index].solidity_file_idx]
TypeError: list indices must be integers or slices, not NoneType

).

I don't however find any call values being made explicitly. What's the reason behind this issue being flagged then?

Unchecked SUICIDE calls

Likewise, Mythril flags these codes as having unchecked SUICIDE calls. https://etherscan.io/address/0x0033fb5561719b8b697b604466d6d39308c58191#code https://etherscan.io/address/0x3e84512f277A5081B9209831C51bCe665035D9DB#code

But i don't really see any such calls explicitly being made.

I must admit i have not really poured over these codes very carefully, and i'm not sure if lines of code get eventually translated to these calls. But is that what's Mythril doing in these case -- that's something i like to understand.

How to Reproduce

myth -xo json filename

Environment

Mythril version: https://github.com/ConsenSys/mythril/commit/657458bda31a868c54c8966a98b4b6a1ba841abd Solidity compiler and version: Version: 0.4.24+commit.e67f0147.Linux.g++ Python version: Python 3.5.2 OS: Ubuntu 16.04 LTS

norhh commented 6 years ago

in f7ae6e... i just checked by mapping the addresses to the PC(i think mythril should return it for simplicity)

PC 3304 = 649 PC 3643, 3969 = 653 PC 1909, 2134, 2394 = 795

They more or less look like false positive to me but it is quite complex so i am not totally sure. So i tried it on solidity_examples/calls.sol and it returned most of the calls present in the program.So it might be a false positive

shashank-srikant commented 6 years ago

i just checked by mapping the addresses to the PC(i think mythril should return it for simplicity)

So i tried it on solidity_examples/calls.sol and it returned most of the calls present in the program.So it might be a false positive

i don't think i quite follow your reasoning. could you explain what it is you're trying to ascertain through these steps? that'll help me develop an intuition on how to debug these issues myself.

norhh commented 6 years ago

It doesn't look like all those calls are made in 1 single transaction.

shashank-srikant commented 6 years ago

460 seems related to this.

norhh commented 6 years ago

I think a part of this is fixed by #460 and regarding the unchecked call retval, there wasn't any check after a call was being made in the code, so i think it should be fine.