Merge pull request #110 from beehive-innovation/2021-10-01-slither

slither sweep

Author: thedavidmeister

contracts/claim/TierByConstructionClaim.sol

@@ -118,6 +118,9 @@ contract TierByConstructionClaim is TierByConstruction {
     }
 
     /// Implementing contracts need to define what is claimed.
+    // Slither false positive. This is intended to overridden.
+    // https://github.com/crytic/slither/issues/929
+    // slither-disable-next-line dead-code
     function _afterClaim(
         address account_,
         uint256 report_,

contracts/factory/Factory.sol

@@ -25,6 +25,9 @@ abstract contract Factory is IFactory, ReentrancyGuard {
     /// arguments and pass them to this function directly.
     ///
     /// @param data_ ABI encoded data to pass to child contract constructor.
+    // Slither false positive. This is intended to overridden.
+    // https://github.com/crytic/slither/issues/929
+    // slither-disable-next-line dead-code
     function _createChild(bytes calldata data_)
         internal
         virtual

contracts/pool/RedeemableERC20Pool.sol

@@ -143,6 +143,9 @@ contract RedeemableERC20Pool is Ownable, Phased {
     uint256 public immutable finalValuation;
 
     /// @param config_ All configuration for the `RedeemableERC20Pool`.
+    // Slither false positive. Constructors cannot be reentrant.
+    // https://github.com/crytic/slither/issues/887
+    // slither-disable-next-line reentrancy-benign
     constructor (RedeemableERC20PoolConfig memory config_) public {
         require(
             config_.reserveInit >= MIN_RESERVE_INIT,

contracts/redeemableERC20/RedeemableERC20.sol

@@ -233,6 +233,9 @@ contract RedeemableERC20 is
     /// clear bounds on gas etc.
     /// @return Dynamic `redeemables` mapped to a fixed size array.
     function getRedeemables() external view returns (address[8] memory) {
+        // Slither false positive here due to a bug in slither.
+        // https://github.com/crytic/slither/issues/884
+        // slither-disable-next-line uninitialized-local
         address[8] memory redeemablesArray_;
         for(uint256 i_ = 0;i_<redeemables.length;i_++) {
             redeemablesArray_[i_] = address(redeemables[i_]);
@@ -312,6 +315,9 @@ contract RedeemableERC20 is
 
     /// Sanity check to ensure `Phase.ONE` is the final phase.
     /// @inheritdoc Phased
+    // Slither false positive. This is overriding an Open Zeppelin hook.
+    // https://github.com/crytic/slither/issues/929
+    // slither-disable-next-line dead-code
     function _beforeScheduleNextPhase(uint32 nextPhaseBlock_)
         internal
         override
@@ -327,6 +333,9 @@ contract RedeemableERC20 is
     /// If a transfer involves either a sender or receiver with the relevant
     /// `unfreezables` state it will ignore these restrictions.
     /// @inheritdoc ERC20
+    // Slither false positive. This is overriding an Open Zeppelin hook.
+    // https://github.com/crytic/slither/issues/929
+    // slither-disable-next-line dead-code
     function _beforeTokenTransfer(
         address sender_,
         address receiver_,

contracts/test/BalancerCoreImports.sol

@@ -1,4 +1,6 @@
 // SPDX-License-Identifier: CAL
+// Slither false positive. Can't do anything about old Balancer code.
+// slither-disable-next-line solc-version
 pragma solidity 0.5.12;
 
 /// Imports to make artifacts available to test scripts.

contracts/tier/ReadWriteTier.sol

@@ -90,6 +90,9 @@ contract ReadWriteTier is ITier {
     /// @param startTier_ The tier the account had before this update.
     /// @param endTier_ The tier the account will have after this update.
     /// @param data_ Additional arbitrary data to inform update requirements.
+    // Slither false positive. This is intended to overridden.
+    // https://github.com/crytic/slither/issues/929
+    // slither-disable-next-line dead-code
     function _afterSetTier(
         address account_,
         Tier startTier_,

contracts/tier/VerifyTier.sol

@@ -27,6 +27,8 @@ contract VerifyTier is ReadOnlyTier {
     function report(address account_) public override view returns (uint256) {
         State memory state_ = verify.state(account_);
         if (
+            // This is comparing an enum variant so it must be equal.
+            // slither-disable-next-line incorrect-equality
             verify.statusAtBlock(
                 state_,
                 uint32(block.number)

contracts/trust/Trust.sol

@@ -353,6 +353,9 @@ contract Trust is ReentrancyGuard {
     /// https://github.com/crytic/slither/issues/887
     ///
     /// @param config_ Config for the Trust.
+    // Slither false positive. Constructors cannot be reentrant.
+    // https://github.com/crytic/slither/issues/887
+    // slither-disable-next-line reentrancy-benign
     constructor (
         TrustConfig memory config_,
         TrustRedeemableERC20Config memory trustRedeemableERC20Config_,

contracts/verify/Verify.sol

@@ -239,7 +239,10 @@ contract Verify is AccessControl {
         // A mistaken add requires an appeal to a REMOVER to restart the
         // process OR a new `msg.sender` (i.e. different wallet address).
         require(id_ != 0, "0_ID");
-        require(states[msg.sender].addedSince == 0, "PRIOR_ADD");
+        // The awkward < 1 here is to silence slither complaining about
+        // equality checks against `0`. The intent is to ensure that
+        // `addedSince` is not already set before we set it.
+        require(states[msg.sender].addedSince < 1, "PRIOR_ADD");
         states[msg.sender] = State(
             id_,
             uint32(block.number),

shell.nix

@@ -1,5 +1,9 @@
 let
-  pkgs = import <nixpkgs> { };
+  pkgs = import (builtins.fetchTarball {
+    name = "nixos-unstable-2021-10-01";
+    url = "https://github.com/nixos/nixpkgs/archive/82155ff501c7622cb2336646bb62f7624261f6d7.tar.gz";
+    sha256 = "0xv47cpgaxb4j46ggjx9gkg299m9cdfzar27xw5h5k2lg5d3dljg";
+  }) { };
 
   local-node = pkgs.writeShellScriptBin "local-node" ''
     hardhat node
@@ -31,24 +35,16 @@ let
   '';
 
   security-check = pkgs.writeShellScriptBin "security-check" ''
-    # Workaround a slither bug due to stale compiled artifacts.
-    # https://github.com/crytic/slither/issues/860
     rm -rf artifacts
-    rm -rf typechain
     rm -rf cache
-
-    # Install slither to a fresh tmp dir to workaround nix-shell immutability.
-    export td=$(mktemp -d)
-    python3 -m venv ''${td}/venv
-    source ''${td}/venv/bin/activate
-    pip install slither-analyzer
+    rm -rf node_modules
+    rm -rf typechain
+    rm -rf bin
+    npm install
 
     # Run slither against all our contracts.
     # Disable npx as nix-shell already handles availability of what we need.
-    # Some contracts are explicitly out of scope for slither:
-    # - configurable-rights-pool contracts
-    # - The test contracts that only exist so the test harness can drive unit tests and will never be deployed
-    # - Open Zeppelin contracts
+    # Dependencies and tests are out of scope.
     slither . --npx-disable --filter-paths="contracts/test" --exclude-dependencies
   '';
 
@@ -146,7 +142,7 @@ pkgs.stdenv.mkDerivation {
   buildInputs = [
     pkgs.nixpkgs-fmt
     pkgs.nodejs-14_x
-    pkgs.python3
+    pkgs.slither-analyzer
     local-node
     local-fork
     local-test