fix: h/m/l issues found with Slither (#455)

* high: false positive arbitrary-send

* medium: reentrancy-no-eth

* medium: false positive calls-loop

* low: false positives reentrancy-events

* fix lint

* low: optimization constable-states external-func

* added new task to slither gh workflow

.github/workflows/slither.yml

@@ -43,6 +43,10 @@ jobs:
           python -m pip install --upgrade pip
           pip3 install slither-analyzer==0.8.1 solc-select==0.2.1
       
-      - name: Summary of static ananlysis
+      - name: Summary of static analysis
         run: |
           slither . --print human-summary
+
+      - name: High/Med/Low issues
+        run: |
+          slither . --config-file slither.config.json

contracts/adapters/CouponOnboarding.sol

@@ -128,6 +128,7 @@ contract CouponOnboardingContract is AdapterGuard, Signatures {
      * @param nonce is a unique identifier for this coupon request
      * @param signature is message signature for verification
      */
+    // function is protected against reentrancy attack with the reentrancyGuard(dao)
     // slither-disable-next-line reentrancy-benign
     function redeemCoupon(
         DaoRegistry dao,
@@ -137,6 +138,11 @@ contract CouponOnboardingContract is AdapterGuard, Signatures {
         bytes memory signature
     ) external reentrancyGuard(dao) {
         uint256 currentFlag = _flags[address(dao)][nonce / 256];
+        _flags[address(dao)][nonce / 256] = DaoHelper.setFlag(
+            currentFlag,
+            nonce % 256,
+            true
+        );
         require(
             DaoHelper.getFlag(currentFlag, nonce % 256) == false,
             "coupon already redeemed"
@@ -154,12 +160,6 @@ contract CouponOnboardingContract is AdapterGuard, Signatures {
             "invalid sig"
         );
 
-        _flags[address(dao)][nonce / 256] = DaoHelper.setFlag(
-            currentFlag,
-            nonce % 256,
-            true
-        );
-
         IERC20 erc20 = IERC20(
             dao.getAddressConfiguration(ERC20InternalTokenAddr)
         );

contracts/adapters/KycOnboarding.sol

@@ -224,6 +224,10 @@ contract KycOnboardingContract is AdapterGuard, Signatures {
      * @param tokenAddr is the address the ETH address(0) or an ERC20 Token address
      * @param signature is message signature for verification
      */
+    // The function is protected against reentrancy with the reentrancyGuard(dao)
+    // so it is fine to change some state after the reentrancyGuard(dao) external call
+    // because it calls the dao contract to lock the session/transaction flow.
+    // slither-disable-next-line reentrancy-benign,reentrancy-events
     function _onboard(
         DaoRegistry dao,
         address kycedMember,
@@ -239,20 +243,26 @@ contract KycOnboardingContract is AdapterGuard, Signatures {
         require(dao.getNbMembers() < maxMembers, "the DAO is full");
 
         _checkKycCoupon(dao, kycedMember, tokenAddr, signature);
+
         OnboardingDetails memory details = _checkData(dao, tokenAddr, amount);
+        totalUnits[dao][tokenAddr] += details.unitsRequested;
 
         BankExtension bank = BankExtension(
             dao.getExtensionAddress(DaoHelper.BANK)
         );
         DaoHelper.potentialNewMember(kycedMember, dao, bank);
-        totalUnits[dao][tokenAddr] += details.unitsRequested;
+
         address payable multisigAddress = payable(
             dao.getAddressConfiguration(
                 _configKey(tokenAddr, FundTargetAddress)
             )
         );
         if (multisigAddress == address(0x0)) {
             if (tokenAddr == DaoHelper.ETH_TOKEN) {
+                // The bank address is loaded from the DAO registry,
+                // hence even if we change that, it belongs to the DAO,
+                // so it is fine to send eth to it.
+                // slither-disable-next-line arbitrary-send
                 bank.addToBalance{value: details.amount}(
                     DaoHelper.GUILD,
                     DaoHelper.ETH_TOKEN,
@@ -269,6 +279,10 @@ contract KycOnboardingContract is AdapterGuard, Signatures {
             }
         } else {
             if (tokenAddr == DaoHelper.ETH_TOKEN) {
+                // The _weth address is defined during the deployment of the contract
+                // There is no way to change it once it has been deployed,
+                // so it is fine to send eth to it.
+                // slither-disable-next-line arbitrary-send
                 _weth.deposit{value: details.amount}();
                 _weth20.safeTransferFrom(
                     address(this),

contracts/adapters/Managing.sol

@@ -181,6 +181,10 @@ contract ManagingContract is IManaging, AdapterGuard {
         ProposalDetails memory proposal
     ) internal {
         for (uint256 i = 0; i < proposal.extensionAclFlags.length; i++) {
+            // It is fine to execute the external call inside the loop
+            // because it is calling the correct function in the dao contract
+            // it won't be calling a fallback that always revert.
+            // slither-disable-next-line calls-loop
             dao.setAclToExtensionForAdapter(
                 // It needs to be registered as extension
                 proposal.extensionAddresses[i],
@@ -205,8 +209,16 @@ contract ManagingContract is IManaging, AdapterGuard {
         for (uint256 i = 0; i < configs.length; i++) {
             Configuration memory config = configs[i];
             if (ConfigType.NUMERIC == config.configType) {
+                // It is fine to execute the external call inside the loop
+                // because it is calling the correct function in the dao contract
+                // it won't be calling a fallback that always revert.
+                // slither-disable-next-line calls-loop
                 dao.setConfiguration(config.key, config.numericValue);
             } else if (ConfigType.ADDRESS == config.configType) {
+                // It is fine to execute the external call inside the loop
+                // because it is calling the correct function in the dao contract
+                // it won't be calling a fallback that always revert.
+                // slither-disable-next-line calls-loop
                 dao.setAddressConfiguration(config.key, config.addressValue);
             }
         }

contracts/adapters/voting/KickBadReporterAdapter.sol

@@ -72,7 +72,7 @@ contract KickBadReporterAdapter is MemberGuard {
             votingState == IVoting.VotingState.NOT_PASS ||
             votingState == IVoting.VotingState.TIE
         ) {
-            //slither-disable-next-line uninitialized-local variable-scope
+            //slither-disable-next-line uninitialized-local,variable-scope
             (uint256 units, address challengeAddress) = votingContract
                 .getChallengeDetails(dao, proposalId);
             BankExtension bank = BankExtension(

contracts/extensions/bank/BankFactory.sol

@@ -48,6 +48,7 @@ contract BankFactory is IFactory, CloneFactory, ReentrancyGuard {
      * @notice Create and initialize a new BankExtension
      * @param maxExternalTokens The maximum number of external tokens stored in the Bank
      */
+    // slither-disable-next-line reentrancy-events
     function create(address dao, uint8 maxExternalTokens)
         external
         nonReentrant

contracts/extensions/token/erc20/ERC20TokenExtensionFactory.sol

@@ -49,6 +49,7 @@ contract ERC20TokenExtensionFactory is IFactory, CloneFactory, ReentrancyGuard {
     /**
      * @notice Creates a clone of the ERC20 Token Extension.
      */
+    // slither-disable-next-line reentrancy-events
     function create(
         address dao,
         string calldata tokenName,

contracts/helpers/WETH.sol

@@ -4,9 +4,9 @@
 pragma solidity ^0.8.0;
 
 contract WETH {
-    string public name = "Wrapped Ether";
-    string public symbol = "WETH";
-    uint8 public decimals = 18;
+    string public constant name = "Wrapped Ether";
+    string public constant symbol = "WETH";
+    uint8 public constant decimals = 18;
 
     event Approval(address indexed src, address indexed guy, uint256 wad);
     event Transfer(address indexed src, address indexed dst, uint256 wad);
@@ -25,25 +25,25 @@ contract WETH {
         emit Deposit(msg.sender, msg.value);
     }
 
-    function withdraw(uint256 wad) public {
+    function withdraw(uint256 wad) external {
         require(balanceOf[msg.sender] >= wad, "not enough WETH to withdraw");
         require(address(this).balance >= wad, "not enough ETH in the contract");
         balanceOf[msg.sender] -= wad;
         payable(msg.sender).transfer(wad);
         emit Withdrawal(msg.sender, wad);
     }
 
-    function totalSupply() public view returns (uint256) {
+    function totalSupply() external view returns (uint256) {
         return address(this).balance;
     }
 
-    function approve(address guy, uint256 wad) public returns (bool) {
+    function approve(address guy, uint256 wad) external returns (bool) {
         allowance[msg.sender][guy] = wad;
         emit Approval(msg.sender, guy, wad);
         return true;
     }
 
-    function transfer(address dst, uint256 wad) public returns (bool) {
+    function transfer(address dst, uint256 wad) external returns (bool) {
         return transferFrom(msg.sender, dst, wad);
     }