search

unlock-protocol / unlock

Ʉnlock is a protocol for memberships built on a blockchain.
https://unlock-protocol.com
MIT License
834 stars 244 forks source link

Consider truffle-security for "automated smart contract security analysis" #1763

Closed HardlyDifficult closed 5 years ago

HardlyDifficult commented 5 years ago

Is your feature request related to a problem? Please describe. n/a

Describe the solution you'd like Investigate using truffle-security

Describe alternatives you've considered truffle-security is a wrapper for MythX which we could consider instead. The value of truffle-security is it can be part of our CI build.

Additional context See https://github.com/ConsenSys/truffle-security

HardlyDifficult commented 5 years ago

and/or slither: static analyzer

HardlyDifficult commented 5 years ago

not before the Solidity 5 update is complete

HardlyDifficult commented 5 years ago

hmm this fails with E:/UnlockProtocol/unlock/smart-contracts/contracts/Unlock.sol 1:0 warning precondition violation SWC-123

✖ 1 problem (0 errors, 1 warning)

These smart contracts were unable to be analyzed: PublicLock, MixinKeys Internal MythX errors encountered: MixinKeys: Analysis failed: source list index out of range PublicLock: Analysis failed: source list index out of range

HardlyDifficult commented 5 years ago

Checked in with the team on Discord, issue is in progress. eta 2 days or so

HardlyDifficult commented 5 years ago

Update: fix should be included in the next release of truffle-security.

HardlyDifficult commented 5 years ago

Fix released

HardlyDifficult commented 5 years ago

New issue filed: https://github.com/ConsenSys/truffle-security/issues/114

HardlyDifficult commented 5 years ago

Update released

julien51 commented 5 years ago

Thanks @hardlydifficult . This is a very interesting idea. What is the status?

HardlyDifficult commented 5 years ago

@julien51 check out #2218 for the initial report.

ATM I'm not clear if adding this to CI builds would be wise or not. But at the least we can run it occasionally and before a release.

julien51 commented 5 years ago

Thanks for this! Any reason why you think this would not be wise in CI?

HardlyDifficult commented 5 years ago

It depends on an external API. The run is about 8 mins and if the service goes down it could be a pain for the team. It's also rolling out as a paid service at some point.

That aside, I wonder if CI is the right place for something like this. In interest in moving forward I wonder if this would encourage bad practices in order to suppress warnings. CI is nice for early feedback of concerns we had not considered, especially if they make us re-think the approach in some way. But it's not like a lint error where it makes sense to address it with every check in.

HardlyDifficult commented 5 years ago

Blocked on a new issue tracked here: https://github.com/ConsenSys/truffle-security/issues/158