WIP MythX integration (Ethereal Hackathon)

[aquiladev]

Purpose

Resolve Consensys/mythx-gitcoin#3

Does this introduce a breaking change?

[ ] Yes
[x] No

Pull Request Type

What kind of change does this Pull Request introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

Intro

The pull request introduces a new tab 'Security'. The tab contains MythX integration.

User can provide own credentials for MythX and analyze compiled contracts

Analyze report:

In case of error:

[muellerberndt]

Hey @aquiladev, I'm getting the following error in the browser console when running remix-IDE locally:

TypeError: url.URL is not a constructor

This is caused by code in armlet's util.js:

exports.joinUrl = (base, path) => {
  const u = new url.URL(path, base)
  return u.href
}

Seems like there is a browser compatibility issue with armlet? Any ideas on how to best fix this?

[muellerberndt]

Ok, I just went to the armlet repo that you already created an issue...

Edit: I got it working for now by patching up armlet.

What would still be nice to have would be highlighting vulnerable code locations using the source mappings in the response  😉

[EtherDotBlue]

Very nice!

[aquiladev]

What would still be nice to have would be highlighting vulnerable code locations using the source mappings in the response  😉

it would be a good improvement for the next version

[aquiladev]

What would still be nice to have would be highlighting vulnerable code locations using the source mappings in the response 😉

@b-mueller
btw, how to achieve it?
There is sourceMap location of the issue in the result of analyses

[muellerberndt]

@aquiladev if you submit source code & byte code you should get back standard solc sourceMaps from the API. MythX for Truffle has helper functions to convert those to lines and columns.

There is an older Remix plugin that has code highlighting. It uses issuesObject.convertMythXReport2EsIssue from the truffle-security package to process the source maps. The plugin itself isn't maintained so I don't think it's functional anymore but the basic process should be in there.

[aquiladev]

The approach requires ASTs of contract

I can't find ASTs in sources or artifacts after compilation in remix-ide. There is the only legacyAST in sources

[muellerberndt]

The approach requires ASTs of contract

Hmm which part requires the AST? You don't need to submit the AST to MythX to get source mappings back, you can also submit the source itself instead like Sabre does:

$ sabre --debug vulnerable.sol
✔ Compiled with solc v0.5.7 successfully
-------------------
MythX Request Body:

{ contractName: 'Vulnerable',
  bytecode:
   '608060405260f860005534801561001557600080fd5b5060a7806100246000396000f3fe6080604052348015600f57600080fd5b5060043610603c5760003560e01c80630dbe671f14604157806326121ff01460495780632e52d60614604f575b600080fd5b60476067565b005b60476072565b60556075565b60408051918252519081900360200190f35b600080546002029055565b33ff5b6000548156fea165627a7a72305820ac56c479b3577e8999bfa9688c45e4abd6f8bc925c08a3f0b6be3399cb4026480029',
  sourceMap:
   '25:172:0:-;;;;8:9:-1;5:2;;;30:1;27;20:12;5:2;25:172:0;;;;;;;;;;;;;;;;;;;;;;;;;;;;;149:46;;;:::i;:::-;;82:61;;;:::i;51:24::-;;;:::i;:::-;;;;;;;;;;;;;;;;149:46;183:1;;;187;183:5;179:9;;149:46::o;82:61::-;125:10;112:24;51;;;;:::o',
  deployedBytecode:
   '6080604052348015600f57600080fd5b5060043610603c5760003560e01c80630dbe671f14604157806326121ff01460495780632e52d60614604f575b600080fd5b60476067565b005b60476072565b60556075565b60408051918252519081900360200190f35b600080546002029055565b33ff5b6000548156fea165627a7a72305820ac56c479b3577e8999bfa9688c45e4abd6f8bc925c08a3f0b6be3399cb4026480029',
  deployedSourceMap:
   '25:172:0:-;;;;8:9:-1;5:2;;;30:1;27;20:12;5:2;25:172:0;;;;;;;;;;;;;;;;;;;;;;;;;;;;;149:46;;;:::i;:::-;;82:61;;;:::i;51:24::-;;;:::i;:::-;;;;;;;;;;;;;;;;149:46;183:1;;;187;183:5;179:9;;149:46::o;82:61::-;125:10;112:24;51;;;;:::o',
  sourceList: [ 'vulnerable.sol' ],
  analysisMode: 'quick',
  sources:
   { 'vulnerable.sol':
      { source:
         'pragma solidity ^0.5.7;\n\ncontract Vulnerable {\n    uint256 public n = 2^250;\n\n    function f() public {\n        selfdestruct(msg.sender);\n    }\n\n    function a() public {\n        n = n * 2;\n    }\n}\n' } },
  mainSource: 'vulnerable.sol' }

-------------------

MythX Response Body:

{ elapsed: 56763,
  issues:
   [ { issues:
        [ { swcID: 'SWC-106',
            swcTitle: 'Unprotected SELFDESTRUCT Instruction',
            description:
             { head: 'The contract can be killed by anyone.',
               tail:
                'Anyone can kill this contract and withdraw its balance to an arbitrary address.' },
            severity: 'High',
            locations: [ { sourceMap: '112:24:0' } ],
            extra:
             { testCase: { initialState: { accounts: null }, steps: null } } } ],
       sourceType: 'solidity-file',
       sourceFormat: 'text',
       sourceList: [ 'vulnerable.sol' ],
       meta:
        { coveredInstructions: 88,
          coveredPaths: 6,
          logs:
           [],
  status:
   { apiVersion: 'v1.4.14',
     harveyVersion: '0.0.18',
     maestroVersion: '1.2.10',
     maruVersion: '0.4.4',
     mythrilVersion: '0.20.4',
     queueTime: 6806,
     runTime: 44019,
     status: 'Finished',
     submittedAt: '2019-04-30T12:32:26.316Z',
     submittedBy: '5c1b124609525e001275d918',
     uuid: '5f037750-4562-4952-a22d-c0c48d90d645' } }
-------------------

vulnerable.sol
  7:8  error  The contract can be killed by anyone  https://smartcontractsecurity.github.io/SWC-registry/docs/SWC-106

✖ 1 problem (1 error, 0 warnings)

Every issue should have a sourceMap entry in its locations. Sabre uses remix sourceMappingDecoder to decode that.

[aquiladev]

MythX doesn't need it, but when I try to create MythXIssues object in order to map issues with source code I have an error. MythXIssues needs AST. I'm not sure that it really needs it.

I'll check sabre solution

[yann300]

@aquiladev @b-mueller thanks for this PR ;)
would that be ok for merging this code? as we are moving to using more the plugin API, I think we should leverage this right now, I'll PM you, that should not be a lot of work.
cc @GrandSchtroumpf

[aquiladev]

@yann300 I think I need to finish code highlighting and improve the report rendering. I change the PR to WIP. If you have any suggestions or thoughts about the necessity or implementation of the functionality, pls guide me.

[yann300]

sure @aquiladev how could i PM you?

[yann300]

nm, found you

[yann300]

i sent you a message in gitter

[muellerberndt]

Hi guys, this PR still needs some improvements & testing:

1. Code highlighting or at least line + column numbers in the report output. Currently no location information is provided.
2. I tested this with some simple input contracts, but not yet with more complex projects that have many imports. Also I encountered a few UI bugs, such as contract selection dropdown not working in Safari (need to double-check on that though).

[muellerberndt]

This is superseded by #1999 right? Can we close it?