perf: slither analysis (#24)

emmanuelJet · Oct 10, 2024 · 29dff17 · 29dff17
1 parent 3aa434b
commit 29dff17
Show file tree

Hide file tree

Showing 30 changed files with 48 additions and 49 deletions.
diff --git a/foundry.toml b/foundry.toml
@@ -2,7 +2,7 @@
 src = "src"
 out = "out"
 libs = ["lib"]
-solc = "0.8.27"
+solc = "0.8.20"
 
 [fmt]
 tab_width = 2

diff --git a/script/MultiSigEnterpriseVault.s.sol b/script/MultiSigEnterpriseVault.s.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 import {Script, console} from 'forge-std/Script.sol';
 import {MultiSigEnterpriseVault} from '../src/MultiSigEnterpriseVault.sol';

diff --git a/src/MultiSigEnterpriseVault.sol b/src/MultiSigEnterpriseVault.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 import {MultiSigTransaction} from './components/MultiSigTransaction.sol';
 import {AddressUtils} from './libraries/AddressUtils.sol';

diff --git a/src/components/MultiSigTimelock.sol b/src/components/MultiSigTimelock.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 import '../libraries/Counters.sol';
 import {User} from './user/User.sol';
@@ -181,9 +181,9 @@ abstract contract MultiSigTimelock is User, IMultiSigTimelock {
   ) public validSigner validAction(actionId) pendingAction(actionId) {
     Action storage action = _actions[actionId];
     if (action.signatures.contains(_msgSender())) revert ActionNotApproved(actionId);
+    if (!action.signatures.add(_msgSender())) revert ActionNotApproved(actionId);
 
     action.approvals.increment();
-    action.signatures.add(_msgSender());
     emit ActionApproved(actionId, _msgSender(), block.timestamp);
   }
 
@@ -196,9 +196,9 @@ abstract contract MultiSigTimelock is User, IMultiSigTimelock {
   ) public validSigner validAction(actionId) pendingAction(actionId) {
     Action storage action = _actions[actionId];
     if (!action.signatures.contains(_msgSender())) revert ActionNotApproved(actionId);
+    if (!action.signatures.remove(_msgSender())) revert ActionNotApproved(actionId);
 
     action.approvals.decrement();
-    action.signatures.remove(_msgSender());
     emit ActionRevoked(actionId, _msgSender(), block.timestamp);
   }
 

diff --git a/src/components/MultiSigTransaction.sol b/src/components/MultiSigTransaction.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 import '../libraries/Counters.sol';
 import '../libraries/AddressUtils.sol';
@@ -84,14 +84,15 @@ abstract contract MultiSigTransaction is MultiSigTimelock, IMultiSigTransaction
    * @inheritdoc IMultiSigTransaction
    */
   function depositToken(address token, uint256 amount) external payable nonReentrant {
-    uint256 allowance = IERC20(token).allowance(_msgSender(), address(this));
+    IERC20 erc20Token = IERC20(token);
+    uint256 allowance = erc20Token.allowance(_msgSender(), address(this));
 
     if (allowance < amount) {
-      uint256 remainingAllowance = amount.subtract(allowance);
-      revert ERC20InsufficientAllowance(_msgSender(), allowance, remainingAllowance);
+      uint256 neededAllowance = amount.subtract(allowance);
+      revert ERC20InsufficientAllowance(_msgSender(), allowance, neededAllowance);
     }
 
-    IERC20(token).transferFrom(_msgSender(), address(this), amount);
+    SafeERC20.safeTransferFrom(erc20Token, _msgSender(), address(this), amount);
     emit FundsReceived(_msgSender(), token, amount);
   }
 
@@ -141,9 +142,9 @@ abstract contract MultiSigTransaction is MultiSigTimelock, IMultiSigTransaction
   ) public validSigner validTransaction(txId) pendingTransaction(txId) {
     Transaction storage txn = _transactions[txId];
     if (txn.signatures.contains(_msgSender())) revert TransactionNotApproved(txId);
+    if (!txn.signatures.add(_msgSender())) revert TransactionNotApproved(txId);
 
     txn.approvals.increment();
-    txn.signatures.add(_msgSender());
     emit TransactionApproved(txId, _msgSender(), block.timestamp);
   }
 
