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.84k stars 736 forks source link

Refactor analysis modules to be as compact as possible #745

Closed muellerberndt closed 5 years ago

muellerberndt commented 5 years ago

Description

Refactor analysis modules to be as readable and compact and as possible.

Those are just some suggestions, I'm sure you guys have better ideas ;)

Some ideas:

  1. Move reusable stuff into the base module:
  1. Include metadata and descriptive texts at the top of the module file. Use those constants in the code instead of repeating strings inline. E.g.:
TITLE = "Ether Thief"

SEVERITY = "Warning"

DESCRIPTION = """

Search for cases where Ether can be withdrawn to a user-specified address. 

An issue is reported if:

- The transaction sender does not match contract creator;
- The sender address can be chosen arbitrarily;
- The receiver address is identical to the sender address;
- The sender can withdraw *more* than the total amount they sent over all transactions.
"""

REPORT_TEXT = \
    "Arbitrary senders other than the contract creator can withdraw ETH from " \
    + "the contract without previously having sent an equivalent amount of ETH " \
    + "to it. This is likely to be a vulnerability."

class EtherThief(DetectionModule):
(...)
JoranHonig commented 5 years ago

Issue constructor -> create a based method that creates a new issue without having to provide all constructor parameters explicitly.

So basically we'd create some IssueFactory at the initialization of the module, instead of creating a new issue directly the factory builds it with a lot of fields already set ?

muellerberndt commented 5 years ago

So basically we'd create some IssueFactory at the initialization of the module, instead of creating a new issue directly the factory builds it with a lot of fields already set ?

Yep, the module author would only have to provide a text description and the rest could be gathered from the environment & metadata.

JoranHonig commented 5 years ago

I think this has been dealt with in the callback refactor in part, any other cleanups will probably happen incrementally. @b-mueller WDYT

muellerberndt commented 5 years ago

Yeah, I'm fine with closing all cosmetic issues right now since we should focus on things that are relevant to MythX.