fix: security fixes (#471)

* fixing issues found during audit

* fix fallback check

* fix compilation issues

* fix tests

* fix tests

* disable fp slither issue

* bump slither version

Co-authored-by: david roon <david@roon.me>

.github/workflows/slither.yml

@@ -41,7 +41,7 @@ jobs:
       - name: Install Slither
         run: |
           python -m pip install --upgrade pip
-          pip3 install slither-analyzer==0.8.1 solc-select==0.2.1
+          pip3 install slither-analyzer==0.8.2 solc-select==0.2.1
       
       - name: Summary of static analysis
         run: |

contracts/adapters/voting/OffchainVoting.sol

@@ -587,14 +587,22 @@ contract OffchainVotingContract is
         external
         reentrancyGuard(dao)
         onlyMember(dao)
-        reimbursable(dao)
     {
-        // slither-disable-next-line timestamp
+        VotingState state = voteResult(dao, proposalId);
+        require(
+            state != VotingState.PASS &&
+                state != VotingState.NOT_PASS &&
+                state != VotingState.TIE,
+            "voting ended"
+        );
+
+        address memberAddr = dao.getAddressIfDelegated(msg.sender);
+        // slither-disable-next-line timestamp,incorrect-equality
         require(
-            votes[address(dao)][proposalId].fallbackVotes[msg.sender] == false,
+            votes[address(dao)][proposalId].fallbackVotes[memberAddr] == false,
             "fallback vote duplicate"
         );
-        votes[address(dao)][proposalId].fallbackVotes[msg.sender] = true;
+        votes[address(dao)][proposalId].fallbackVotes[memberAddr] = true;
         votes[address(dao)][proposalId].fallbackVotesCount += 1;
 
         if (

contracts/core/DaoRegistry.sol

@@ -179,6 +179,10 @@ contract DaoRegistry is MemberGuard, AdapterGuard {
      * @dev Sets the state of the dao to READY
      */
     function finalizeDao() external {
+        require(
+            isActiveMember(this, msg.sender) || isAdapter(msg.sender),
+            "not allowed to finalize"
+        );
         state = DaoState.READY;
     }
 

contracts/extensions/bank/Bank.sol

@@ -35,7 +35,7 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 SOFTWARE.
  */
 
-contract BankExtension is AdapterGuard, IExtension, ERC165 {
+contract BankExtension is IExtension, ERC165 {
     using Address for address payable;
     using SafeERC20 for IERC20;
 
@@ -95,11 +95,12 @@ contract BankExtension is AdapterGuard, IExtension, ERC165 {
     /// @notice Clonable contract must have an empty constructor
     constructor() {}
 
+    // slither-disable-next-line calls-loop
     modifier hasExtensionAccess(AclFlag flag) {
         require(
             address(this) == msg.sender ||
                 address(dao) == msg.sender ||
-                dao.state() == DaoRegistry.DaoState.CREATION ||
+                DaoHelper.isInCreationModeAndHasAccess(dao) ||
                 dao.hasAdapterAccessToExtension(
                     msg.sender,
                     address(this),

contracts/extensions/erc1155/ERC1155TokenExtension.sol

@@ -1,8 +1,8 @@
 pragma solidity ^0.8.0;
 // SPDX-License-Identifier: MIT
 import "../../core/DaoRegistry.sol";
-import "../../guards/AdapterGuard.sol";
 import "../../guards/MemberGuard.sol";
+import "../../helpers/DaoHelper.sol";
 import "../IExtension.sol";
 import "@openzeppelin/contracts/utils/Address.sol";
 import "@openzeppelin/contracts/token/ERC1155/IERC1155.sol";
@@ -33,12 +33,7 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 SOFTWARE.
  */
 
-contract ERC1155TokenExtension is
-    AdapterGuard,
-    MemberGuard,
-    IExtension,
-    IERC1155Receiver
-{
+contract ERC1155TokenExtension is MemberGuard, IExtension, IERC1155Receiver {
     using Address for address payable;
     //LIBRARIES
     using EnumerableSet for EnumerableSet.UintSet;
@@ -87,7 +82,7 @@ contract ERC1155TokenExtension is
     //MODIFIERS
     modifier hasExtensionAccess(IExtension extension, AclFlag flag) {
         require(
-            dao.state() == DaoRegistry.DaoState.CREATION ||
+            DaoHelper.isInCreationModeAndHasAccess(dao) ||
                 dao.hasAdapterAccessToExtension(
                     msg.sender,
                     address(extension),

contracts/extensions/erc1271/ERC1271.sol

@@ -4,6 +4,7 @@ pragma solidity ^0.8.0;
 
 import "../../core/DaoRegistry.sol";
 import "../IExtension.sol";
+import "../../helpers/DaoHelper.sol";
 import "../../guards/AdapterGuard.sol";
 import "@openzeppelin/contracts/interfaces/IERC1271.sol";
 
@@ -34,7 +35,7 @@ SOFTWARE.
 /**
  * @dev Signs arbitrary messages and exposes ERC1271 interface
  */
-contract ERC1271Extension is AdapterGuard, IExtension, IERC1271 {
+contract ERC1271Extension is IExtension, IERC1271 {
     using Address for address payable;
 
     bool public initialized = false; // internally tracks deployment under eip-1167 proxy pattern
@@ -58,7 +59,7 @@ contract ERC1271Extension is AdapterGuard, IExtension, IERC1271 {
         require(
             address(this) == msg.sender ||
                 address(dao) == msg.sender ||
-                dao.state() == DaoRegistry.DaoState.CREATION ||
+                DaoHelper.isInCreationModeAndHasAccess(dao) ||
                 dao.hasAdapterAccessToExtension(
                     msg.sender,
                     address(this),

contracts/extensions/executor/Executor.sol

@@ -4,7 +4,6 @@ pragma solidity ^0.8.0;
 
 import "../../core/DaoRegistry.sol";
 import "../IExtension.sol";
-import "../../guards/AdapterGuard.sol";
 
 /**
 MIT License
@@ -39,7 +38,7 @@ SOFTWARE.
  * This contract was based on the OpenZeppelin Proxy contract:
  * https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/proxy/Proxy.sol
  */
-contract ExecutorExtension is AdapterGuard, IExtension {
+contract ExecutorExtension is IExtension {
     using Address for address payable;
 
     bool public initialized = false; // internally tracks deployment under eip-1167 proxy pattern
@@ -56,7 +55,7 @@ contract ExecutorExtension is AdapterGuard, IExtension {
         require(
             address(this) == msg.sender ||
                 address(dao) == msg.sender ||
-                dao.state() == DaoRegistry.DaoState.CREATION ||
+                DaoHelper.isInCreationModeAndHasAccess(dao) ||
                 dao.hasAdapterAccessToExtension(
                     msg.sender,
                     address(this),

contracts/extensions/nft/NFT.sol

@@ -3,7 +3,6 @@ pragma solidity ^0.8.0;
 // SPDX-License-Identifier: MIT
 
 import "../../core/DaoRegistry.sol";
-import "../../guards/AdapterGuard.sol";
 import "../IExtension.sol";
 import "@openzeppelin/contracts/utils/Address.sol";
 import "@openzeppelin/contracts/token/ERC721/IERC721.sol";
@@ -34,7 +33,7 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 SOFTWARE.
  */
 
-contract NFTExtension is AdapterGuard, IExtension, IERC721Receiver {
+contract NFTExtension is IExtension, IERC721Receiver {
     using Address for address payable;
     // Add the library methods
     using EnumerableSet for EnumerableSet.UintSet;
@@ -69,7 +68,7 @@ contract NFTExtension is AdapterGuard, IExtension, IERC721Receiver {
 
     modifier hasExtensionAccess(IExtension extension, AclFlag flag) {
         require(
-            dao.state() == DaoRegistry.DaoState.CREATION ||
+            DaoHelper.isInCreationModeAndHasAccess(dao) ||
                 dao.hasAdapterAccessToExtension(
                     msg.sender,
                     address(extension),

contracts/extensions/token/erc20/InternalTokenVestingExtension.sol

@@ -3,6 +3,7 @@ pragma solidity ^0.8.0;
 // SPDX-License-Identifier: MIT
 import "../../../core/DaoRegistry.sol";
 import "../../../extensions/IExtension.sol";
+import "../../../helpers/DaoHelper.sol";
 
 /**
 MIT License
@@ -45,7 +46,7 @@ contract InternalTokenVestingExtension is IExtension {
 
     modifier hasExtensionAccess(AclFlag flag) {
         require(
-            _dao.state() == DaoRegistry.DaoState.CREATION ||
+            DaoHelper.isInCreationModeAndHasAccess(_dao) ||
                 _dao.hasAdapterAccessToExtension(
                     msg.sender,
                     address(this),

contracts/guards/AdapterGuard.sol

@@ -3,7 +3,7 @@ pragma solidity ^0.8.0;
 // SPDX-License-Identifier: MIT
 
 import "../core/DaoRegistry.sol";
-import "../extensions/IExtension.sol";
+import "../helpers/DaoHelper.sol";
 
 /**
 MIT License
@@ -34,8 +34,8 @@ abstract contract AdapterGuard {
      */
     modifier onlyAdapter(DaoRegistry dao) {
         require(
-            (dao.state() == DaoRegistry.DaoState.CREATION &&
-                creationModeCheck(dao)) || dao.isAdapter(msg.sender),
+            dao.isAdapter(msg.sender) ||
+                DaoHelper.isInCreationModeAndHasAccess(dao),
             "onlyAdapter"
         );
         _;
@@ -58,18 +58,10 @@ abstract contract AdapterGuard {
 
     modifier hasAccess(DaoRegistry dao, DaoRegistry.AclFlag flag) {
         require(
-            (dao.state() == DaoRegistry.DaoState.CREATION &&
-                creationModeCheck(dao)) ||
+            DaoHelper.isInCreationModeAndHasAccess(dao) ||
                 dao.hasAdapterAccess(msg.sender, flag),
             "accessDenied"
         );
         _;
     }
-
-    function creationModeCheck(DaoRegistry dao) internal view returns (bool) {
-        return
-            dao.getNbMembers() == 0 ||
-            dao.isMember(msg.sender) ||
-            dao.isAdapter(msg.sender);
-    }
 }

contracts/helpers/DaoHelper.sol

@@ -175,4 +175,23 @@ library DaoHelper {
             }
         }
     }
+
+    /**
+     * A DAO is in creation mode is the state of the DAO is equals to CREATION and
+     * 1. The number of members in the DAO is ZERO or,
+     * 2. The sender of the tx is a DAO member (usually the DAO owner) or,
+     * 3. The sender is an adapter.
+     */
+    // slither-disable-next-line calls-loop
+    function isInCreationModeAndHasAccess(DaoRegistry dao)
+        internal
+        view
+        returns (bool)
+    {
+        return
+            dao.state() == DaoRegistry.DaoState.CREATION &&
+            (dao.getNbMembers() == 0 ||
+                dao.isMember(msg.sender) ||
+                dao.isAdapter(msg.sender));
+    }
 }

contracts/helpers/OffchainVotingHelper.sol

@@ -212,8 +212,8 @@ contract OffchainVotingHelperContract {
     ) external view returns (bool) {
         return
             fallbackVotesCount >
-            (dao.getNbMembers() * 100) /
-                dao.getConfiguration(FallbackThreshold);
+            (dao.getNbMembers() * dao.getConfiguration(FallbackThreshold)) /
+                100;
     }
 
     function isReadyToSubmitResult(

test/extensions/vesting.test.js

@@ -72,7 +72,8 @@ describe("Extension - Vesting", () => {
       daoOwner,
       UNITS,
       1000,
-      Math.floor(now.getTime() / 1000)
+      Math.floor(now.getTime() / 1000),
+      { from: daoOwner }
     );
 
     const v = await vesting.vesting(daoOwner, UNITS);
@@ -107,7 +108,8 @@ describe("Extension - Vesting", () => {
       daoOwner,
       UNITS,
       100,
-      Math.floor(now.getTime() / 1000)
+      Math.floor(now.getTime() / 1000),
+      { from: daoOwner }
     );
 
     let v = await vesting.vesting(daoOwner, UNITS);
@@ -126,7 +128,8 @@ describe("Extension - Vesting", () => {
       daoOwner,
       UNITS,
       100,
-      Math.floor(now.getTime() / 1000)
+      Math.floor(now.getTime() / 1000),
+      { from: daoOwner }
     );
 
     v = await vesting.vesting(daoOwner, UNITS);
@@ -184,7 +187,8 @@ describe("Extension - Vesting", () => {
       daoOwner,
       UNITS,
       100,
-      Math.floor(now.getTime() / 1000)
+      Math.floor(now.getTime() / 1000),
+      { from: daoOwner }
     );
 
     v = await vesting.vesting(daoOwner, UNITS);
@@ -203,7 +207,7 @@ describe("Extension - Vesting", () => {
       minBalance.toString() === "75" || minBalance.toString() === "76"
     ).equal(true);
 
-    await vesting.removeVesting(daoOwner, UNITS, 50);
+    await vesting.removeVesting(daoOwner, UNITS, 50, { from: daoOwner });
 
     minBalance = await vesting.getMinimumBalance(daoOwner, UNITS);
     expect(
@@ -213,7 +217,7 @@ describe("Extension - Vesting", () => {
 
   it("should not be possible to create a new vesting without the ACL permission", async () => {
     // Finalize the DAO to be able to check the extension permissions
-    await this.dao.finalizeDao();
+    await this.dao.finalizeDao({ from: daoOwner });
     const vesting = this.extensions.vestingExt;
     const now = new Date();
     await expectRevert(
@@ -230,7 +234,7 @@ describe("Extension - Vesting", () => {
 
   it("should not be possible to removeVesting a vesting schedule the without ACL permission", async () => {
     // Finalize the DAO to be able to check the extension permissions
-    await this.dao.finalizeDao();
+    await this.dao.finalizeDao({ from: daoOwner });
     const vesting = this.extensions.vestingExt;
     await expectRevert(
       vesting.removeVesting(daoOwner, UNITS, 100, { from: daoOwner }),

website/docs/tutorial/extensions/HowToCreateAnExtension.md

@@ -75,7 +75,7 @@ Example:
             // 2. Allowed if the DAO is calling the extension
                 address(dao) == msg.sender ||
             // 3. Allowed if the DAO state is in CREATION mode
-                dao.state() == DaoRegistry.DaoState.CREATION ||
+                DaoHelper.isInCreationModeAndHasAccess(dao) ||
             // 4. Allowed if the sender is a registered adapter
                 dao.hasAdapterAccessToExtension(
                     msg.sender,
@@ -167,7 +167,7 @@ contract MyExtension is DaoConstants, IExtension {
         // 2. Allowed if the DAO is calling the extension
         address(dao) == msg.sender ||
         // 3. Allowed if the DAO state is in CREATION mode
-        dao.state() == DaoRegistry.DaoState.CREATION ||
+        DaoHelper.isInCreationModeAndHasAccess(dao) ||
         // 4. Allowed if the sender is a registered adapter
         dao.hasAdapterAccessToExtension(msg.sender, address(this), uint8(flag)),
       // 5. Revert message