Incorrect source maps

[muellerberndt]

Incorrect source mappings are sometimes reported. This issue was first reported by @glesaint in the MythX plugin for Truffle.

E.g. ERC 20 TokenTimelock.sol:

$ sabre TokenTimelock.sol

utils/Address.sol
27:263  warning  Multiple sends are executed in one transaction  https://smartcontractsecurity.github.io/SWC-registry/docs/SWC-113
math/SafeMath.sol
66:1307  warning  The contract executes an external message call  https://smartcontractsecurity.github.io/SWC-registry/docs/SWC-107

This issue was fixed in MythX-Truffle with PR 185.

[muellerberndt]

Here is an analysis of the bug:

Summary: This is a bug in truffle-security not sending all the necessary sources in its request to the api, and mucking with the source maps.

In detail:

solc-js generates source maps that refer to multiple different files. For TokenTimelock.sol it generates a source map containing the entry “3172:25:2” where the “2” refers to the file SafeERC20.sol truffle-security only includes 1 source file in its request to MythX and zeroes-out any file references. This is the culprit code with a telling comment:

// Take truffle's build/contracts/xxx.json JSON and make it
// compatible with the Mythril Platform API
const truffle2MythXJSON = function(truffleJSON, toolId = 'truffle-security') {
    ...
    // FIXME: why do we only one sourcePath in sourceList?
    // We shouldn't be zeroing this but instead correcting sourceList to
    // have the multiple entries.
    sourceMap = srcmap.zeroedSourceMap(sourceMap);
    deployedSourceMap = srcmap.zeroedSourceMap(deployedSourceMap);

This piece of code is where the bug is

3. truffle-security makes a request to the API containing only TokenTimelock.sol ‘s source-code, and a source map that contains the modified entry “3172:25:0”. Mythril finds a bug, and uses the buggy source map to report the location “3172:25:0”, which doesn’t exist in TokenTimelock.sol (but does exist in SafeERC20.sol)

[muellerberndt]

The imports test project can be used to test this: Each imported file contains an assert violation, Sabre should report the correct filename and line/column for each issue.

[Skyge]

Just like what you said, Sabre makes a request to the API, so does this mean that the bug generates at the original file of the API rather than in Sabre.

[blitz-1306]

Is this issue still actual? I rerun the tests on imports test project and have no problems with it, except that API, aside from floating pragma, detected only one assert violation in for ImportTest.sol, matching assert(5 == 6) at funcInExternalDir1(). Other files were processed just fine.

This may be awkward, but API response is properly processed and no mispointing locations are posted. Here is the output:

sabre project_directory/ImportTest.sol

✔ Compiled with solc v0.5.10 successfully
==== Floating Pragma ====
Severity: Low
File: /home/blitz/development/solc-imports-test/project_directory/ImportTest.sol
Link: https://smartcontractsecurity.github.io/SWC-registry/docs/SWC-103
--------------------
A floating pragma is set.
It is recommended to make a conscious choice on what version of Solidity is used for compilation. Currently multiple versions "^0.5.0" are allowed.
--------------------
Location: from 1:0 to 1:23

pragma solidity ^0.5.0;

==== Assert Violation ====
Severity: Low
File: /home/blitz/development/solc-imports-test/somedir/InExternalDir2.sol
Link: https://smartcontractsecurity.github.io/SWC-registry/docs/SWC-110
--------------------
A reachable exception has been detected.
It is possible to trigger an exception (opcode 0xfe). Exceptions can be caused by type errors, division by zero, out-of-bounds array access, or assert violations. Note that explicit `assert()` should only be used to check invariants. Use `require()` for regular input checking.
--------------------
Location: from 6:2 to 6:16

assert(5 == 6)
--------------------
Transaction Sequence:

Tx #1:
    Origin: 0xaffeaffeaffeaffeaffeaffeaffeaffeaffeaffe [ CREATOR ]
    Function: funcInExternalDir1() [ f59dd579 ]
    Data: 0xf59dd579
    Value: 0x0
--------------------
Transaction Sequence:

Tx #1:
    Origin: 0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa0 [ USER ]
    Function: funcInExternalDir1() [ f59dd579 ]
    Data: 0xf59dd57900000000000000000000000000000000000000000000000000
    Value: 0x0

[muellerberndt]

Yeah, I think this one should be fixed by now.