Add static contract analysis to CI #93
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
elenadimitrova
force-pushed
the
maintenance/security-testing
branch
from
58d5575 to
5027712
Compare
elenadimitrova
changed the title
elenadimitrova
force-pushed
the
maintenance/security-testing
branch
4 times, most recently
from
ce4fa35 to
2443d17
Compare
elenadimitrova
force-pushed
the
maintenance/security-testing
branch
from
bdb0ee7 to
1f9ee17
Compare
elenadimitrova
force-pushed
the
maintenance/security-testing
branch
2 times, most recently
from
0e24b47 to
ce8ef4d
Compare
elenadimitrova
changed the base branch from
develop
to
feature/relayer-rebase-all-modules
juniset
reviewed
May 13, 2020
| @@ -125,7 +125,7 @@ contract ArgentENSManager is IENSManager, Owned, Managed { | |||
| * @param _subnode The target subnode. | |||
| * @return true if the subnode is available. | |||
| */ | |||
| function isAvailable(bytes32 _subnode) public view returns (bool) { | |||
| function isAvailable(bytes32 _subnode) external view returns (bool) { | |||
There was a problem hiding this comment.
Is this change necessary? I thought the idea was to not touch the infrastructure contracts for the next release.
There was a problem hiding this comment.
These were raised by slither on the ENS contracts for this rule. They are not dangerous so have ignored them for now but good to fix in future.
| @@ -46,7 +46,7 @@ contract ArgentENSResolver is Owned, Managed, ENSResolver { | |||
| * @param _node The node to update. | |||
| * @param _addr The address to set. | |||
| */ | |||
| function setAddr(bytes32 _node, address _addr) public onlyManager { | |||
| function setAddr(bytes32 _node, address _addr) external onlyManager { | |||
There was a problem hiding this comment.
Same comment as above. Are these changes necessary for Slither or is just cleaning?
| _wallet.setOwner(recoveryOwner); | ||
| guardianStorage.setLock(_wallet, 0); | ||
|
|
||
| emit RecoveryFinalized(address(_wallet), config.recovery); |
There was a problem hiding this comment.
It should be recoveryOwner and not config.recovery.
| delete recoveryConfigs[address(_wallet)]; | ||
| guardianStorage.setLock(_wallet, 0); | ||
|
|
||
| emit RecoveryCanceled(address(_wallet), config.recovery); |
There was a problem hiding this comment.
Since we deleted the storage slot at position recoveryConfigs[address(_wallet)], shouldn't config.recovery always return 0 ?
There was a problem hiding this comment.
Yes you're right.
Configure slither run
elenadimitrova
force-pushed
the
maintenance/security-testing
branch
from
2046893 to
e3c35ce
Compare
elenadimitrova
changed the base branch from
feature/relayer-rebase-all-modules
to
develop
as per conversation with olivdb
Add exteptions to comply with the newly released slither 0.6.11
Also add test:coverage details
…ning in infrastructure contracts as those will not be updated in this release
elenadimitrova
force-pushed
the
maintenance/security-testing
branch
from
81b43ee to
d7ce2fc
Compare
as those won't be upgraded in next release 2.0
juniset
approved these changes
May 14, 2020
| @@ -145,9 +145,11 @@ contract RecoveryManager is BaseModule, RelayerModule { | |||
| */ | |||
| function cancelRecovery(BaseWallet _wallet) external onlyExecute onlyWhenRecovery(_wallet) { | |||
| RecoveryConfig storage config = recoveryConfigs[address(_wallet)]; | |||
| emit RecoveryCanceled(address(_wallet), config.recovery); | |||
| guardianStorage.setLock(_wallet, 0); | |||
| address recoveryOwner = config.recovery; | |||
There was a problem hiding this comment.
Since config is only used to define recoveryOwner it is probably more efficient to do
address recoveryOwner = recoveryConfigs[address(_wallet)].recovery;
ffe349c to
1a58c5a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Enables static analysis on the contracts using
slitherhttps://github.com/crytic/slitherThis is run by CI in a parallel python-based container on all builds now. Some valid suggested fixes were applied the rest were ignored in the
slither.db.json. Two new npm commands addedsecurity:slitherandsecurity:slither:triage, the former runs slither analyzer on the contracts and the latter runs the same but in triage mode allowing us to ignore errors that are either false positives or we don't consider a threat.Includes extracting all test contracts out in a
/contracts-testfolder due to overlap in contract names between dappsys and openzeppelin contracts we use in unit testing, namelyIERC20which trips upslitheratm.Removes duplication of
IUniswapExchangeandIUniswapFactorycontract definitions which otherwise causesslitherto error .Found and logged a couple of problems with the latest
slither release 0.6.11crytic/slither#456 and crytic/slither#457. Those were resolved inslither release 0.6.12