Consensys / truffle-security

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

Line numbers don't always match source code #66

Closed imthatcarlos closed 5 years ago

imthatcarlos commented 5 years ago

running truffle run verify produces:

screen shot 2019-02-12 at 11 32 42 am

and the associated code (up to line 54):

pragma solidity 0.5.0;

import "openzeppelin-solidity/contracts/ownership/Ownable.sol";
import "openzeppelin-solidity/contracts/lifecycle/Pausable.sol";
import "openzeppelin-solidity/contracts/math/SafeMath.sol";
import "./VTToken.sol";
import "./TToken.sol";
import "./Main.sol";

/**
 * @title AssetRegistry
 * This contract manages the functionality for assets - adding records, as well as reading data. It allows
 * users to add assets to the platform, and asset owners to fund their assets once sold.
 *
 * @author Carlos Beltran <imthatcarlos@gmail.com>
 */
contract AssetRegistry is Pausable, Ownable {
  using SafeMath for uint;

  event AssetRecordCreated(address tokenAddress, address assetOwner, uint id);
  event AssetRecordUpdated(address indexed tokenAddress, uint id);
  event AssetFunded(uint id, address tokenAddress);

  struct Asset {
    address owner;
    address payable tokenAddress;
    bool filled;
    bool funded;
  }

  TToken private stableToken;
  address private mainContractAddress;

  Asset[] private assets;
  mapping (address => uint[]) private ownerToAssetIds;
  mapping (address => uint) private tokenToAssetIds;

  // allows us to easily calculate the total value of all assets
  uint[] private assetProjectedValuesUSD;

  modifier hasActiveAsset() {
    require(ownerToAssetIds[msg.sender].length != 0, "must have an active asset");
    _;
  }

  modifier validAsset(uint _id) {
    require(assets[_id].owner != address(0));
    _;
  }

  modifier onlyAssetOwner(uint _id) {
    require(assets[_id].owner == msg.sender);
    _;
  }
  1. the line numbers don't always seem to match up with the appropriate source code... or am I reading that wrong? (lines 1 and 3) here is the truffle artifact
  2. two internal mythx errors

Truffle v5.0.2 (core: 5.0.2) Solidity v0.5.0 (solc-js) Node v10.11.0

rocky commented 5 years ago

Thanks! Will look at soon.

rocky commented 5 years ago

Will look at soon.

Maybe not as soon as I would have liked.

The two internal errors have been fixed and I believe if you run again with truffle-security, version 2.0.0 that will be gone.

I would like to track down the location errors. The problem with this though is that this is a large set of contracts and files and although you have provided the truffle artifcat, I don't have access to all of the files needed to reproduce this.

And rather than run with all of the files, better would be if we could isloate this. I am guessing either the locations problems are fixe or you could get the same results but chop down the contract into something smaller that I can use to reproduce. Thanks in advance.

imthatcarlos commented 5 years ago

@rocky if you'd like, I can give you access to the repo so you can have my exact setup. Is that too much or what do you think?

rocky commented 5 years ago

Sure - give me access to the repo - thanks.

imthatcarlos commented 5 years ago

Just did, check out the branch mythx

rocky commented 5 years ago

@imthatcarlos thanks - with this I have been able to easily reproduce the bug and have enough information so I can pass it off to the responsible parties here.

If you are curious, the problems where the locations are 1:0 are caught by the fuzzer and relate to how the information it has gets melded back with other issues.

RIght now I have no time estimate on when this will get addressed, I'll keep you posted.

rocky commented 5 years ago

It looks like addressing and presenting it in a meaningful way is going to take some discussion.

I am told that the difficulty associating the bytecode offset with the source code is as a result of a dynamically-generated function call address.

So in the meantime here is some information that I hope may be useful. This is the raw output from the fuzzer in JSON format:

{
    "issues": [
        {
            "swc-id": "SWC-123",
            "title": "precondition violation",
            "details": "Precondition violations should be avoided when passing inputs to other contracts. Make sure your contracts provide only valid inputs to both callees (e.g, via passed arguments) and callers (e.g., via return values).",
            "code-hash": "0xd6a271e58404cfd95406d16bdc12f24f8e33aee4b87ebec2fb1d1c331091954a",
            "function-signature": "00000000",
            "bytecode-offset": 290,
            "stack-trace": [
                {
                    "op-code": "CREATE",
                    "code-hash": "0xb17b583de9bdbb8dbda78285df7abbe982b373490723cc2694896c87bc8dae64",
                    "function-signature": "6a0cc449",
                    "bytecode-offset": 4895
                }
            ],
            "time-secs": 4.497308861
        },
        {
            "swc-id": "SWC-110",
            "title": "assertion violation",
            "details": "Assertion violations should be avoided within your contracts. Make sure that your program logic is correct (e.g., no division by zero) and that you add appropriate validation for inputs from both callers (e.g, passed arguments) and callees (e.g., return values). If your contracts are written in Solidity you may want to use the \"require\" language construct for this purpose.",
            "code-hash": "0xb17b583de9bdbb8dbda78285df7abbe982b373490723cc2694896c87bc8dae64",
            "function-signature": "2854af99",
            "bytecode-offset": 3545,
            "stack-trace": [],
            "time-secs": 6.412805462
        },
        {
            "swc-id": "SWC-110",
            "title": "assertion violation",
            "details": "Assertion violations should be avoided within your contracts. Make sure that your program logic is correct (e.g., no division by zero) and that you add appropriate validation for inputs from both callers (e.g, passed arguments) and callees (e.g., return values). If your contracts are written in Solidity you may want to use the \"require\" language construct for this purpose.",
            "code-hash": "0xb17b583de9bdbb8dbda78285df7abbe982b373490723cc2694896c87bc8dae64",
            "function-signature": "0409efcd",
            "bytecode-offset": 2254,
            "stack-trace": [],
            "time-secs": 6.660975065
        },
        {
            "swc-id": "SWC-110",
            "title": "assertion violation",
            "details": "Assertion violations should be avoided within your contracts. Make sure that your program logic is correct (e.g., no division by zero) and that you add appropriate validation for inputs from both callers (e.g, passed arguments) and callees (e.g., return values). If your contracts are written in Solidity you may want to use the \"require\" language construct for this purpose.",
            "code-hash": "0xb17b583de9bdbb8dbda78285df7abbe982b373490723cc2694896c87bc8dae64",
            "function-signature": "b3e444a7",
            "bytecode-offset": 7175,
            "stack-trace": [],
            "time-secs": 9.666337898
        },
        {
            "swc-id": "SWC-127",
            "title": "jump to arbitrary destination",
            "details": "A caller can trigger a jump to an arbitrary destination.",
            "code-hash": "0x497a74b6f2beea96da5f6318de670706217e0edbcad5ce1bc5eaf89880d3f50b",
            "function-signature": "00000000",
            "bytecode-offset": 903,
            "stack-trace": [
                {
                    "op-code": "CREATE",
                    "code-hash": "0xb17b583de9bdbb8dbda78285df7abbe982b373490723cc2694896c87bc8dae64",
                    "function-signature": "6a0cc449",
                    "bytecode-offset": 4895
                }
            ],
            "time-secs": 26.878317139
        },
        {
            "swc-id": "SWC-127",
            "title": "jump to arbitrary destination",
            "details": "A caller can trigger a jump to an arbitrary destination.",
            "code-hash": "0xcc8f9d2be824285e0603317cba15118dd5c82ff0237ceb1b46daea8a99643370",
            "function-signature": "00000000",
            "bytecode-offset": 903,
            "stack-trace": [
                {
                    "op-code": "CREATE",
                    "code-hash": "0xb17b583de9bdbb8dbda78285df7abbe982b373490723cc2694896c87bc8dae64",
                    "function-signature": "6a0cc449",
                    "bytecode-offset": 4895
                }
            ],
            "time-secs": 78.71825101
        }
    ],
    "covered-paths": 721,
    "covered-instructions": 276715,
    "error": "",
    "total-time-secs": 96.005005074
}
imthatcarlos commented 5 years ago

thanks for the comprehensive answer! appreciate it.

rocky commented 5 years ago

Now that truffle-security 1.3.0 and armlet 2.2.0 have been released, would you try these out to see if problems are still there?

If not, then close the issue.

imthatcarlos commented 5 years ago

I updated and still have the mismatched line numbers. You did mention it's difficult. FWIW:

MythX Logs:
info: failed to start automated fuzz testing
info: failed to start automated fuzz testing
rocky commented 5 years ago

For the automated fuzz test messages we will be adding the UUID in there, see #115

I also opened an issue regarding what is causing this problem, but that is not publically visible.

The line number problem, yes is going to take a while. The plan as I understand it right now is to just say that this was associated in code created by "new".

In an ideal world we would return the bytecode and offset, and those of use who can do magic with that might be able to go further. See http://github.com/rocky/python-uncompyle6 for example.