Consensys / truffle-security

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

Add better (non-eslint?) format with more useful information #35

Open daniyarchambylov opened 5 years ago

daniyarchambylov commented 5 years ago

When truffle-analyze gerates issue reports for smart contract it displays only path to smart contract file but not which smart contract is poorly written. We need to display contract name somehow. Suggestion: Before:

/home/daniyar/tmp/truffle-next-20/test/contracts/03-auction.sol
  1:488  error  Arbitrary senders other than the contract creator can withdraw ETH from the contract account without previously having sent an equivalent amount of ETH to it  SWC-105
  1:488  error  Possible transaction order dependence vulnerability: The value or direction of the call statement is determined from a tainted storage location                SWC-114

✖ 2 problems (2 errors, 0 warnings)

/home/daniyar/tmp/truffle-next-20/test/contracts/03-auction.sol
  1:990   error    This binary add operation can result in integer overflow                                                                                         SWC-101
  1:1170  warning  The return value of an external call is not checked                                                                                              SWC-104
  1:1360  error    Possible transaction order dependence vulnerability: The value or direction of the call statement is determined from a tainted storage location  SWC-114

✖ 3 problems (2 errors, 1 warning)

After

/home/daniyar/tmp/truffle-next-20/test/contracts/03-auction.sol:DosAuction
  1:488  error  Arbitrary senders other than the contract creator can withdraw ETH from the contract account without previously having sent an equivalent amount of ETH to it  SWC-105
  1:488  error  Possible transaction order dependence vulnerability: The value or direction of the call statement is determined from a tainted storage location                SWC-114

✖ 2 problems (2 errors, 0 warnings)

/home/daniyar/tmp/truffle-next-20/test/contracts/03-auction.sol:SecureAuction
  1:990   error    This binary add operation can result in integer overflow                                                                                         SWC-101
  1:1170  warning  The return value of an external call is not checked                                                                                              SWC-104
  1:1360  error    Possible transaction order dependence vulnerability: The value or direction of the call statement is determined from a tainted storage location  SWC-114

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

The challenge here is that we are working inside a format that has already been well established and pre-defined. Unless what you want to add naturally fits into that, we don't have the freedom to change that. See for example the discussion in: https://github.com/eslint/eslint/issues/11176

What we can do is write another report format that adds additional concepts if that is not there.

The notion of adding a contract name in addition to a file name feels like something that should have come up before in other contexts. For example a contract name is like a class, module, or function name in a conventional programming language. So check the various eslint styles to see how those are handled.

If the "contract/class/module/function" subclassification inside the file does not appear in eslint styles, make scan of how this kind of things comes up in other contexts. For example what format does gdb use to indicate a class or module name when it gives a location?

Or look at other eslint formatters to see if they handle something analougous. I had been looking at for example: https://www.npmjs.com/package/eslint-friendly-formatter for example.

rocky commented 5 years ago

Here are items that would be good to have in our custom report format:

Note that the markdown format in vscode-solidity may do some of this. So adding a markdown report format may be useful.

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


This issue now has a funding of 0.25 ETH (33.36 USD @ $133.43/ETH) attached to it as part of the MythX fund.

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Cancelled


The funding of 0.25 ETH (34.48 USD @ $137.94/ETH) attached to this issue has been cancelled by the bounty submitter

muellerberndt commented 5 years ago

Cancelled the bounty on this as specifications aren't clear.