@@ -156,9 +157,9 @@ abstract contract MultiSigTransaction is MultiSigTimelock, IMultiSigTransaction
   ) public validSigner validTransaction(txId) pendingTransaction(txId) {
     Transaction storage txn = _transactions[txId];
     if (!txn.signatures.contains(_msgSender())) revert TransactionNotApproved(txId);
+    if (!txn.signatures.remove(_msgSender())) revert TransactionNotApproved(txId);
 
     txn.approvals.decrement();
-    txn.signatures.remove(_msgSender());
     emit TransactionRevoked(txId, _msgSender(), block.timestamp);
   }
 

diff --git a/src/components/user/User.sol b/src/components/user/User.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 import '../../libraries/Counters.sol';
 import '../../utilities/VaultConstants.sol';

diff --git a/src/components/user/roles/ExecutorRole.sol b/src/components/user/roles/ExecutorRole.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 import {AccessControl} from '@openzeppelin/contracts/access/AccessControl.sol';
 import {IExecutorRole} from '../../../interfaces/user/roles/IExecutorRole.sol';

diff --git a/src/components/user/roles/OwnerRole.sol b/src/components/user/roles/OwnerRole.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 import {AccessControl} from '@openzeppelin/contracts/access/AccessControl.sol';
 import {IOwnerRole} from '../../../interfaces/user/roles/IOwnerRole.sol';
