Consensys / truffle-security

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

False positive SWC-123 warning #224

Open elenadimitrova opened 5 years ago

elenadimitrova commented 5 years ago

The following lines falsely produce SWC-123 warnings

contract OneTxPaymentFactory is ExtensionFactory, ColonyDataTypes { contract OldRolesFactory is ExtensionFactory, ColonyDataTypes {

see https://github.com/JoinColony/colonyNetwork/blob/develop/contracts/extensions/OneTxPaymentFactory.sol#L28 and https://github.com/JoinColony/colonyNetwork/blob/develop/contracts/extensions/OldRolesFactory.sol#L28

nbanmp commented 5 years ago

This is likely the same or similar issue to #222, causing a real issue in an imported contract to be reported in a different contract.

elenadimitrova commented 5 years ago

This error still persists even after upgrading to 1.5.2 which does fix #222 , see failing build here https://circleci.com/gh/JoinColony/colonyNetwork/8986.

Please reopen.

nbanmp commented 5 years ago

:(

nbanmp commented 5 years ago

Okay, I was able to sync up with someone who knows what is happening here.

What is happening is that the fuzzer tries to pass the address of the contract itself as _colony and tries to call function hasUserRole on itself which will fail in the fall-back function, which is compiler-generated. The tool then reports two locations for the issue: (1) the location of the call site (i.e., line with hasUserRole) and, (2) the location of the failing require (i.e., in the compiler generated code which will often map to the entire contract).

elenadimitrova commented 5 years ago

This makes sense and would explain a few more instances of where this is happening for us, list below:

https://github.com/JoinColony/colonyNetwork/blob/develop/contracts/ColonyFunding.sol#L25 https://github.com/JoinColony/colonyNetwork/blob/develop/contracts/ContractRecovery.sol#L28 https://github.com/JoinColony/colonyNetwork/blob/develop/contracts/TokenLocking.sol#L29