@@ -20,26 +20,26 @@ abstract contract OwnerRole is AccessControl, IOwnerRole {
 
   /**
    * @dev Initializes the Owner role and sets the initial owner override timelock.
-   * @param owner_ The address of the initial owner.
+   * @param initialOwner The address of the initial owner.
    * @param initialOwnerOverrideTimelock The initial timelock value for owner override.
    */
-  constructor(address owner_, uint256 initialOwnerOverrideTimelock) {
-    AddressUtils.requireValidUserAddress(owner_);
+  constructor(address initialOwner, uint256 initialOwnerOverrideTimelock) {
+    AddressUtils.requireValidUserAddress(initialOwner);
     if (initialOwnerOverrideTimelock <= 0) {
       revert InvalidOwnerOverrideTimelockValue(initialOwnerOverrideTimelock);
     }
 
     // Grant DEFAULT_ADMIN_ROLE to the owner
-    _grantRole(DEFAULT_ADMIN_ROLE, owner_);
+    _grantRole(DEFAULT_ADMIN_ROLE, initialOwner);
 
     // Now change the admin role of DEFAULT_ADMIN_ROLE to OWNER_ROLE
     _setRoleAdmin(DEFAULT_ADMIN_ROLE, OWNER_ROLE);
 
     // Grant OWNER_ROLE to the owner
-    _grantRole(OWNER_ROLE, owner_);
+    _grantRole(OWNER_ROLE, initialOwner);
 
     ownerOverrideTimelock = initialOwnerOverrideTimelock;
-    _owner = owner_;
+    _owner = initialOwner;
   }
 
   /**

diff --git a/src/components/user/roles/SignerRole.sol b/src/components/user/roles/SignerRole.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 import {EnumerableSet} from '@openzeppelin/contracts/utils/structs/EnumerableSet.sol';
 import {AccessControl} from '@openzeppelin/contracts/access/AccessControl.sol';
@@ -83,9 +83,9 @@ abstract contract SignerRole is AccessControl, ISignerRole {
   ) internal virtual validExecutor {
     _validateSignerAddress(newSigner);
     if (isSigner(newSigner)) revert SignerAlreadyExists(newSigner);
+    if (!_signers.add(newSigner)) revert SignerAlreadyExists(newSigner);
 
     _grantRole(SIGNER_ROLE, newSigner);
-    _signers.add(newSigner);
     _signerCount.increment();
     emit SignerAdded(newSigner);
   }
@@ -99,9 +99,9 @@ abstract contract SignerRole is AccessControl, ISignerRole {
     address signer
   ) internal virtual validExecutor {
     if (!isSigner(signer)) revert SignerDoesNotExist(signer);
+    if (!_signers.remove(signer)) revert SignerDoesNotExist(signer);
 
     _revokeRole(SIGNER_ROLE, signer);
-    _signers.remove(signer);
     _signerCount.decrement();
     emit SignerRemoved(signer);
   }

diff --git a/src/interfaces/IMultiSigTimelock.sol b/src/interfaces/IMultiSigTimelock.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 import {ActionType} from '../utilities/VaultEnums.sol';
 

diff --git a/src/interfaces/IMultiSigTransaction.sol b/src/interfaces/IMultiSigTransaction.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 /**
  * @title IMultiSigTransaction Interface

diff --git a/src/interfaces/user/IUser.sol b/src/interfaces/user/IUser.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 /**
  * @title IUser Interface

diff --git a/src/interfaces/user/roles/IExecutorRole.sol b/src/interfaces/user/roles/IExecutorRole.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 /**
  * @title IExecutorRole Interface

diff --git a/src/interfaces/user/roles/IOwnerRole.sol b/src/interfaces/user/roles/IOwnerRole.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 /**
  * @title IOwnerRole Interface

diff --git a/src/interfaces/user/roles/ISignerRole.sol b/src/interfaces/user/roles/ISignerRole.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 /**
  * @title ISignerRole Interface

diff --git a/src/libraries/AddressUtils.sol b/src/libraries/AddressUtils.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 import '@openzeppelin/contracts/utils/Address.sol';
 

diff --git a/src/libraries/Counters.sol b/src/libraries/Counters.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 import './SafeMath.sol';
 

diff --git a/src/libraries/SafeMath.sol b/src/libraries/SafeMath.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 /**
  * @title SafeMath Library

diff --git a/src/utilities/VaultConstants.sol b/src/utilities/VaultConstants.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 /// @dev Role identifier for the Owner role in bytes32
 bytes32 constant OWNER_ROLE = keccak256('RoleType.OWNER');

diff --git a/src/utilities/VaultEnums.sol b/src/utilities/VaultEnums.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 /**
  * @title Vault RoleType Enum

diff --git a/src/utilities/VaultStructs.sol b/src/utilities/VaultStructs.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 import '@openzeppelin/contracts/utils/structs/EnumerableSet.sol';
 import '../libraries/Counters.sol';

diff --git a/test/MultiSigEnterpriseVault.t.sol b/test/MultiSigEnterpriseVault.t.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 import {Test, console} from 'forge-std/Test.sol';
 import {MultiSigEnterpriseVault} from '../src/MultiSigEnterpriseVault.sol';

diff --git a/test/components/BaseMultiSigTest.t.sol b/test/components/BaseMultiSigTest.t.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 import '../MultiSigEnterpriseVault.t.sol';
 

diff --git a/test/components/MultiSigFuzzTest.t.sol b/test/components/MultiSigFuzzTest.t.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 import {IERC20} from '@openzeppelin/contracts/token/ERC20/IERC20.sol';
 import {Address} from '@openzeppelin/contracts/utils/Address.sol';
@@ -61,9 +61,7 @@ contract MultiSigFuzzTest is BaseMultiSigTest {
         && recipient != vaultExecutor && recipient != vaultAddress
     );
 
-    vm.deal(vaultOwner, 100 ether);
-    vm.prank(vaultOwner);
-    Address.sendValue(payable(vaultAddress), 10 ether);
+    vm.deal(vaultAddress, 100 ether);
     uint256 initialRecipientBalance = recipient.balance;
     uint256 initialVaultBalance = vaultAddress.balance;
 

diff --git a/test/components/MultiSigTimelockTest.t.sol b/test/components/MultiSigTimelockTest.t.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 import {ActionType} from '../../src/utilities/VaultEnums.sol';
 import './BaseMultiSigTest.t.sol';

diff --git a/test/components/MultiSigTransactionTest.t.sol b/test/components/MultiSigTransactionTest.t.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 import {IERC20} from '@openzeppelin/contracts/token/ERC20/IERC20.sol';
 import {Address} from '@openzeppelin/contracts/utils/Address.sol';

diff --git a/test/components/roles/ExecutorRoleTest.t.sol b/test/components/roles/ExecutorRoleTest.t.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 import '../../MultiSigEnterpriseVault.t.sol';
 import {RoleType} from '../../../src/utilities/VaultEnums.sol';

diff --git a/test/components/roles/OwnerRoleTest.t.sol b/test/components/roles/OwnerRoleTest.t.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 import '../../MultiSigEnterpriseVault.t.sol';
 import {RoleType} from '../../../src/utilities/VaultEnums.sol';

diff --git a/test/components/roles/SignerRoleTest.t.sol b/test/components/roles/SignerRoleTest.t.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 import '../../MultiSigEnterpriseVault.t.sol';
 

diff --git a/test/mocks/MockERC20Token.sol b/test/mocks/MockERC20Token.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.27;
+pragma solidity ^0.8.20;
 
 import '@openzeppelin/contracts/token/ERC20/ERC20.sol';