From 98bd7f67f46534d17059d68c5f9c966860ec6528 Mon Sep 17 00:00:00 2001 From: "Emmanuel Joseph (JET)" Date: Fri, 4 Oct 2024 23:40:40 +0100 Subject: [PATCH] feat: timelock and threshold (#22) --- README.md | 14 +- script/MultiSigEnterpriseVault.s.sol | 6 +- src/MultiSigEnterpriseVault.sol | 75 +++- src/components/MultiSigTimelock.sol | 384 ++++++++++++++++++ src/components/MultiSigTransaction.sol | 313 ++++++++------ src/components/roles/SignerRole.sol | 101 ----- src/components/{ => user}/User.sol | 80 ++-- .../{ => user}/roles/ExecutorRole.sol | 47 ++- src/components/{ => user}/roles/OwnerRole.sol | 67 +-- src/components/user/roles/SignerRole.sol | 127 ++++++ src/interfaces/IMultiSigEnterpriseVault.sol | 20 - src/interfaces/IMultiSigTimelock.sol | 163 ++++++++ src/interfaces/IMultiSigTransaction.sol | 28 +- src/interfaces/roles/ISignerRole.sol | 27 -- src/interfaces/{ => user}/IUser.sol | 8 +- .../{ => user}/roles/IExecutorRole.sol | 8 +- .../{ => user}/roles/IOwnerRole.sol | 4 +- src/interfaces/user/roles/ISignerRole.sol | 51 +++ src/libraries/AddressUtils.sol | 36 +- src/libraries/ArraysUtils.sol | 76 ---- src/libraries/ERC20Validator.sol | 61 +++ src/utilities/VaultEnums.sol | 26 +- src/utilities/VaultStructs.sol | 41 +- test/MultiSigEnterpriseVault.t.sol | 10 +- test/components/BaseMultiSigTest.t.sol | 8 + test/components/MultiSigFuzzTest.t.sol | 131 ++++-- test/components/MultiSigTimelockTest.t.sol | 191 +++++++++ test/components/MultiSigTransactionTest.t.sol | 70 +++- test/components/roles/ExecutorRoleTest.t.sol | 2 +- test/components/roles/OwnerRoleTest.t.sol | 12 +- test/components/roles/SignerRoleTest.t.sol | 24 +- test/mocks/MockERC20Token.sol | 6 +- 32 files changed, 1651 insertions(+), 566 deletions(-) create mode 100644 src/components/MultiSigTimelock.sol delete mode 100644 src/components/roles/SignerRole.sol rename src/components/{ => user}/User.sol (85%) rename src/components/{ => user}/roles/ExecutorRole.sol (75%) rename src/components/{ => user}/roles/OwnerRole.sol (50%) create mode 100644 src/components/user/roles/SignerRole.sol delete mode 100644 src/interfaces/IMultiSigEnterpriseVault.sol create mode 100644 src/interfaces/IMultiSigTimelock.sol delete mode 100644 src/interfaces/roles/ISignerRole.sol rename src/interfaces/{ => user}/IUser.sol (67%) rename src/interfaces/{ => user}/roles/IExecutorRole.sol (91%) rename src/interfaces/{ => user}/roles/IOwnerRole.sol (92%) create mode 100644 src/interfaces/user/roles/ISignerRole.sol delete mode 100644 src/libraries/ArraysUtils.sol create mode 100644 src/libraries/ERC20Validator.sol create mode 100644 test/components/MultiSigTimelockTest.t.sol diff --git a/README.md b/README.md index 14607a6..a1bfafb 100644 --- a/README.md +++ b/README.md @@ -4,10 +4,20 @@ An open-source, enterprise-grade Multi-Signature Vault smart contract developed ## Features -- Multi-signature functionality with flexible threshold settings. -- Separate timelocks for transactions and owner overrides. +- Multi-Signature Vault functionality to manage **ETH** and **ERC20** tokens. +- Separate timelocks for transactions and owner overrides with delays. - Role-based access control (Owner, Executor, Signers). - Secure self-destruct mechanism with safety checks. +- Flexible and administrative threshold settings. + +## Use Cases + +The MultiSigEnterpriseVault is ideal for: + +- **Enterprise Teams**: Organizations that require multiple approvals before executing high-value transactions, ensuring decentralized control. +- **DAOs (Decentralised Autonomous Organizations)**: Enables decentralized decision-making with threshold-based approvals for critical actions. +- **Family or Joint Accounts**: Multiple signatories can manage shared assets securely with transaction approval mechanisms. +- **Fund Custodians**: Securely manage pooled funds with transaction timelock to prevent unilateral decisions. ## Getting Started diff --git a/script/MultiSigEnterpriseVault.s.sol b/script/MultiSigEnterpriseVault.s.sol index a1bc739..4e50ca9 100644 --- a/script/MultiSigEnterpriseVault.s.sol +++ b/script/MultiSigEnterpriseVault.s.sol @@ -14,10 +14,12 @@ contract MultiSigEnterpriseVaultScript is Script { vm.startBroadcast(); uint256 initialThreshold = 3; - uint256 initialOwnerOverrideLimit = 3 days; + uint256 initialMultiSigTimelock = 1 days; + uint256 initialOwnerOverrideTimelock = 3 days; address vaultOwner = makeAddr('vaultOwner'); - vault = new MultiSigEnterpriseVault(vaultOwner, initialThreshold, initialOwnerOverrideLimit); + vault = + new MultiSigEnterpriseVault(vaultOwner, initialThreshold, initialMultiSigTimelock, initialOwnerOverrideTimelock); vaultAddress = address(vault); console.log('MultiSigVault Contract Address:', vaultAddress); diff --git a/src/MultiSigEnterpriseVault.sol b/src/MultiSigEnterpriseVault.sol index 717275e..790a20c 100644 --- a/src/MultiSigEnterpriseVault.sol +++ b/src/MultiSigEnterpriseVault.sol @@ -1,42 +1,85 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.27; -import {IMultiSigEnterpriseVault} from './interfaces/IMultiSigEnterpriseVault.sol'; import {MultiSigTransaction} from './components/MultiSigTransaction.sol'; import {AddressUtils} from './libraries/AddressUtils.sol'; /** * @title MultiSig Enterprise Vault Contract * @author Emmanuel Joseph (JET) + * @notice This contract manages a Multi-Signature Vault system, providing role-based access control with customizable + * signatory threshold and timelocks for actions. The vault supports both ETH and ERC20 tokens for transaction execution. */ -contract MultiSigEnterpriseVault is MultiSigTransaction, IMultiSigEnterpriseVault { +contract MultiSigEnterpriseVault is MultiSigTransaction { /** - * @dev Initializes the MultiSigEnterpriseVault with the owner, initial signatory threshold, and owner override limit. + * @dev Initializes the MultiSigEnterpriseVault with the owner, initial signatory threshold, initial timelock, and owner override timelock. * @param owner The address of the contract owner. * @param initialThreshold The initial threshold for signatory approval. - * @param initialOwnerOverrideLimit The initial timelock limit for owner override. + * @param initialMultiSigTimelock The initial timelock for signatory approval. + * @param initialOwnerOverrideTimelock The initial timelock for owner override. */ constructor( address owner, uint256 initialThreshold, - uint256 initialOwnerOverrideLimit - ) MultiSigTransaction(owner, initialThreshold, initialOwnerOverrideLimit) { - AddressUtils.requireValidUserAddress(owner); + uint256 initialMultiSigTimelock, + uint256 initialOwnerOverrideTimelock + ) MultiSigTransaction(owner, initialThreshold, initialMultiSigTimelock, initialOwnerOverrideTimelock) {} + + /** + * @dev Modifier to ensure an owner can perform an action without requiring signatory approval. + * Reverts with `SignersApprovalRequired` if the total number of signers is equal to or greater than the signatory threshold. + */ + modifier withoutSignersApproval() { + if (_totalValidSigners() >= signatoryThreshold) { + revert SignersApprovalRequired(); + } + _; } /** - * @notice Owner updates the signatory threshold for the vault. + * @notice Updates the signatory threshold for the vault. * @param newThreshold The new threshold value for signatory approval. - * @dev Only callable by the owner of the contract. + * @dev + * - Requires the total valid signers to be less than the signatory threshold. + * - Emits a `ThresholdUpdated` event upon successful execution. + * - Only callable by the owner. */ - function ownerUpdateSignatoryThreshold( + function updateSignatoryThreshold( uint256 newThreshold - ) public onlyOwner { - if (newThreshold > signatoryThreshold && totalSigners() >= signatoryThreshold) { - revert SignersApprovalRequired(); - } + ) public onlyOwner withoutSignersApproval { + _updateSignatoryThreshold(newThreshold); + } - signatoryThreshold = newThreshold; - emit ThresholdUpdated(newThreshold); + /** + * @notice Adds a new signer to the vault. + * @param newSigner The address of the new signer. + * @dev + * - Requires the new signer to be a valid address. + * - Requires the total valid signers to be less than the signatory threshold. + * - Emits a `SignerAdded` event upon successful execution. + * - Only callable by the owner. + */ + function addSigner( + address newSigner + ) public onlyOwner withoutSignersApproval { + AddressUtils.requireValidUserAddress(newSigner); + if (isSigner(newSigner)) revert SignerAlreadyExists(newSigner); + _addSigner(newSigner); + } + + /** + * @notice Removes a signer from the vault. + * @param signer The address of the signer to be removed. + * @dev + * - Requires the signer to be a valid address. + * - Requires the total valid signers to be less than the signatory threshold. + * - Emits a `SignerRemoved` event upon successful execution. + * - Only callable by the owner. + */ + function removeSigner( + address signer + ) public onlyOwner withoutSignersApproval { + if (!isSigner(signer)) revert SignerDoesNotExist(signer); + _removeSigner(signer); } } diff --git a/src/components/MultiSigTimelock.sol b/src/components/MultiSigTimelock.sol new file mode 100644 index 0000000..02b78cd --- /dev/null +++ b/src/components/MultiSigTimelock.sol @@ -0,0 +1,384 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity ^0.8.27; + +import '../libraries/Counters.sol'; +import {User} from './user/User.sol'; +import {SafeMath} from '../libraries/SafeMath.sol'; +import {Action} from '../utilities/VaultStructs.sol'; +import {ActionType} from '../utilities/VaultEnums.sol'; +import {IMultiSigTimelock} from '../interfaces/IMultiSigTimelock.sol'; +import {EnumerableSet} from '@openzeppelin/contracts/utils/structs/EnumerableSet.sol'; + +/** + * @title MultiSigTimelock + * @author Emmanuel Joseph (JET) + * @dev Manages the timelock logics in the MultiSigVault system. + */ +abstract contract MultiSigTimelock is User, IMultiSigTimelock { + using EnumerableSet for EnumerableSet.AddressSet; + using Counters for Counters.Counter; + using SafeMath for uint256; + + /// @dev Stores the multi-sig timelock for function execution. + uint256 public multiSigTimelock; + + /// @notice Stores the minimum number of approvals to execute an action + uint256 public signatoryThreshold; + + /// @notice Stores the check for a pending action that requires approvals + bool private _isPendingAction; + + /// @notice Using Counter library for total actions + Counters.Counter private _actionCount; + + /// @notice Mapping to store all actions by their ID + mapping(uint256 => Action) private _actions; + + /** + * @dev Initializes the `MultiSigTimelock` and `User` contracts. + * @param owner The address of the contract owner. + * @param initialThreshold The initial threshold for signatory approval. + * @param initialMultiSigTimelock The initial timelock for signatory approval. + * @param initialOwnerOverrideTimelock The initial timelock for owner override. + */ + constructor( + address owner, + uint256 initialThreshold, + uint256 initialMultiSigTimelock, + uint256 initialOwnerOverrideTimelock + ) User(owner, initialOwnerOverrideTimelock) { + if (initialMultiSigTimelock <= 0) { + revert InvalidMultiSigTimelockValue(initialMultiSigTimelock); + } + + if (initialThreshold <= 0) { + revert InvalidSignatoryThreshold(initialThreshold); + } + + signatoryThreshold = initialThreshold; + multiSigTimelock = initialMultiSigTimelock; + } + + /** + * @dev Modifier to check if the total number of signers meets or exceeds the signatory threshold and the contract has an executor. + * - Reverts with `InsufficientSigners` if the total signers doesn't meet or exceeds the signatory threshold. + * - Reverts with `MissingExecutor` if no valid executor exists in the multi-sig vault contract. + */ + modifier isExecutable() { + if (_totalValidSigners() < signatoryThreshold) { + revert InsufficientSigners(signatoryThreshold, _totalValidSigners()); + } + + if (executor() == address(0)) { + revert MissingExecutor(); + } + _; + } + + /** + * @dev Modifier to ensure a valid vault action type. + * Reverts with `InvalidAction` if the given action ID is invalid. + * @param actionType The type of the action to validate. + */ + modifier validActionType( + ActionType actionType + ) { + if (actionType == ActionType.INVALID) { + revert InvalidActionType(actionType); + } + _; + } + + /** + * @dev Modifier to ensure a valid vault action. + * Reverts with `InvalidAction` if the given action ID is invalid. + * @param actionId The ID of the action to validate. + */ + modifier validAction( + uint256 actionId + ) { + if (actionId == 0 || actionId > _actionCount.current()) { + revert InvalidAction(actionId); + } + _; + } + + /** + * @dev Modifier to ensure a valid vault action has't been executed. + * Reverts with `ActionAlreadyExecuted` if the valid action has been executed. + * @param actionId The ID of the action to validate. + */ + modifier pendingAction( + uint256 actionId + ) { + if (_actions[actionId].isExecuted) { + revert ActionAlreadyExecuted(actionId); + } + _; + } + + /** + * @notice Initiates a new action requiring approval. + * @param actionType The type of action. + * @param target The target address (for signer-related actions). + * @param value The associated value. + */ + function initiateAction( + ActionType actionType, + address target, + uint256 value + ) public validSigner isExecutable validActionType(actionType) { + if (_isPendingAction) revert PendingActionState(_isPendingAction); + + if (actionType == ActionType.ADD_SIGNER && target == address(0)) { + revert InvalidUserProfile(target); + } + + if (actionType == ActionType.ADD_SIGNER && isSigner(target)) { + revert SignerAlreadyExists(target); + } + + if (actionType == ActionType.REMOVE_SIGNER && !isSigner(target)) { + revert SignerDoesNotExist(target); + } + + if (actionType == ActionType.INCREASE_TIMELOCK && value <= multiSigTimelock) { + revert InvalidMultiSigTimelockIncrease(value); + } + + if (actionType == ActionType.DECREASE_TIMELOCK && value >= multiSigTimelock) { + revert InvalidMultiSigTimelockDecrease(value); + } + + if (actionType == ActionType.INCREASE_THRESHOLD && value <= signatoryThreshold) { + revert InvalidSignatoryThresholdIncrease(value); + } + + if (actionType == ActionType.DECREASE_THRESHOLD && value >= signatoryThreshold) { + revert InvalidSignatoryThresholdDecrease(value); + } + + _isPendingAction = true; + _actionCount.increment(); + uint256 actionId = _actionCount.current(); + Action storage newAction = _actions[actionId]; + + newAction.initiator = _msgSender(); + newAction.actionType = actionType; + newAction.target = target; + newAction.value = value; + newAction.timestamp = block.timestamp; + + emit ActionInitiated(actionId, _msgSender(), actionType); + } + + /** + * @notice Enables valid signers to approve a action. + * @param actionId The ID of the action to approve. + */ + function approveAction( + uint256 actionId + ) public validSigner validAction(actionId) pendingAction(actionId) { + Action storage action = _actions[actionId]; + if (action.signatures.contains(_msgSender())) revert ActionNotApproved(actionId); + + action.approvals.increment(); + action.signatures.add(_msgSender()); + emit ActionApproved(actionId, _msgSender(), block.timestamp); + } + + /** + * @notice Enables valid signers to revokes a action approval. + * @param actionId The ID of the action to revoke approval for. + */ + function revokeActionApproval( + uint256 actionId + ) public validSigner validAction(actionId) pendingAction(actionId) { + Action storage action = _actions[actionId]; + if (!action.signatures.contains(_msgSender())) revert ActionNotApproved(actionId); + + action.approvals.decrement(); + action.signatures.remove(_msgSender()); + emit ActionRevoked(actionId, _msgSender(), block.timestamp); + } + + /** + * @notice Executes the approved action if threshold approvals and timelock are met. + * @param actionId The ID of the action. + */ + function executeAction( + uint256 actionId + ) public validExecutor validAction(actionId) pendingAction(actionId) { + Action storage action = _actions[actionId]; + uint256 executionTimestamp = block.timestamp; + if (!_isMultiSigTimelockElapsed(action.timestamp)) { + uint256 requiredTime = action.timestamp.add(multiSigTimelock); + revert TimelockNotElapsed(requiredTime, executionTimestamp); + } + + if (_isOwner(_msgSender())) { + if (!action.signatures.contains(_msgSender())) revert ActionNotApproved(actionId); + if (!_isSignatoryThresholdMet(action.approvals.current())) { + revert InsufficientSignerApprovals(signatoryThreshold, action.approvals.current()); + } + } else { + action.isOverride = true; + } + + // Perform the action based on ActionType + if (action.actionType == ActionType.INCREASE_THRESHOLD || action.actionType == ActionType.DECREASE_THRESHOLD) { + _updateSignatoryThreshold(action.value); + } else if (action.actionType == ActionType.INCREASE_TIMELOCK || action.actionType == ActionType.DECREASE_TIMELOCK) { + _updateMultiSigTimelock(action.value); + } else if (action.actionType == ActionType.ADD_SIGNER) { + _addSigner(action.target); + } else if (action.actionType == ActionType.REMOVE_SIGNER) { + _removeSigner(action.target); + } + + _isPendingAction = false; + action.isExecuted = true; + emit ActionExecuted(actionId, action.actionType, _msgSender(), executionTimestamp); + } + + /** + * @notice Deletes the latest action item if not executed. + * @dev Only callable by a contract executor (Owner or Executor) + */ + function deletePendingAction() public validExecutor { + if (!_isPendingAction) revert PendingActionState(_isPendingAction); + + uint256 latestActionId = _actionCount.current(); + if (_actions[latestActionId].isExecuted) { + revert ActionAlreadyExecuted(latestActionId); + } + + delete _actions[latestActionId]; + _actionCount.decrement(); + } + + /** + * @notice Returns the total number of actions. + * @return uint256 The total number of actions. + * @dev Only callable by contract users. + */ + function totalActions() public view onlyUser returns (uint256) { + return _actionCount.current(); + } + + /** + * @notice Returns the action details of a given ID, excluding signatures. + * @param actionId The ID of the requested action. + * @return initiator The address of the action initiator. + * @return actionType The type of action being initiated. + * @return target The target address (for signer-related actions). + * @return value The value associated with the action (e.g., timelock or threshold values). + * @return timestamp The time when the action was initiated. + * @return approvals The total number of approvals. + * @return isExecuted Whether the action has been executed. + * @return isOverride Whether the action was overridden by the executor. + */ + function getAction( + uint256 actionId + ) + public + view + onlyUser + validAction(actionId) + returns ( + address initiator, + ActionType actionType, + address target, + uint256 value, + uint256 timestamp, + uint256 approvals, + bool isExecuted, + bool isOverride + ) + { + Action storage action = _actions[actionId]; + return ( + action.initiator, + action.actionType, + action.target, + action.value, + action.timestamp, + action.approvals.current(), + action.isExecuted, + action.isOverride + ); + } + + /** + * @notice Returns the total number of approvals an action has. + * @param actionId The ID of the requested action. + * @return uint256 The action total number of approvers. + */ + function getActionApprovals( + uint256 actionId + ) public view validAction(actionId) onlyUser returns (uint256) { + return _actions[actionId].approvals.current(); + } + + /** + * @notice Returns the signatures associated with an action. + * @param actionId The ID of the requested action. + * @return address[] The action signatures array. + */ + function getActionSignatures( + uint256 actionId + ) public view validAction(actionId) onlyUser returns (address[] memory) { + Action storage action = _actions[actionId]; + return action.signatures.values(); + } + + /** + * @notice Verifies if the signatory threshold has been met for an action. + * @param approvalCount The current number of approvals for the action. + * @return bool Returns true if the required signatory threshold is met, otherwise false. + */ + function _isSignatoryThresholdMet( + uint256 approvalCount + ) internal view returns (bool) { + return approvalCount >= signatoryThreshold; + } + + /** + * @notice Verifies if the timelock period has passed for an action. + * @param initiatedAt The timestamp when the action was initiated. + * @return bool Returns true if the timelock period has elapsed, otherwise false. + */ + function _isMultiSigTimelockElapsed( + uint256 initiatedAt + ) internal view returns (bool) { + return block.timestamp >= initiatedAt + multiSigTimelock; + } + + /** + * @notice Updates the signatory threshold for the vault. + * Emits the `ThresholdUpdated` event. + * @param newThreshold The new threshold value for signatory approval. + * @dev Only callable by the contract executors. + */ + function _updateSignatoryThreshold( + uint256 newThreshold + ) internal validExecutor { + uint256 oldThreshold = signatoryThreshold; + signatoryThreshold = newThreshold; + emit ThresholdUpdated(oldThreshold, newThreshold); + } + + /** + * @notice Updates the multi-sig vault timelock. + * Emits the `MultiSigTimelockUpdated` event. + * @param newLimit The new timelock value for action execution. + * @dev Only callable by the contract executors. + */ + function _updateMultiSigTimelock( + uint256 newLimit + ) private validExecutor { + uint256 oldLimit = multiSigTimelock; + multiSigTimelock = newLimit; + emit MultiSigTimelockUpdated(oldLimit, newLimit); + } +} diff --git a/src/components/MultiSigTransaction.sol b/src/components/MultiSigTransaction.sol index 8226148..ef7a5e9 100644 --- a/src/components/MultiSigTransaction.sol +++ b/src/components/MultiSigTransaction.sol @@ -6,20 +6,25 @@ import '../libraries/AddressUtils.sol'; import '../utilities/VaultConstants.sol'; import {IERC20} from '@openzeppelin/contracts/token/ERC20/IERC20.sol'; import {SafeERC20} from '@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol'; +import {EnumerableSet} from '@openzeppelin/contracts/utils/structs/EnumerableSet.sol'; import {IMultiSigTransaction} from '../interfaces/IMultiSigTransaction.sol'; +import {ERC20Validator} from '../libraries/ERC20Validator.sol'; import {Transaction} from '../utilities/VaultStructs.sol'; -import {ArraysUtils} from '../libraries/ArraysUtils.sol'; -import {User} from './User.sol'; +import {MultiSigTimelock} from './MultiSigTimelock.sol'; +import {SafeMath} from '../libraries/SafeMath.sol'; /** * @title MultiSigTransaction + * @author Emmanuel Joseph (JET) * @dev Manages transaction initiation, approval, and execution for ETH and ERC20 transfers. */ -abstract contract MultiSigTransaction is User, IMultiSigTransaction { +abstract contract MultiSigTransaction is MultiSigTimelock, IMultiSigTransaction { + using EnumerableSet for EnumerableSet.AddressSet; using Counters for Counters.Counter; + using SafeMath for uint256; - /// @notice The minimum number of approvals to execute a transaction - uint256 public signatoryThreshold; + /// @notice Stores the check for a pending transaction that requires approvals + bool private _isPendingTransaction; /// @notice Using Counter library for total transactions Counters.Counter private _transactionCount; @@ -27,33 +32,44 @@ abstract contract MultiSigTransaction is User, IMultiSigTransaction { /// @notice Mapping to store all transactions by their ID mapping(uint256 => Transaction) private _transactions; - /// @notice Mapping to track approvals for each transaction by signers - mapping(uint256 => mapping(address => bool)) public approvals; - /** - * @dev Initializes the `MultiSigTransaction` and `User` contracts. + * @dev Initializes the `MultiSigTransaction` and `MultiSigTimelock` contracts. * @param owner The address of the contract owner. * @param initialThreshold The initial threshold for signatory approval. - * @param initialOwnerOverrideLimit The initial timelock limit for owner override. + * @param initialMultiSigTimelock The initial timelock for signatory approval. + * @param initialOwnerOverrideTimelock The initial timelock for owner override. */ constructor( address owner, uint256 initialThreshold, - uint256 initialOwnerOverrideLimit - ) User(owner, initialOwnerOverrideLimit) { - signatoryThreshold = initialThreshold; - } + uint256 initialMultiSigTimelock, + uint256 initialOwnerOverrideTimelock + ) MultiSigTimelock(owner, initialThreshold, initialMultiSigTimelock, initialOwnerOverrideTimelock) {} /** * @dev Modifier to ensure a valid vault transaction. * Reverts with `InvalidTransaction` if the given transaction ID is invalid. - * @param transactionId The ID of the transaction to validate. + * @param txId The ID of the transaction to validate. */ modifier validTransaction( - uint256 transactionId + uint256 txId ) { - if (transactionId == 0 || transactionId < _transactionCount.current()) { - revert InvalidTransaction(transactionId); + if (txId == 0 || txId > _transactionCount.current()) { + revert InvalidTransaction(txId); + } + _; + } + + /** + * @dev Modifier to ensure a valid vault transaction has't been executed. + * Reverts with `TransactionAlreadyExecuted` if the valid transaction has been executed. + * @param txId The ID of the transaction to validate. + */ + modifier pendingTransaction( + uint256 txId + ) { + if (_transactions[txId].isExecuted) { + revert TransactionAlreadyExecuted(txId); } _; } @@ -69,170 +85,223 @@ abstract contract MultiSigTransaction is User, IMultiSigTransaction { * @inheritdoc IMultiSigTransaction */ function depositToken(address token, uint256 amount) external payable { - AddressUtils.requireValidTokenAddress(token); - require(amount >= msg.value, 'MultiSigTransaction: Invalid deposit amount'); - - IERC20(token).transferFrom(_msgSender(), address(this), msg.value); - emit FundsReceived(_msgSender(), token, msg.value); - } - - /** - * @notice Returns the balance of ETH held in the contract. - * @return The ETH balance of the contract. - */ - function getBalance() public view returns (uint256) { - return address(this).balance; - } - - /** - * @notice Returns the balance of a specific ERC20 token held in the contract. - * @param token The ERC20 token address. - * @return The token balance of the contract. - */ - function getTokenBalance( - address token - ) public view returns (uint256) { - AddressUtils.requireValidTokenAddress(token); - return IERC20(token).balanceOf(address(this)); - } + ERC20Validator.requireValidERC20Token(token); + uint256 allowance = IERC20(token).allowance(_msgSender(), address(this)); - /** - * @notice Returns the total number of transactions. - * @return uint256 The total number of transactions. - * @dev Only callable by the owner. - */ - function totalTransactions() public view returns (uint256) { - return _transactionCount.current(); - } - - /** - * @notice Returns the transaction details of a given ID. - * @param transactionId The ID of the requested transaction. - * @return Transaction The transaction object associated with the provided ID. - */ - function getTransaction( - uint256 transactionId - ) public view validTransaction(transactionId) onlyUser returns (Transaction memory) { - return _transactions[transactionId]; - } - - /** - * @notice Returns the total number of approvals a transaction has. - * @param transactionId The ID of the requested transaction. - * @return uint256 The transaction total number of approvers. - */ - function getTransactionApprovals( - uint256 transactionId - ) public view validTransaction(transactionId) onlyUser returns (uint256) { - return _transactions[transactionId].approvals.current(); - } + if (allowance < amount) { + uint256 remainingAllowance = amount.subtract(allowance); + revert ERC20InsufficientAllowance(_msgSender(), allowance, remainingAllowance); + } - /** - * @notice Returns the signatures associated with a transaction. - * @param transactionId The ID of the requested transaction. - * @return address[] The transaction signatures array. - */ - function getTransactionSignatures( - uint256 transactionId - ) public view validTransaction(transactionId) onlyUser returns (address[] memory) { - return _transactions[transactionId].signatures; + IERC20(token).transferFrom(_msgSender(), address(this), amount); + emit FundsReceived(_msgSender(), token, amount); } /** * @notice Initiates a new transaction by an Owner or Signer. - * @param target The target address for the transaction. + * @param to The address receiving the transaction token. * @param value The amount of ETH or tokens to send. * @param token The ERC20 token address (0x0 for ETH). * @param data The transaction data (0x0 for empty data). */ function initiateTransaction( - address payable target, + address payable to, address token, uint256 value, bytes memory data - ) public validSigner { - AddressUtils.requireValidTransactionTarget(target); + ) public validSigner isExecutable { + AddressUtils.requireValidTransactionReceiver(to); + if (_isPendingTransaction) revert PendingTransactionState(_isPendingTransaction); + if (token == address(0)) { if (value > getBalance()) revert InsufficientTokenBalance(getBalance(), value); } else { + ERC20Validator.requireValidERC20Token(token); if (value > getTokenBalance(token)) revert InsufficientTokenBalance(getTokenBalance(token), value); } + _isPendingTransaction = true; _transactionCount.increment(); uint256 transactionId = _transactionCount.current(); Transaction storage txn = _transactions[transactionId]; txn.initiator = _msgSender(); - txn.target = target; + txn.to = to; txn.token = token; txn.value = value; + txn.timestamp = block.timestamp; txn.data = data; - emit TransactionInitiated(transactionId, _msgSender(), target, token, value); + emit TransactionInitiated(transactionId, _msgSender(), to, token, value); } /** * @notice Enables valid signers to approve a transaction. - * @param transactionId The ID of the transaction to approve. + * @param txId The ID of the transaction to approve. */ function approveTransaction( - uint256 transactionId - ) public validTransaction(transactionId) validSigner { - Transaction storage txn = _transactions[transactionId]; - if (txn.isExecuted) revert TransactionAlreadyExecuted(transactionId); - if (approvals[transactionId][_msgSender()]) revert TransactionNotApproved(transactionId); + uint256 txId + ) public validSigner validTransaction(txId) pendingTransaction(txId) { + Transaction storage txn = _transactions[txId]; + if (txn.signatures.contains(_msgSender())) revert TransactionNotApproved(txId); txn.approvals.increment(); - txn.signatures.push(_msgSender()); - approvals[transactionId][_msgSender()] = true; - emit TransactionApproved(transactionId, _msgSender(), block.timestamp); + txn.signatures.add(_msgSender()); + emit TransactionApproved(txId, _msgSender(), block.timestamp); } /** * @notice Enables valid signers to revokes a transaction approval. - * @param transactionId The ID of the transaction to revoke approval for. + * @param txId The ID of the transaction to revoke approval for. */ - function revokeApproval( - uint256 transactionId - ) public validTransaction(transactionId) validSigner { - Transaction storage txn = _transactions[transactionId]; - if (txn.isExecuted) revert TransactionAlreadyExecuted(transactionId); - if (!approvals[transactionId][_msgSender()]) revert TransactionNotApproved(transactionId); + function revokeTransactionApproval( + uint256 txId + ) public validSigner validTransaction(txId) pendingTransaction(txId) { + Transaction storage txn = _transactions[txId]; + if (!txn.signatures.contains(_msgSender())) revert TransactionNotApproved(txId); txn.approvals.decrement(); - approvals[transactionId][_msgSender()] = false; - uint256 signerSignatureIndex = ArraysUtils.arrayElementIndexLookup(_msgSender(), txn.signatures); - ArraysUtils.removeElementFromArray(signerSignatureIndex, txn.signatures); - - emit TransactionRevoked(transactionId, _msgSender(), block.timestamp); + txn.signatures.remove(_msgSender()); + emit TransactionRevoked(txId, _msgSender(), block.timestamp); } /** * @notice Executes a transaction if the threshold is met. - * @param transactionId The ID of the transaction to execute. + * @param txId The ID of the transaction to execute. */ function executeTransaction( - uint256 transactionId - ) public validTransaction(transactionId) validExecutor { - Transaction storage txn = _transactions[transactionId]; - if (txn.isExecuted) revert TransactionAlreadyExecuted(transactionId); - if (txn.approvals.current() < signatoryThreshold) { - revert InsufficientApprovals(signatoryThreshold, txn.approvals.current()); + uint256 txId + ) public validExecutor validTransaction(txId) pendingTransaction(txId) { + uint256 executionTimestamp = block.timestamp; + Transaction storage txn = _transactions[txId]; + if (!_isMultiSigTimelockElapsed(txn.timestamp)) { + uint256 requiredTime = txn.timestamp.add(multiSigTimelock); + revert TimelockNotElapsed(requiredTime, executionTimestamp); } - if (hasRole(OWNER_ROLE, _msgSender())) { - if (!approvals[transactionId][_msgSender()]) revert TransactionNotApproved(transactionId); + + if (_isOwner(_msgSender())) { + if (!txn.signatures.contains(_msgSender())) revert TransactionNotApproved(txId); + if (!_isSignatoryThresholdMet(txn.approvals.current())) { + revert InsufficientSignerApprovals(signatoryThreshold, txn.approvals.current()); + } + } else { + txn.isOverride = true; } txn.isExecuted = true; + _isPendingTransaction = false; if (txn.token == address(0)) { // Send ETH - Address.sendValue(txn.target, txn.value); + Address.sendValue(txn.to, txn.value); } else { // Send ERC20 tokens IERC20 token = IERC20(txn.token); - SafeERC20.safeTransfer(token, txn.target, txn.value); + SafeERC20.safeTransfer(token, txn.to, txn.value); } - emit TransactionExecuted(transactionId, _msgSender(), block.timestamp); + emit TransactionExecuted(txId, _msgSender(), block.timestamp); + } + + /** + * @notice Deletes the latest transaction item if not executed. + * @dev Only callable by a contract executor (Owner or Executor) + */ + function deletePendingTransaction() public validExecutor { + if (!_isPendingTransaction) revert PendingTransactionState(_isPendingTransaction); + + uint256 latestTxId = _transactionCount.current(); + if (_transactions[latestTxId].isExecuted) { + revert ActionAlreadyExecuted(latestTxId); + } + + delete _transactions[latestTxId]; + _transactionCount.decrement(); + } + + /** + * @notice Returns the balance of ETH held in the contract. + * @return The ETH balance of the contract. + */ + function getBalance() public view returns (uint256) { + return address(this).balance; + } + + /** + * @notice Returns the balance of a specific ERC20 token held in the contract. + * @param token The ERC20 token address. + * @return The token balance of the contract. + */ + function getTokenBalance( + address token + ) public returns (uint256) { + ERC20Validator.requireValidERC20Token(token); + return IERC20(token).balanceOf(address(this)); + } + + /** + * @notice Returns the total number of transactions. + * @return uint256 The total number of transactions. + * @dev Only callable by the owner. + */ + function totalTransactions() public view returns (uint256) { + return _transactionCount.current(); + } + + /** + * @notice Returns the transaction details of a given ID, excluding signatures. + * @param txId The ID of the requested transaction. + * @return initiator The address of the transaction initiator. + * @return to The address receiving the transaction tokens. + * @return token The token contract address (0x0 for ETH). + * @return value The ETH or token value to send. + * @return approvals The total number of approvals. + * @return isExecuted Whether the transaction has been executed. + * @return isOverride Whether the transaction was overridden by the executor. + * @return data The transaction data (0x0 for empty data). + */ + function getTransaction( + uint256 txId + ) + public + view + onlyUser + validTransaction(txId) + returns ( + address initiator, + address to, + address token, + uint256 value, + uint256 approvals, + bool isExecuted, + bool isOverride, + bytes memory data + ) + { + Transaction storage txn = _transactions[txId]; + return + (txn.initiator, txn.to, txn.token, txn.value, txn.approvals.current(), txn.isExecuted, txn.isOverride, txn.data); + } + + /** + * @notice Returns the total number of approvals a transaction has. + * @param txId The ID of the requested transaction. + * @return uint256 The transaction total number of approvers. + */ + function getTransactionApprovals( + uint256 txId + ) public view validTransaction(txId) onlyUser returns (uint256) { + return _transactions[txId].approvals.current(); + } + + /** + * @notice Returns the signatures associated with a transaction. + * @param txId The ID of the requested transaction. + * @return address[] The transaction signatures array. + */ + function getTransactionSignatures( + uint256 txId + ) public view validTransaction(txId) onlyUser returns (address[] memory) { + Transaction storage txn = _transactions[txId]; + return txn.signatures.values(); } } diff --git a/src/components/roles/SignerRole.sol b/src/components/roles/SignerRole.sol deleted file mode 100644 index 0131dfe..0000000 --- a/src/components/roles/SignerRole.sol +++ /dev/null @@ -1,101 +0,0 @@ -// SPDX-License-Identifier: GPL-3.0 -pragma solidity ^0.8.27; - -import {AccessControl} from '@openzeppelin/contracts/access/AccessControl.sol'; -import {SIGNER_ROLE, OWNER_ROLE} from '../../utilities/VaultConstants.sol'; -import {ISignerRole} from '../../interfaces/roles/ISignerRole.sol'; -import {AddressUtils} from '../../libraries/AddressUtils.sol'; -import {ArraysUtils} from '../../libraries/ArraysUtils.sol'; -import '../../libraries/Counters.sol'; - -/** - * @title Signer Role Contract - * @author Emmanuel Joseph (JET) - * @dev Abstract contract providing the logic for the Signer role in the MultiSigVault system. - */ -abstract contract SignerRole is AccessControl, ISignerRole { - using Counters for Counters.Counter; - using AddressUtils for address; - - /// @notice Using Counter library for total signers - Counters.Counter private _signerCount; - - /// @dev Stores store all signer addresses. - address[] private _signers; - - /** - * @dev Modifier to restrict access to functions to only the signer. - * Reverts with `AccessControlUnauthorizedSigner` if the caller is not a signer. - */ - modifier onlySigner() { - if (!isSigner(_msgSender())) { - revert AccessControlUnauthorizedSigner(_msgSender()); - } - _; - } - - /** - * @notice Checks if an address is a signer. - * @param signer The address to check. - * @return status True if the address is a signer, otherwise false. - */ - function isSigner( - address signer - ) public view returns (bool status) { - status = hasRole(SIGNER_ROLE, signer); - } - - /** - * @notice Returns the total number of signers. - * @return uint256 The number of signers in the system. - */ - function totalSigners() public view returns (uint256) { - return _signerCount.current(); - } - - /** - * @notice Returns an array of all current signers' addresses. - * @return address[] The list of signers' addresses. - */ - function getSigners() public view returns (address[] memory) { - return _signers; - } - - /** - * @notice Adds a new signer. - * @param newSigner The address of the new signer. - * @dev Only callable by the owner. - */ - function _addSigner( - address newSigner - ) internal onlyRole(OWNER_ROLE) { - require(!isSigner(newSigner), 'SignerRole: Signer already exists'); - newSigner.requireValidUserAddress(); - - grantRole(SIGNER_ROLE, newSigner); - _signers.push(newSigner); - _signerCount.increment(); - emit SignerAdded(newSigner); - } - - /** - * @notice Removes the current signer. - * @param signer The address of the signer to be removed. - * @dev Only callable by the owner. - */ - function _removeSigner( - address signer - ) internal onlyRole(OWNER_ROLE) { - require(isSigner(signer), 'SignerRole: Signer does not exist'); - - /// Remove Signer ROle - revokeRole(SIGNER_ROLE, signer); - - // Remove signer from the _signers array - uint256 signerIndex = ArraysUtils.arrayElementIndexLookup(signer, _signers); - ArraysUtils.removeElementFromArray(signerIndex, _signers); - _signerCount.decrement(); - - emit SignerRemoved(signer); - } -} diff --git a/src/components/User.sol b/src/components/user/User.sol similarity index 85% rename from src/components/User.sol rename to src/components/user/User.sol index b864cd2..8b89c2b 100644 --- a/src/components/User.sol +++ b/src/components/user/User.sol @@ -1,15 +1,16 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.27; -import '../libraries/Counters.sol'; -import '../utilities/VaultConstants.sol'; -import {IUser} from '../interfaces/IUser.sol'; +import '../../libraries/Counters.sol'; +import '../../utilities/VaultConstants.sol'; import {OwnerRole} from './roles/OwnerRole.sol'; import {SignerRole} from './roles/SignerRole.sol'; -import {RoleType} from '../utilities/VaultEnums.sol'; import {ExecutorRole} from './roles/ExecutorRole.sol'; -import {UserProfile} from '../utilities/VaultStructs.sol'; -import {AddressUtils} from '../libraries/AddressUtils.sol'; +import {IUser} from '../../interfaces/user/IUser.sol'; +import {SafeMath} from '../../libraries/SafeMath.sol'; +import {RoleType} from '../../utilities/VaultEnums.sol'; +import {UserProfile} from '../../utilities/VaultStructs.sol'; +import {AddressUtils} from '../../libraries/AddressUtils.sol'; /** * @title User Contract @@ -65,22 +66,11 @@ abstract contract User is OwnerRole, ExecutorRole, SignerRole, IUser { /** * @dev Modifier to restrict access to functions to valid signers. - * Reverts with `UnauthorizedTransactionSigner` if the caller is not an authorized transaction signer. + * Reverts with `UnauthorizedMultiSigSigner` if the caller is not an authorized multi-sig signer. */ modifier validSigner() { if (!isSigner(_msgSender()) && !hasRole(OWNER_ROLE, _msgSender())) { - revert UnauthorizedTransactionSigner(_msgSender()); - } - _; - } - - /** - * @dev Modifier to restrict access to functions to valid executors. - * Reverts with `UnauthorizedTransactionExecutor` if the caller is not an authorized transaction executor. - */ - modifier validExecutor() { - if (!hasRole(EXECUTOR_ROLE, _msgSender()) && !hasRole(OWNER_ROLE, _msgSender())) { - revert UnauthorizedTransactionExecutor(_msgSender()); + revert UnauthorizedMultiSigSigner(_msgSender()); } _; } @@ -153,15 +143,31 @@ abstract contract User is OwnerRole, ExecutorRole, SignerRole, IUser { _removeUser(oldExecutor); } + /** + * @notice Allows an executor to approve the owner override after the timelock has elapsed. + */ + function approveOwnerOverride() public onlyExecutor { + address currentOwner = owner(); + address currentExecutor = _msgSender(); + uint256 currentTimestamp = block.timestamp; + + _approveOwnerOverride(currentOwner, currentTimestamp, ownerOverrideTimelock); + _changeOwner(); + + _removeUser(currentOwner); + _removeUser(currentExecutor); + _addUser(currentExecutor, RoleType.OWNER); + } + /** * @notice Adds a new signer user. * @param newSigner The address of the new signer. * @dev Only callable by the owner. */ - function addSigner( + function _addSigner( address newSigner - ) public onlyOwner { - _addSigner(newSigner); + ) internal override validExecutor { + super._addSigner(newSigner); _addUser(newSigner, RoleType.SIGNER); } @@ -170,30 +176,14 @@ abstract contract User is OwnerRole, ExecutorRole, SignerRole, IUser { * @param signer The address of the signer to be removed. * @dev Only callable by the owner. */ - function removeSigner( + function _removeSigner( address signer - ) public onlyOwner { + ) internal override validExecutor { signer.requireValidUserAddress(); - _removeSigner(signer); + super._removeSigner(signer); _removeUser(signer); } - /** - * @notice Allows an executor to approve the owner override after the timelock has elapsed. - */ - function approveOwnerOverride() public onlyExecutor { - address currentOwner = owner(); - address currentExecutor = _msgSender(); - uint256 currentTimestamp = block.timestamp; - - _approveOwnerOverride(currentOwner, currentTimestamp, ownerOverrideTimelock); - _changeOwner(); - - _removeUser(currentOwner); - _removeUser(currentExecutor); - _addUser(currentExecutor, RoleType.OWNER); - } - /** * @notice Checks if an address is a user. * @param user The address to check. @@ -205,6 +195,14 @@ abstract contract User is OwnerRole, ExecutorRole, SignerRole, IUser { status = _users[user].user != address(0); } + /** + * @notice Returns the total number of valid signers. + * @return uint256 The total number of valid signers. + */ + function _totalValidSigners() internal view returns (uint256) { + return SafeMath.add(totalSigners(), 1); + } + /** * @dev Adds a user profile and assigns a role. * diff --git a/src/components/roles/ExecutorRole.sol b/src/components/user/roles/ExecutorRole.sol similarity index 75% rename from src/components/roles/ExecutorRole.sol rename to src/components/user/roles/ExecutorRole.sol index 28ecb29..b163a8b 100644 --- a/src/components/roles/ExecutorRole.sol +++ b/src/components/user/roles/ExecutorRole.sol @@ -2,10 +2,10 @@ pragma solidity ^0.8.27; import {AccessControl} from '@openzeppelin/contracts/access/AccessControl.sol'; -import {IExecutorRole} from '../../interfaces/roles/IExecutorRole.sol'; -import {AddressUtils} from '../../libraries/AddressUtils.sol'; -import {SafeMath} from '../../libraries/SafeMath.sol'; -import '../../utilities/VaultConstants.sol'; +import {IExecutorRole} from '../../../interfaces/user/roles/IExecutorRole.sol'; +import {AddressUtils} from '../../../libraries/AddressUtils.sol'; +import {SafeMath} from '../../../libraries/SafeMath.sol'; +import '../../../utilities/VaultConstants.sol'; /** * @title Executor Role Contract @@ -30,7 +30,7 @@ abstract contract ExecutorRole is AccessControl, IExecutorRole { * Reverts with `AccessControlUnauthorizedExecutor` if the caller is not the executor. */ modifier onlyExecutor() { - if (!hasRole(EXECUTOR_ROLE, _msgSender())) { + if (!_isExecutor(_msgSender())) { revert AccessControlUnauthorizedExecutor(_msgSender()); } _; @@ -71,7 +71,7 @@ abstract contract ExecutorRole is AccessControl, IExecutorRole { } _validateExecutorAddress(newExecutor); - grantRole(EXECUTOR_ROLE, newExecutor); + _grantRole(EXECUTOR_ROLE, newExecutor); _executor = newExecutor; emit ExecutorAdded(newExecutor); @@ -84,7 +84,7 @@ abstract contract ExecutorRole is AccessControl, IExecutorRole { function _removeExecutor() internal onlyRole(OWNER_ROLE) { address oldExecutor = _executor; oldExecutor.requireValidUserAddress(); - revokeRole(EXECUTOR_ROLE, oldExecutor); + _revokeRole(EXECUTOR_ROLE, oldExecutor); _executor = address(0); emit ExecutorRemoved(oldExecutor); } @@ -101,8 +101,8 @@ abstract contract ExecutorRole is AccessControl, IExecutorRole { oldExecutor.requireValidUserAddress(); _validateExecutorAddress(newExecutor); - revokeRole(EXECUTOR_ROLE, oldExecutor); - grantRole(EXECUTOR_ROLE, newExecutor); + _revokeRole(EXECUTOR_ROLE, oldExecutor); + _grantRole(EXECUTOR_ROLE, newExecutor); _executor = newExecutor; @@ -147,12 +147,33 @@ abstract contract ExecutorRole is AccessControl, IExecutorRole { emit OwnerOverrideApproved(newOwner, executionTimestamp); } + /** + * @notice Checks if an address is the contract executor. + * @param account The address to check. + * @return status True if the address is the contract executor, otherwise false. + */ + function _isExecutor( + address account + ) internal view returns (bool status) { + status = account == _executor && hasRole(EXECUTOR_ROLE, account); + } + + /** + * @dev Validates the provided executor address. + * + * This function checks if the given executor address is a valid user address + * and ensures that the address does not have the OWNER_ROLE or SIGNER_ROLE. + * If the address has either of these roles, the function reverts with an + * `InvalidExecutorUser` error. + * + * @param executor_ The address of the executor to validate. + */ function _validateExecutorAddress( - address newExecutor + address executor_ ) private view { - newExecutor.requireValidUserAddress(); - if (hasRole(OWNER_ROLE, newExecutor) || hasRole(SIGNER_ROLE, newExecutor)) { - revert InvalidExecutorUser(newExecutor); + executor_.requireValidUserAddress(); + if (hasRole(OWNER_ROLE, executor_) || hasRole(SIGNER_ROLE, executor_)) { + revert InvalidExecutorUser(executor_); } } } diff --git a/src/components/roles/OwnerRole.sol b/src/components/user/roles/OwnerRole.sol similarity index 50% rename from src/components/roles/OwnerRole.sol rename to src/components/user/roles/OwnerRole.sol index d06ca60..ff9ff20 100644 --- a/src/components/roles/OwnerRole.sol +++ b/src/components/user/roles/OwnerRole.sol @@ -2,8 +2,9 @@ pragma solidity ^0.8.27; import {AccessControl} from '@openzeppelin/contracts/access/AccessControl.sol'; -import {IOwnerRole} from '../../interfaces/roles/IOwnerRole.sol'; -import {OWNER_ROLE} from '../../utilities/VaultConstants.sol'; +import {IOwnerRole} from '../../../interfaces/user/roles/IOwnerRole.sol'; +import {AddressUtils} from '../../../libraries/AddressUtils.sol'; +import {OWNER_ROLE} from '../../../utilities/VaultConstants.sol'; /** * @title Owner Role Contract @@ -19,25 +20,26 @@ abstract contract OwnerRole is AccessControl, IOwnerRole { /** * @dev Initializes the Owner role and sets the initial owner override timelock. - * @param _ownerAddress The address of the initial owner. - * @param _initialOwnerOverrideLimit The initial timelock value for owner override. + * @param owner_ The address of the initial owner. + * @param initialOwnerOverrideTimelock The initial timelock value for owner override. */ - constructor(address _ownerAddress, uint256 _initialOwnerOverrideLimit) { - if (_initialOwnerOverrideLimit <= 0) { - revert InvalidOwnerOverrideLimitValue(_initialOwnerOverrideLimit); + constructor(address owner_, uint256 initialOwnerOverrideTimelock) { + AddressUtils.requireValidUserAddress(owner_); + if (initialOwnerOverrideTimelock <= 0) { + revert InvalidOwnerOverrideTimelockValue(initialOwnerOverrideTimelock); } // Grant DEFAULT_ADMIN_ROLE to the owner - _grantRole(DEFAULT_ADMIN_ROLE, _ownerAddress); + _grantRole(DEFAULT_ADMIN_ROLE, owner_); // 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, _ownerAddress); + _grantRole(OWNER_ROLE, owner_); - ownerOverrideTimelock = _initialOwnerOverrideLimit; - _owner = _ownerAddress; + ownerOverrideTimelock = initialOwnerOverrideTimelock; + _owner = owner_; } /** @@ -45,7 +47,7 @@ abstract contract OwnerRole is AccessControl, IOwnerRole { * Reverts with `AccessControlUnauthorizedOwner` if the caller is not the owner. */ modifier onlyOwner() { - if (!hasRole(OWNER_ROLE, _msgSender())) { + if (!_isOwner(_msgSender())) { revert AccessControlUnauthorizedOwner(_msgSender()); } _; @@ -63,32 +65,45 @@ abstract contract OwnerRole is AccessControl, IOwnerRole { * @notice Increases the owner override timelock to a new limit. * Emits the `OwnerOverrideTimelockIncreased` event. * - * @param newLimit The new timelock value for the owner override. + * @param newOwnerTimelock The new timelock value for the owner override. * @dev - * - `newLimit` must be higher than the current value. + * - `newOwnerTimelock` must be higher than the current value. */ - function increaseOwnerOverrideTimelockLimit( - uint256 newLimit + function increaseOwnerOverrideTimelock( + uint256 newOwnerTimelock ) public onlyRole(OWNER_ROLE) { - require(newLimit > ownerOverrideTimelock, 'OwnerRole: New limit must be higher'); - ownerOverrideTimelock = newLimit; - emit OwnerOverrideTimelockIncreased(newLimit); + if (newOwnerTimelock <= ownerOverrideTimelock) revert InvalidOwnerOverrideTimelockValue(newOwnerTimelock); + + ownerOverrideTimelock = newOwnerTimelock; + emit OwnerOverrideTimelockIncreased(newOwnerTimelock); } /** * @notice Decreases the owner override timelock to a new limit. * Emits the `OwnerOverrideTimelockDecreased` event. * - * @param newLimit The new timelock value for the owner override. + * @param newOwnerTimelock The new timelock value for the owner override. * @dev - * - `newLimit` must be lower than the current value. + * - `newOwnerTimelock` must be lower than the current value. */ - function decreaseOwnerOverrideTimelockLimit( - uint256 newLimit + function decreaseOwnerOverrideTimelock( + uint256 newOwnerTimelock ) public onlyRole(OWNER_ROLE) { - require(newLimit < ownerOverrideTimelock, 'OwnerRole: New limit must be lower'); - ownerOverrideTimelock = newLimit; - emit OwnerOverrideTimelockDecreased(newLimit); + if (newOwnerTimelock >= ownerOverrideTimelock) revert InvalidOwnerOverrideTimelockValue(newOwnerTimelock); + + ownerOverrideTimelock = newOwnerTimelock; + emit OwnerOverrideTimelockDecreased(newOwnerTimelock); + } + + /** + * @notice Checks if an address is the contract owner. + * @param account The address to check. + * @return status True if the address is the contract owner, otherwise false. + */ + function _isOwner( + address account + ) internal view returns (bool status) { + status = account == _owner && hasRole(OWNER_ROLE, account); } /** diff --git a/src/components/user/roles/SignerRole.sol b/src/components/user/roles/SignerRole.sol new file mode 100644 index 0000000..fe4fc4e --- /dev/null +++ b/src/components/user/roles/SignerRole.sol @@ -0,0 +1,127 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity ^0.8.27; + +import {EnumerableSet} from '@openzeppelin/contracts/utils/structs/EnumerableSet.sol'; +import {AccessControl} from '@openzeppelin/contracts/access/AccessControl.sol'; +import {ISignerRole} from '../../../interfaces/user/roles/ISignerRole.sol'; +import {AddressUtils} from '../../../libraries/AddressUtils.sol'; +import '../../../utilities/VaultConstants.sol'; +import '../../../libraries/Counters.sol'; + +/** + * @title Signer Role Contract + * @author Emmanuel Joseph (JET) + * @dev Abstract contract providing the logic for the Signer role in the MultiSigVault system. + */ +abstract contract SignerRole is AccessControl, ISignerRole { + using EnumerableSet for EnumerableSet.AddressSet; + using Counters for Counters.Counter; + using AddressUtils for address; + + /// @notice Using Counter library for total signers + Counters.Counter private _signerCount; + + /// @dev Using EnumerableSet library for signer addresses. + EnumerableSet.AddressSet private _signers; + + /** + * @dev Modifier to restrict access to functions to only the signer. + * Reverts with `AccessControlUnauthorizedSigner` if the caller is not a signer. + */ + modifier onlySigner() { + if (!isSigner(_msgSender())) { + revert AccessControlUnauthorizedSigner(_msgSender()); + } + _; + } + + /** + * @dev Modifier to restrict access to functions to valid executors. + * Reverts with `UnauthorizedMultiSigExecutor` if the caller is not an authorized multi-sig executor. + */ + modifier validExecutor() { + if (!hasRole(EXECUTOR_ROLE, _msgSender()) && !hasRole(OWNER_ROLE, _msgSender())) { + revert UnauthorizedMultiSigExecutor(_msgSender()); + } + _; + } + + /** + * @notice Checks if an address is a signer. + * @param signer The address to check. + * @return status True if the address is a signer, otherwise false. + */ + function isSigner( + address signer + ) public view returns (bool status) { + status = _signers.contains(signer) && hasRole(SIGNER_ROLE, signer); + } + + /** + * @notice Returns the total number of signers. + * @return uint256 The number of signers in the contract. + */ + function totalSigners() public view returns (uint256) { + return _signerCount.current(); + } + + /** + * @notice Returns an array of all current signers' addresses. + * @return address[] The list of signers' addresses. + */ + function getSigners() public view returns (address[] memory) { + return _signers.values(); + } + + /** + * @notice Adds a new signer. + * @param newSigner The address of the new signer. + * @dev Only callable by a valid executor. + */ + function _addSigner( + address newSigner + ) internal virtual validExecutor { + _validateSignerAddress(newSigner); + if (isSigner(newSigner)) revert SignerAlreadyExists(newSigner); + + _grantRole(SIGNER_ROLE, newSigner); + _signers.add(newSigner); + _signerCount.increment(); + emit SignerAdded(newSigner); + } + + /** + * @notice Removes the current signer. + * @param signer The address of the signer to be removed. + * @dev Only callable by a valid executor. + */ + function _removeSigner( + address signer + ) internal virtual validExecutor { + if (!isSigner(signer)) revert SignerDoesNotExist(signer); + + _revokeRole(SIGNER_ROLE, signer); + _signers.remove(signer); + _signerCount.decrement(); + emit SignerRemoved(signer); + } + + /** + * @dev Validates the provided signer address. + * + * This function checks if the given signer address is a valid user address + * and ensures that the address does not have the OWNER_ROLE or SIGNER_ROLE. + * If the address has either of these roles, the function reverts with an + * `InvalidSignerUser` error. + * + * @param signer The address of the signer to validate. + */ + function _validateSignerAddress( + address signer + ) private view { + signer.requireValidUserAddress(); + if (hasRole(OWNER_ROLE, signer) || hasRole(EXECUTOR_ROLE, signer)) { + revert InvalidSignerUser(signer); + } + } +} diff --git a/src/interfaces/IMultiSigEnterpriseVault.sol b/src/interfaces/IMultiSigEnterpriseVault.sol deleted file mode 100644 index bcfd76b..0000000 --- a/src/interfaces/IMultiSigEnterpriseVault.sol +++ /dev/null @@ -1,20 +0,0 @@ -// SPDX-License-Identifier: GPL-3.0 -pragma solidity ^0.8.27; - -/** - * @title IMultiSigEnterpriseVault Interface - * @author Emmanuel Joseph (JET) - * @dev Interface defining errors, events and external functions for the MultiSig Enterprise Vault contract. - */ -interface IMultiSigEnterpriseVault { - /** - * @dev Error thrown when an signers approval is required to perform an action. - */ - error SignersApprovalRequired(); - - /** - * @dev Event emitted when the signatory threshold is updated. - * @param newThreshold The new threshold value for signatory approval. - */ - event ThresholdUpdated(uint256 newThreshold); -} diff --git a/src/interfaces/IMultiSigTimelock.sol b/src/interfaces/IMultiSigTimelock.sol new file mode 100644 index 0000000..019fb59 --- /dev/null +++ b/src/interfaces/IMultiSigTimelock.sol @@ -0,0 +1,163 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity ^0.8.27; + +import {ActionType} from '../utilities/VaultEnums.sol'; + +/** + * @title IMultiSigTimelock Interface + * @dev Interface defining errors and events for timelock management. + */ +interface IMultiSigTimelock { + /** + * @dev Error thrown when the provided action ID is not valid. + * @param actionId The ID of the invalid action. + */ + error InvalidAction(uint256 actionId); + + /** + * @dev Error thrown when the provided action type is not valid. + * @param action The type of the invalid action. + */ + error InvalidActionType(ActionType action); + + /** + * @dev Error indicating the state of a pending action. + * @param isPending Boolean value representing whether the latest action is pending. + */ + error PendingActionState(bool isPending); + + /** + * @dev Error thrown when a action has already been executed. + * @param actionId The ID of the action that was already executed. + */ + error ActionAlreadyExecuted(uint256 actionId); + + /** + * @dev Error thrown when the required number of approvals has not been met. + * @param actionId The ID of the not approved action. + */ + error ActionNotApproved(uint256 actionId); + + /** + * @dev Error thrown when a timelock period has not yet elapsed. + * @param requiredTimestamp The timestamp at which the timelock will be met. + * @param currentTimestamp The current timestamp. + */ + error TimelockNotElapsed(uint256 requiredTimestamp, uint256 currentTimestamp); + + /** + * @dev Error thrown when the total number of signers is below the required signatory threshold. + * @param requiredThreshold The required number of signers. + * @param currentSigners The current number of signers. + */ + error InsufficientSigners(uint256 requiredThreshold, uint256 currentSigners); + + /** + * @dev Error thrown when an invalid signatory threshold value is given. + * @param signatoryThreshold The invalid signatory threshold value. + */ + error InvalidSignatoryThreshold(uint256 signatoryThreshold); + + /** + * @dev Error thrown when an invalid timelock value is used during initialization. + * @param timelockValue The invalid timelock value. + */ + error InvalidMultiSigTimelockValue(uint256 timelockValue); + + /** + * @dev Error thrown when trying to increase the timelock to an invalid value. + * @param newLimit The invalid increase timelock. + */ + error InvalidMultiSigTimelockIncrease(uint256 newLimit); + + /** + * @dev Error thrown when trying to decrease the timelock to an invalid value. + * @param newLimit The invalid decrease timelock. + */ + error InvalidMultiSigTimelockDecrease(uint256 newLimit); + + /** + * @dev Error thrown when trying to increase the signatory threshold to an invalid value. + * @param newThreshold The invalid increase signatory threshold. + */ + error InvalidSignatoryThresholdIncrease(uint256 newThreshold); + + /** + * @dev Error thrown when trying to decrease the signatory threshold to an invalid value. + * @param newThreshold The invalid decrease signatory threshold. + */ + error InvalidSignatoryThresholdDecrease(uint256 newThreshold); + + /** + * @dev Error thrown when an signers approval is required to perform an action. + */ + error SignersApprovalRequired(); + + /** + * @dev Error thrown when insufficient signer approvals have been met. + * @param requiredApprovals The number of approvals required. + * @param currentApprovals The current number of approvals. + */ + error InsufficientSignerApprovals(uint256 requiredApprovals, uint256 currentApprovals); + + /** + * @dev Event emitted when a new action is initiated. + * @param actionId The unique ID of the initiated action. + * @param initiator The address of the action initiator. + * @param actionType The type of action being initiated. + */ + event ActionInitiated(uint256 indexed actionId, address indexed initiator, ActionType actionType); + + /** + * @dev Event emitted when an action is approved. + * @param actionId The ID of the approved action. + * @param approver The address of the account who approved the action. + * @param timestamp The timestamp (in seconds) when the action was approved. + */ + event ActionApproved(uint256 indexed actionId, address indexed approver, uint256 timestamp); + + /** + * @dev Event emitted when an action approval is revoked. + * @param actionId The ID of the action. + * @param revoker The address of the account who revoked approval. + * @param timestamp The timestamp (in seconds) when the action was revoked. + */ + event ActionRevoked(uint256 indexed actionId, address indexed revoker, uint256 timestamp); + + /** + * @dev Event emitted when an action is executed. + * @param actionId The ID of the executed action. + * @param actionType The type of action executed. + * @param executor The address of the account who executed the action. + * @param timestamp The timestamp (in seconds) when the action was executed. + */ + event ActionExecuted( + uint256 indexed actionId, ActionType indexed actionType, address indexed executor, uint256 timestamp + ); + + /** + * @dev Event emitted when the signatory threshold is updated. + * @param oldThreshold The old threshold value for signatory approval. + * @param newThreshold The new threshold value for signatory approval. + */ + event ThresholdUpdated(uint256 indexed oldThreshold, uint256 indexed newThreshold); + + /** + * @dev Event emitted when the action timelock is updated. + * @param oldLimit The old action timelock value. + * @param newLimit The new action timelock value. + */ + event MultiSigTimelockUpdated(uint256 oldLimit, uint256 newLimit); + + /** + * @dev Returns the current timelock for the vault. + * @return The current timelock. + */ + function multiSigTimelock() external view returns (uint256); + + /** + * @dev Returns the current signatory threshold for the vault. + * @return The current signatory threshold. + */ + function signatoryThreshold() external view returns (uint256); +} diff --git a/src/interfaces/IMultiSigTransaction.sol b/src/interfaces/IMultiSigTransaction.sol index bf6d8ac..195c955 100644 --- a/src/interfaces/IMultiSigTransaction.sol +++ b/src/interfaces/IMultiSigTransaction.sol @@ -12,6 +12,12 @@ interface IMultiSigTransaction { */ error InvalidTransaction(uint256 transactionId); + /** + * @dev Error indicating the state of a pending transaction. + * @param isPending Boolean value representing whether the latest transaction is pending. + */ + error PendingTransactionState(bool isPending); + /** * @dev Error thrown when there are insufficient tokens to complete a transaction. * @param availableBalance The available vault token balance. @@ -19,6 +25,14 @@ interface IMultiSigTransaction { */ error InsufficientTokenBalance(uint256 availableBalance, uint256 requestedValue); + /** + * @dev Indicates a failure with the `spender`’s `allowance`. Used in transfers. + * @param owner The address whose tokens are being transferred. + * @param allowance Amount of tokens a `spender` is allowed to operate with. + * @param needed The remaining allowance needed to complete the transfer. + */ + error ERC20InsufficientAllowance(address owner, uint256 allowance, uint256 needed); + /** * @dev Error thrown when a transaction has already been executed. * @param transactionId The ID of the transaction that was already executed. @@ -50,12 +64,12 @@ interface IMultiSigTransaction { * @dev Event emitted when a transaction is initiated. * @param transactionId The ID of the initiated transaction. * @param initiator The address of the initiator. - * @param target The address to receive the value. + * @param to The address to receive the value. * @param token The token contract address (0x0 for ETH). * @param value The value to be transferred. */ event TransactionInitiated( - uint256 indexed transactionId, address indexed initiator, address indexed target, address token, uint256 value + uint256 indexed transactionId, address indexed initiator, address indexed to, address token, uint256 value ); /** @@ -77,7 +91,7 @@ interface IMultiSigTransaction { /** * @dev Event emitted when a transaction is executed. * @param transactionId The ID of the executed transaction. - * @param executor The address of the signer who executed the transaction. + * @param executor The address of the account who executed the transaction. * @param timestamp The timestamp (in seconds) when the transaction was executed. */ event TransactionExecuted(uint256 indexed transactionId, address indexed executor, uint256 timestamp); @@ -85,16 +99,10 @@ interface IMultiSigTransaction { /** * @notice Receives ERC20 tokens and emits `FundsReceived` event * @param token The ERC20 token address. - * @param amount The amount of tokens sent. + * @param amount The amount of ERC20 tokens sent. */ function depositToken(address token, uint256 amount) external payable; - /** - * @dev Returns the current signatory threshold for the vault. - * @return The current signatory threshold. - */ - function signatoryThreshold() external view returns (uint256); - /** * @dev Returns the vault's total transactions. * @return The total transaction value. diff --git a/src/interfaces/roles/ISignerRole.sol b/src/interfaces/roles/ISignerRole.sol deleted file mode 100644 index f1725dc..0000000 --- a/src/interfaces/roles/ISignerRole.sol +++ /dev/null @@ -1,27 +0,0 @@ -// SPDX-License-Identifier: GPL-3.0 -pragma solidity ^0.8.27; - -/** - * @title ISignerRole Interface - * @author Emmanuel Joseph (JET) - * @dev Interface defining errors and events related to the Signer role in the MultiSigVault system. - */ -interface ISignerRole { - /** - * @dev Error thrown when an unauthorized account attempts to perform an signer action. - * @param account The address of the unauthorized account. - */ - error AccessControlUnauthorizedSigner(address account); - - /** - * @dev Event emitted when a new signer is added. - * @param signer The new signer address. - */ - event SignerAdded(address indexed signer); - - /** - * @dev Event emitted when an signer is removed. - * @param signer The removed signer address. - */ - event SignerRemoved(address indexed signer); -} diff --git a/src/interfaces/IUser.sol b/src/interfaces/user/IUser.sol similarity index 67% rename from src/interfaces/IUser.sol rename to src/interfaces/user/IUser.sol index 70cc1c7..2086160 100644 --- a/src/interfaces/IUser.sol +++ b/src/interfaces/user/IUser.sol @@ -17,11 +17,5 @@ interface IUser { * @dev Error thrown when an unauthorized account attempts to sign a transaction. * @param account The address of the unauthorized account. */ - error UnauthorizedTransactionSigner(address account); - - /** - * @dev Error thrown when an unauthorized account attempts to execute a transaction. - * @param account The address of the unauthorized account. - */ - error UnauthorizedTransactionExecutor(address account); + error UnauthorizedMultiSigSigner(address account); } diff --git a/src/interfaces/roles/IExecutorRole.sol b/src/interfaces/user/roles/IExecutorRole.sol similarity index 91% rename from src/interfaces/roles/IExecutorRole.sol rename to src/interfaces/user/roles/IExecutorRole.sol index f3cb29f..715ee91 100644 --- a/src/interfaces/roles/IExecutorRole.sol +++ b/src/interfaces/user/roles/IExecutorRole.sol @@ -15,8 +15,14 @@ interface IExecutorRole { /** * @dev Error thrown when attempting to add a new executor with an existing user role. + * @param executor The invalid executor address. */ - error InvalidExecutorUser(address account); + error InvalidExecutorUser(address executor); + + /** + * @dev Error thrown when the contract does not have a valid executor. + */ + error MissingExecutor(); /** * @dev Error thrown when attempting to add a new executor when one exists. diff --git a/src/interfaces/roles/IOwnerRole.sol b/src/interfaces/user/roles/IOwnerRole.sol similarity index 92% rename from src/interfaces/roles/IOwnerRole.sol rename to src/interfaces/user/roles/IOwnerRole.sol index c3bee8f..33d5ebe 100644 --- a/src/interfaces/roles/IOwnerRole.sol +++ b/src/interfaces/user/roles/IOwnerRole.sol @@ -9,9 +9,9 @@ pragma solidity ^0.8.27; interface IOwnerRole { /** * @dev Error thrown when an invalid owner override limit is provided. - * @param limit The invalid limit value. + * @param value The invalid timelock value. */ - error InvalidOwnerOverrideLimitValue(uint256 limit); + error InvalidOwnerOverrideTimelockValue(uint256 value); /** * @dev Error thrown when an unauthorized account attempts to perform an owner action. diff --git a/src/interfaces/user/roles/ISignerRole.sol b/src/interfaces/user/roles/ISignerRole.sol new file mode 100644 index 0000000..45be85e --- /dev/null +++ b/src/interfaces/user/roles/ISignerRole.sol @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity ^0.8.27; + +/** + * @title ISignerRole Interface + * @author Emmanuel Joseph (JET) + * @dev Interface defining errors and events related to the Signer role in the MultiSigVault system. + */ +interface ISignerRole { + /** + * @dev Error thrown when an unauthorized account attempts to perform an signer action. + * @param account The address of the unauthorized account. + */ + error AccessControlUnauthorizedSigner(address account); + + /** + * @dev Error thrown when attempting to add a new signer with an existing user role. + * @param signer The invalid signer address. + */ + error InvalidSignerUser(address signer); + + /** + * @dev Error thrown when an unauthorized account attempts to execute a transaction. + * @param account The address of the unauthorized account. + */ + error UnauthorizedMultiSigExecutor(address account); + + /** + * @dev Error thrown when trying to add a signer that already exists. + * @param newSigner The address of the signer that already exists. + */ + error SignerAlreadyExists(address newSigner); + + /** + * @dev Error indicating that the specified signer does not exist. + * @param signer The address of the signer that does not exist. + */ + error SignerDoesNotExist(address signer); + + /** + * @dev Event emitted when a new signer is added. + * @param signer The new signer address. + */ + event SignerAdded(address indexed signer); + + /** + * @dev Event emitted when an signer is removed. + * @param signer The removed signer address. + */ + event SignerRemoved(address indexed signer); +} diff --git a/src/libraries/AddressUtils.sol b/src/libraries/AddressUtils.sol index 9543d9c..b93c99c 100644 --- a/src/libraries/AddressUtils.sol +++ b/src/libraries/AddressUtils.sol @@ -17,16 +17,10 @@ library AddressUtils { error InvalidUserAddress(address account); /** - * @dev Error thrown when an invalid token address is provided. - * @param token The invalid token address. + * @dev Error thrown when an invalid transaction receiver is provided. + * @param receiver The invalid receiver address. */ - error InvalidTokenAddress(address token); - - /** - * @dev Error thrown when an invalid transaction target is provided. - * @param target The invalid target address. - */ - error InvalidTransactionTarget(address target); + error InvalidTransactionReceiver(address receiver); /** * @notice Ensure a user address is valid (i.e., not a zero address). @@ -41,26 +35,14 @@ library AddressUtils { } /** - * @notice Ensure a token address is valid. - * @param token The token address to validate. - */ - function requireValidTokenAddress( - address token - ) internal pure { - if (token == address(0)) { - revert InvalidTokenAddress(token); - } - } - - /** - * @notice Ensure a transaction target is valid. - * @param target The transaction target to validate. + * @notice Ensure a transaction receiver is valid. + * @param receiver The transaction receiver to validate. */ - function requireValidTransactionTarget( - address payable target + function requireValidTransactionReceiver( + address payable receiver ) internal pure { - if (target == address(0)) { - revert InvalidTransactionTarget(target); + if (receiver == address(0)) { + revert InvalidTransactionReceiver(receiver); } } } diff --git a/src/libraries/ArraysUtils.sol b/src/libraries/ArraysUtils.sol deleted file mode 100644 index 90d85ac..0000000 --- a/src/libraries/ArraysUtils.sol +++ /dev/null @@ -1,76 +0,0 @@ -// SPDX-License-Identifier: GPL-3.0 -pragma solidity ^0.8.27; - -import '@openzeppelin/contracts/utils/Arrays.sol'; - -/** - * @title ArraysUtils Library - * @dev Extends OpenZeppelin's Arrays library to add custom utility functions. - */ -library ArraysUtils { - /** - * @dev Error thrown when the provided index is not found in an array. - * @param index The invalid array index - */ - error ArrayIndexOutOfBound(uint256 index); - - /** - * @dev Finds the index of a 'uint256' array element. - * @param element The array element to find - * @param array The array to lookup - * @return index The index of the found element - */ - function arrayElementIndexLookup(uint256 element, uint256[] memory array) internal pure returns (uint256 index) { - index = 0; - while (array[index] != element) { - index++; - } - } - - /** - * @dev Finds the index of a 'address' array element. - * @param element The array element to find - * @param array The array to lookup - * @return index The index of the found element - */ - function arrayElementIndexLookup(address element, address[] memory array) internal pure returns (uint256 index) { - index = 0; - while (array[index] != element) { - index++; - } - } - - /** - * @dev Removes an element by index from a 'uint256' array. - * @param index The array index of the element - * @param array The array to lookup for removal - * - * Requirements: - * - Ensures the `index` is available in the given array - */ - function removeElementFromArray(uint256 index, uint256[] storage array) internal { - if (index > array.length) revert ArrayIndexOutOfBound(index); - for (uint256 i = index; i < array.length - 1; i++) { - array[i] = array[i + 1]; - } - - array.pop(); - } - - /** - * @dev Removes an element by index from a 'address' array. - * @param index The array index of the element - * @param array The array to lookup for removal - * - * Requirements: - * - Ensures the `index` is available in the given array - */ - function removeElementFromArray(uint256 index, address[] storage array) internal { - if (index > array.length) revert ArrayIndexOutOfBound(index); - for (uint256 i = index; i < array.length - 1; i++) { - array[i] = array[i + 1]; - } - - array.pop(); - } -} diff --git a/src/libraries/ERC20Validator.sol b/src/libraries/ERC20Validator.sol new file mode 100644 index 0000000..6f9e830 --- /dev/null +++ b/src/libraries/ERC20Validator.sol @@ -0,0 +1,61 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity ^0.8.27; + +import {IERC20} from '@openzeppelin/contracts/token/ERC20/IERC20.sol'; + +/** + * @title ERC20Validator Library + * @author Emmanuel Joseph (JET) + * @dev Collection of functions related to address type for validating ERC20 tokens. + */ +library ERC20Validator { + /** + * @dev Error thrown when an invalid ERC20 token address is provided. + * @param token The invalid token address. + */ + error InvalidERC20TokenAddress(address token); + + /** + * @notice Checks if the given address is a contract. + * @param account The address to check. + * @return bool True if the address is a contract, false otherwise. + */ + function isContract( + address account + ) internal view returns (bool) { + uint256 size; + assembly { + size := extcodesize(account) + } + return size > 0; + } + + /** + * @notice Checks if a given address is an ERC20 token contract. + * @param token The address of the token contract to check. + * @dev This function verifies if the provided address is a contract and attempts to call + * several ERC20 functions to ensure compliance with the ERC20 standard. + * If any of these calls fail, the function reverts with an error. + */ + function requireValidERC20Token( + address token + ) internal { + if (!isContract(token)) { + revert InvalidERC20TokenAddress(token); + } + + // Try to call ERC20 functions and check for success + try IERC20(token).balanceOf(address(this)) returns (uint256) {} + catch { + revert InvalidERC20TokenAddress(token); + } + try IERC20(token).transfer(address(this), 0) returns (bool) {} + catch { + revert InvalidERC20TokenAddress(token); + } + try IERC20(token).transferFrom(address(this), address(this), 0) returns (bool) {} + catch { + revert InvalidERC20TokenAddress(token); + } + } +} diff --git a/src/utilities/VaultEnums.sol b/src/utilities/VaultEnums.sol index 7c27385..56756d2 100644 --- a/src/utilities/VaultEnums.sol +++ b/src/utilities/VaultEnums.sol @@ -7,7 +7,7 @@ pragma solidity ^0.8.27; * @notice RoleType helps differentiate between Owner, Executor, and Signer roles. * * @dev Enum representing the different roles in the MultiSig Vault contract. - * - INVALID 0: Represents an invalid role + * - INVALID 0: Represents an invalid role for security purpose * - OWNER 1: Represents the Owner role * - EXECUTOR 2: Represents the Executor role * - SIGNER 3: Represents the Signer role @@ -18,3 +18,27 @@ enum RoleType { EXECUTOR, SIGNER } + +/** + * @title Vault ActionType Enum + * @author Emmanuel Joseph (JET) + * @notice ActionType helps differentiate between types of actions that require approval. + * + * @dev Enum representing the different signatory actions in the MultiSig Vault contract. + * - INVALID 0: Represents an invalid action for security purpose + * - ADD_SIGNER 1: Represents the add signer action + * - REMOVE_SIGNER 2: Represents the remove signer action + * - INCREASE_TIMELOCK 3: Represents the increase timelock action + * - DECREASE_TIMELOCK 4: Represents the decrease timelock action + * - INCREASE_THRESHOLD 5: Represents the increase threshold action + * - DECREASE_THRESHOLD 6: Represents the decrease threshold action + */ +enum ActionType { + INVALID, + ADD_SIGNER, + REMOVE_SIGNER, + INCREASE_TIMELOCK, + DECREASE_TIMELOCK, + INCREASE_THRESHOLD, + DECREASE_THRESHOLD +} diff --git a/src/utilities/VaultStructs.sol b/src/utilities/VaultStructs.sol index 1e5451d..5500ba9 100644 --- a/src/utilities/VaultStructs.sol +++ b/src/utilities/VaultStructs.sol @@ -1,10 +1,12 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.27; +import '@openzeppelin/contracts/utils/structs/EnumerableSet.sol'; import '../libraries/Counters.sol'; -import {RoleType} from './VaultEnums.sol'; +import './VaultEnums.sol'; using Counters for Counters.Counter; +using EnumerableSet for EnumerableSet.AddressSet; /** * @title UserProfile Struct @@ -29,23 +31,52 @@ struct UserProfile { * * @dev * - `initiator`: The initiator of the transaction (Owner or Signer). - * - `target`: The address to receive the transaction tokens. + * - `to`: The address receiving the transaction tokens. * - `token`: The token contract address (0x0 for ETH). * - `value`: The ETH or token value to send. * - `approvals`: The total number of approvals * - `signatures`: The addresses of those who approved/signed the transaction * - `isExecuted`: Checks whether the transaction has been executed. - * - `isOverride`: Checks whether the transaction was override by the `executor`. + * - `isOverride`: Checks whether the transaction was overridden by the `executor`. * - `data`: The transaction data (0x0 for empty data) */ struct Transaction { address initiator; - address payable target; + address payable to; address token; uint256 value; + uint256 timestamp; Counters.Counter approvals; - address[] signatures; + EnumerableSet.AddressSet signatures; bool isExecuted; bool isOverride; bytes data; } + +/** + * @title Action Struct + * @author Emmanuel Joseph (JET) + * @notice This struct stores information about actions that require approval. + * + * @dev + * - `initiator`: The initiator of the action (Owner or Signer). + * - `actionType`: The type of action being proposed. + * - `target`: The target address (for signer-related actions). + * - `value`: The value related to the action (e.g., timelock or threshold values). + * - `timestamp`: The time when the action was initiated. + * - `approvals`: The total number of approvals + * - `signatures`: The addresses of those who approved/signed the action + * - `isExecuted`: Checks whether the action has been executed. + * - `isOverride`: Checks whether the action was overridden by the `executor`. + */ +struct Action { + address initiator; + ActionType actionType; + address target; + uint256 value; + uint256 timestamp; + Counters.Counter approvals; + EnumerableSet.AddressSet signatures; + bool isExecuted; + bool isOverride; +} diff --git a/test/MultiSigEnterpriseVault.t.sol b/test/MultiSigEnterpriseVault.t.sol index 432df9c..721f53b 100644 --- a/test/MultiSigEnterpriseVault.t.sol +++ b/test/MultiSigEnterpriseVault.t.sol @@ -11,14 +11,18 @@ contract MultiSigEnterpriseVaultTest is Test { address internal vaultAddress; address internal vaultDeployer; uint256 internal initialThreshold; - uint256 internal initialOwnerOverrideLimit; + uint256 internal initialMultiSigTimelock; + uint256 internal initialOwnerOverrideTimelock; function setUp() public virtual { initialThreshold = 3; - initialOwnerOverrideLimit = 3 days; + initialMultiSigTimelock = 1 days; + initialOwnerOverrideTimelock = 3 days; vaultOwner = makeAddr('vaultOwner'); + vm.deal(vaultOwner, 100 ether); - vault = new MultiSigEnterpriseVault(vaultOwner, initialThreshold, initialOwnerOverrideLimit); + vault = + new MultiSigEnterpriseVault(vaultOwner, initialThreshold, initialMultiSigTimelock, initialOwnerOverrideTimelock); vaultAddress = address(vault); vaultDeployer = msg.sender; } diff --git a/test/components/BaseMultiSigTest.t.sol b/test/components/BaseMultiSigTest.t.sol index 17525b1..84c8c6e 100644 --- a/test/components/BaseMultiSigTest.t.sol +++ b/test/components/BaseMultiSigTest.t.sol @@ -25,8 +25,16 @@ contract BaseMultiSigTest is MultiSigEnterpriseVaultTest { vault.addSigner(secondSigner); } + function testInitialValues() public view { + assertEq(vault.owner(), vaultOwner); + assertEq(vault.signatoryThreshold(), initialThreshold); + assertEq(vault.multiSigTimelock(), initialMultiSigTimelock); + assertEq(vault.ownerOverrideTimelock(), initialOwnerOverrideTimelock); + } + function testTotalUsers() public { vm.prank(vaultOwner); assertEq(vault.totalUsers(), 4); + assertEq(vault.totalSigners(), 2); } } diff --git a/test/components/MultiSigFuzzTest.t.sol b/test/components/MultiSigFuzzTest.t.sol index 0472168..a854b56 100644 --- a/test/components/MultiSigFuzzTest.t.sol +++ b/test/components/MultiSigFuzzTest.t.sol @@ -1,37 +1,26 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.27; -import '../MultiSigEnterpriseVault.t.sol'; - -contract MultiSigFuzzTest is MultiSigEnterpriseVaultTest { - function testFuzzRemoveSigner( - address signer - ) public { - vm.assume(signer != address(0)); - - vm.prank(vaultOwner); - vault.addSigner(signer); - assertTrue(vault.isSigner(signer)); - - vm.prank(vaultOwner); - vault.removeSigner(signer); - assertFalse(vault.isSigner(signer)); - } +import {IERC20} from '@openzeppelin/contracts/token/ERC20/IERC20.sol'; +import {Address} from '@openzeppelin/contracts/utils/Address.sol'; +import {ActionType} from '../../src/utilities/VaultEnums.sol'; +import {MockERC20Token} from '../mocks/MockERC20Token.sol'; +import './BaseMultiSigTest.t.sol'; +contract MultiSigFuzzTest is BaseMultiSigTest { function testFuzzUpdateExecutor( address newExecutor ) public { - vm.assume(newExecutor != address(0)); - vm.assume(newExecutor != vaultOwner); + vm.assume( + newExecutor != address(0) && newExecutor != vaultOwner && newExecutor != vaultExecutor + && newExecutor != vaultAddress && newExecutor != firstSigner && newExecutor != secondSigner + ); - vm.prank(vaultOwner); - vault.addExecutor(newExecutor); - assertEq(vault.executor(), newExecutor); + assertEq(vault.executor(), vaultExecutor); - address anotherExecutor = makeAddr('randomExecutor'); vm.prank(vaultOwner); - vault.updateExecutor(anotherExecutor); - assertEq(vault.executor(), anotherExecutor); + vault.updateExecutor(newExecutor); + assertEq(vault.executor(), newExecutor); } function testFuzzIncreaseOwnerOverrideTimelock( @@ -40,16 +29,98 @@ contract MultiSigFuzzTest is MultiSigEnterpriseVaultTest { vm.assume(newLimit > vault.ownerOverrideTimelock()); vm.prank(vaultOwner); - vault.increaseOwnerOverrideTimelockLimit(newLimit); + vault.increaseOwnerOverrideTimelock(newLimit); assertEq(vault.ownerOverrideTimelock(), newLimit); } - function testFuzzUpdateThreshold( - uint256 newThreshold + function testFuzzIncreaseTimelock( + uint256 newTimelock ) public { - vm.assume(newThreshold > 0); + vm.assume(newTimelock > vault.multiSigTimelock()); + + vm.prank(firstSigner); + vault.initiateAction(ActionType.INCREASE_TIMELOCK, address(0), newTimelock); + + vm.prank(firstSigner); + vault.approveAction(1); + vm.prank(secondSigner); + vault.approveAction(1); + vm.prank(vaultOwner); + vault.approveAction(1); + + skip(30 hours); + vm.prank(vaultOwner); + vault.executeAction(1); + + assertEq(vault.multiSigTimelock(), newTimelock); + } + + function testFuzzInitiateApproveExecuteETHTransaction(address payable recipient, bytes memory data) public { + vm.assume( + recipient != address(0) && recipient != vaultOwner && recipient != firstSigner && recipient != secondSigner + && recipient != vaultExecutor && recipient != vaultAddress + ); + + vm.deal(vaultOwner, 100 ether); + vm.prank(vaultOwner); + Address.sendValue(payable(vaultAddress), 10 ether); + uint256 initialRecipientBalance = recipient.balance; + uint256 initialVaultBalance = vaultAddress.balance; + + vm.prank(firstSigner); + uint256 txValue = 3 ether; + vault.initiateTransaction(recipient, address(0), txValue, data); + assertEq(vault.totalTransactions(), 1); + + vm.prank(firstSigner); + vault.approveTransaction(1); + vm.prank(secondSigner); + vault.approveTransaction(1); + vm.prank(vaultOwner); + vault.approveTransaction(1); + + skip(36 hours); + + vm.prank(vaultExecutor); + vault.executeTransaction(1); + assertEq(vault.getBalance(), initialVaultBalance - txValue); + assertEq(recipient.balance, initialRecipientBalance + txValue); + } + + function testFuzzInitiateApproveExecuteERC20Transaction(address payable recipient, bytes memory data) public { + vm.assume( + recipient != address(0) && recipient != vaultOwner && recipient != firstSigner && recipient != secondSigner + && recipient != vaultExecutor && recipient != vaultAddress + ); + + IERC20 mockToken = IERC20(address(new MockERC20Token(vaultOwner))); + address mockAddress = address(mockToken); + + vm.prank(vaultOwner); + mockToken.approve(vaultAddress, 300 ether); vm.prank(vaultOwner); - vault.ownerUpdateSignatoryThreshold(newThreshold); - assertEq(vault.signatoryThreshold(), newThreshold); + vault.depositToken(mockAddress, 300 ether); + + uint256 initialRecipientBalance = mockToken.balanceOf(recipient); + uint256 initialVaultBalance = mockToken.balanceOf(vaultAddress); + + vm.prank(firstSigner); + uint256 txValue = 100 ether; + vault.initiateTransaction(recipient, mockAddress, txValue, data); + assertEq(vault.totalTransactions(), 1); + + vm.prank(firstSigner); + vault.approveTransaction(1); + vm.prank(secondSigner); + vault.approveTransaction(1); + vm.prank(vaultOwner); + vault.approveTransaction(1); + + skip(36 hours); + + vm.prank(vaultExecutor); + vault.executeTransaction(1); + assertEq(vault.getTokenBalance(mockAddress), initialVaultBalance - txValue); + assertEq(mockToken.balanceOf(recipient), initialRecipientBalance + txValue); } } diff --git a/test/components/MultiSigTimelockTest.t.sol b/test/components/MultiSigTimelockTest.t.sol new file mode 100644 index 0000000..14f4617 --- /dev/null +++ b/test/components/MultiSigTimelockTest.t.sol @@ -0,0 +1,191 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity ^0.8.27; + +import {ActionType} from '../../src/utilities/VaultEnums.sol'; +import './BaseMultiSigTest.t.sol'; + +contract MultiSigTimelockTest is BaseMultiSigTest { + function testInitiateAndVerifyIncreaseTimelockAction() public { + vm.prank(firstSigner); + uint256 executionTimestamp = block.timestamp; + vault.initiateAction(ActionType.INCREASE_TIMELOCK, address(0), 2 days); + + vm.prank(firstSigner); + assertEq(vault.totalActions(), 1); + + vm.prank(firstSigner); + ( + address initiator, + ActionType actionType, + address target, + uint256 value, + uint256 timestamp, + uint256 approvals, + bool isExecuted, + bool isOverride + ) = vault.getAction(1); + assertEq(initiator, firstSigner); + assertTrue(actionType == ActionType.INCREASE_TIMELOCK); + assertEq(target, address(0)); + assertEq(value, 2 days); + assertEq(timestamp, executionTimestamp); + assertEq(approvals, 0); + assertFalse(isExecuted); + assertFalse(isOverride); + } + + function testInitiateInvalidActions() public { + vm.prank(firstSigner); + vm.expectRevert(); + vault.initiateAction(ActionType.ADD_SIGNER, address(0), 0); + + vm.prank(firstSigner); + vm.expectRevert(); + vault.initiateAction(ActionType.ADD_SIGNER, secondSigner, 0); + + vm.prank(firstSigner); + vm.expectRevert(); + vault.initiateAction(ActionType.REMOVE_SIGNER, makeAddr('newSigner'), 0); + + vm.prank(secondSigner); + vm.expectRevert(); + vault.initiateAction(ActionType.INCREASE_TIMELOCK, address(0), 3 hours); + + vm.prank(secondSigner); + vm.expectRevert(); + vault.initiateAction(ActionType.DECREASE_TIMELOCK, address(0), 3 days); + + vm.prank(vaultOwner); + vm.expectRevert(); + vault.initiateAction(ActionType.INCREASE_THRESHOLD, address(0), 2); + + vm.prank(vaultOwner); + vm.expectRevert(); + vault.initiateAction(ActionType.DECREASE_THRESHOLD, address(0), 5); + } + + function testCannotInitiateInvalidAction() public { + vm.prank(vaultOwner); + vm.expectRevert(); + vault.initiateAction(ActionType.INVALID, address(0), 0); + } + + function testCannotInitiateTwoPendingActions() public { + vm.prank(firstSigner); + vault.initiateAction(ActionType.ADD_SIGNER, makeAddr('newSigner'), 0); + vm.prank(firstSigner); + assertEq(vault.totalActions(), 1); + + vm.prank(secondSigner); + vm.expectRevert(); + vault.initiateAction(ActionType.INCREASE_TIMELOCK, address(0), 30 hours); + } + + function testCannotRetrieveInvalidAction() public { + vm.prank(firstSigner); + vault.initiateAction(ActionType.ADD_SIGNER, makeAddr('newSigner'), 0); + vm.prank(firstSigner); + assertEq(vault.totalActions(), 1); + + vm.prank(firstSigner); + vm.expectRevert(); + vault.getAction(2); + } + + function testDeletePendingAction() public { + address newSigner = makeAddr('newSigner'); + vm.prank(firstSigner); + vault.initiateAction(ActionType.ADD_SIGNER, newSigner, 0); + vm.prank(firstSigner); + assertEq(vault.totalActions(), 1); + vm.prank(firstSigner); + (,, address target,,,,,) = vault.getAction(1); + assertEq(target, newSigner); + + vm.prank(vaultExecutor); + vault.deletePendingAction(); + + vm.prank(vaultExecutor); + assertEq(vault.totalActions(), 0); + } + + function testRevokeActionApproval() public { + vm.prank(firstSigner); + vault.initiateAction(ActionType.ADD_SIGNER, makeAddr('newSigner'), 0); + vm.prank(firstSigner); + assertEq(vault.getActionApprovals(1), 0); + + // Approve the action + vm.prank(firstSigner); + vault.approveAction(1); + vm.prank(firstSigner); + assertEq(vault.getActionApprovals(1), 1); + + // Revoke approval + vm.prank(firstSigner); + vault.revokeActionApproval(1); + + // Check the approval count after revocation + vm.prank(firstSigner); + assertEq(vault.getActionApprovals(1), 0); + } + + function testCannotApproveActionTwice() public { + vm.prank(firstSigner); + vault.initiateAction(ActionType.ADD_SIGNER, makeAddr('newSigner'), 0); + vm.prank(firstSigner); + assertEq(vault.getActionApprovals(1), 0); + + vm.prank(firstSigner); + vault.approveAction(1); + vm.prank(firstSigner); + assertEq(vault.getActionApprovals(1), 1); + + vm.prank(firstSigner); + vm.expectRevert(); + vault.approveAction(1); + } + + function testInsufficientActionApprovals() public { + vm.prank(firstSigner); + vault.initiateAction(ActionType.DECREASE_TIMELOCK, address(0), 12 hours); + + vm.prank(firstSigner); + vault.approveAction(1); + vm.prank(secondSigner); + vault.approveAction(1); + + vm.warp(block.timestamp + vault.multiSigTimelock()); + + vm.prank(vaultOwner); + vm.expectRevert(); + vault.executeAction(1); + } + + function testApproveAndExecuteAction() public { + vm.prank(firstSigner); + uint256 newTimelock = 30 hours; + vault.initiateAction(ActionType.INCREASE_TIMELOCK, address(0), newTimelock); + + // Signer approvals + vm.prank(firstSigner); + vault.approveAction(1); + vm.prank(secondSigner); + vault.approveAction(1); + vm.prank(vaultOwner); + vault.approveAction(1); + vm.prank(vaultExecutor); + assertEq(vault.getActionApprovals(1), 3); + vm.prank(vaultExecutor); + assertEq(vault.getActionSignatures(1).length, 3); + + // Simulate time elapse for timelock + skip(36 hours); + + // Execute action + vm.prank(vaultExecutor); + vault.executeAction(1); + + assertEq(vault.multiSigTimelock(), newTimelock); + } +} diff --git a/test/components/MultiSigTransactionTest.t.sol b/test/components/MultiSigTransactionTest.t.sol index 97101f5..7a7fcad 100644 --- a/test/components/MultiSigTransactionTest.t.sol +++ b/test/components/MultiSigTransactionTest.t.sol @@ -3,7 +3,6 @@ pragma solidity ^0.8.27; import {IERC20} from '@openzeppelin/contracts/token/ERC20/IERC20.sol'; import {Address} from '@openzeppelin/contracts/utils/Address.sol'; -import {Transaction} from '../../src/utilities/VaultStructs.sol'; import {MockERC20Token} from '../mocks/MockERC20Token.sol'; import './BaseMultiSigTest.t.sol'; @@ -14,16 +13,20 @@ contract MultiSigTransactionTest is BaseMultiSigTest { function setUp() public override { super.setUp(); - mockToken = IERC20(address(new MockERC20Token())); + mockToken = IERC20(address(new MockERC20Token(vaultOwner))); mockAddress = address(mockToken); - deal(mockAddress, vaultAddress, 1000 ether); + vm.prank(vaultOwner); + mockToken.approve(vaultAddress, 300 ether); + vm.prank(vaultOwner); + vault.depositToken(mockAddress, 300 ether); + vm.prank(vaultOwner); Address.sendValue(payable(vaultAddress), 10 ether); } - function testContactBalance() public view { + function testContactBalance() public { assertEq(vault.getBalance(), 10 ether); - assertEq(vault.getTokenBalance(mockAddress), 1000 ether); + assertEq(vault.getTokenBalance(mockAddress), 300 ether); } function testInitiateETHTransaction() public { @@ -38,13 +41,24 @@ contract MultiSigTransactionTest is BaseMultiSigTest { assertEq(vault.getTransactionApprovals(1), 0); vm.prank(secondSigner); - Transaction memory txn = vault.getTransaction(1); - assertEq(txn.initiator, firstSigner); - assertEq(txn.target, txReceiver); - assertEq(txn.token, address(0)); - assertEq(txn.value, txValue); - assertFalse(txn.isExecuted); - assertEq(txn.data, txData); + ( + address initiator, + address to, + address token, + uint256 value, + uint256 approvals, + bool isExecuted, + bool isOverride, + bytes memory data + ) = vault.getTransaction(1); + assertEq(initiator, firstSigner); + assertEq(to, txReceiver); + assertEq(token, address(0)); + assertEq(value, txValue); + assertEq(approvals, 0); + assertFalse(isExecuted); + assertFalse(isOverride); + assertEq(data, txData); } function testApproveAndExecuteETHTransaction() public { @@ -57,6 +71,9 @@ contract MultiSigTransactionTest is BaseMultiSigTest { vault.approveTransaction(1); vm.prank(secondSigner); vault.approveTransaction(1); + + // Fast forward time to simulate passing the timelock period + skip(2 days); vm.prank(vaultOwner); vault.approveTransaction(1); @@ -75,10 +92,10 @@ contract MultiSigTransactionTest is BaseMultiSigTest { assertEq(vault.totalTransactions(), 1); vm.prank(firstSigner); - Transaction memory txn = vault.getTransaction(1); - assertEq(txn.initiator, secondSigner); - assertEq(txn.token, mockAddress); - assertEq(txn.value, tokenAmount); + (address initiator,, address token, uint256 value,,,,) = vault.getTransaction(1); + assertEq(initiator, secondSigner); + assertEq(token, mockAddress); + assertEq(value, tokenAmount); } function testApproveAndExecuteERC20Transaction() public { @@ -86,18 +103,25 @@ contract MultiSigTransactionTest is BaseMultiSigTest { uint256 tokenAmount = 100 ether; vault.initiateTransaction(payable(address(0x1234)), mockAddress, tokenAmount, '0x'); - vm.prank(firstSigner); - vault.approveTransaction(1); vm.prank(secondSigner); vault.approveTransaction(1); vm.prank(vaultOwner); vault.approveTransaction(1); + // Fast forward time to simulate passing the timelock period for executor override + skip(2 days); + vm.prank(vaultExecutor); vault.executeTransaction(1); + bool isOverride; + uint256 approvals; + vm.prank(vaultExecutor); + (,,,, approvals,, isOverride,) = vault.getTransaction(1); + assertTrue(isOverride); + assertEq(approvals, 2); assertEq(mockToken.balanceOf(payable(address(0x1234))), tokenAmount); - assertEq(vault.getTokenBalance(mockAddress), 900 ether); + assertEq(vault.getTokenBalance(mockAddress), 200 ether); } function testRevokeApproval() public { @@ -110,20 +134,22 @@ contract MultiSigTransactionTest is BaseMultiSigTest { assertEq(vault.getTransactionApprovals(1), 1); vm.prank(firstSigner); - vault.revokeApproval(1); + vault.revokeTransactionApproval(1); vm.prank(firstSigner); assertEq(vault.getTransactionApprovals(1), 0); } - function testInsufficientApprovals() public { + function testInsufficientTransactionApprovals() public { vm.prank(firstSigner); vault.initiateTransaction(payable(address(0x1234)), address(0), 1 ether, '0x'); vm.prank(firstSigner); vault.approveTransaction(1); - vm.prank(vaultExecutor); + skip(2 days); + + vm.prank(vaultOwner); vm.expectRevert(); vault.executeTransaction(1); } diff --git a/test/components/roles/ExecutorRoleTest.t.sol b/test/components/roles/ExecutorRoleTest.t.sol index f429cff..5953771 100644 --- a/test/components/roles/ExecutorRoleTest.t.sol +++ b/test/components/roles/ExecutorRoleTest.t.sol @@ -156,7 +156,7 @@ contract ExecutorRoleTest is MultiSigEnterpriseVaultTest { vm.prank(vaultExecutor); uint256 newOwnerOverrideTimelock = 24 hours; - vault.decreaseOwnerOverrideTimelockLimit(newOwnerOverrideTimelock); + vault.decreaseOwnerOverrideTimelock(newOwnerOverrideTimelock); assertEq(vault.ownerOverrideTimelock(), newOwnerOverrideTimelock); } diff --git a/test/components/roles/OwnerRoleTest.t.sol b/test/components/roles/OwnerRoleTest.t.sol index 1bc9b54..8dd8ca9 100644 --- a/test/components/roles/OwnerRoleTest.t.sol +++ b/test/components/roles/OwnerRoleTest.t.sol @@ -21,35 +21,35 @@ contract OwnerRoleTest is MultiSigEnterpriseVaultTest { function testOnlyOwnerCanUpdateThreshold() public { vm.prank(address(0x5678)); vm.expectRevert(); - vault.ownerUpdateSignatoryThreshold(5); + vault.updateSignatoryThreshold(5); } function testUpdateSignatoryThreshold() public { vm.prank(vaultOwner); uint256 newSignatoryThreshold = 5; - vault.ownerUpdateSignatoryThreshold(newSignatoryThreshold); + vault.updateSignatoryThreshold(newSignatoryThreshold); assertEq(vault.signatoryThreshold(), newSignatoryThreshold); } function testUnauthorizedTimelockUpdate() public { vm.prank(address(0x5678)); vm.expectRevert(); - vault.increaseOwnerOverrideTimelockLimit(5 days); + vault.increaseOwnerOverrideTimelock(5 days); } function testOwnerOverrideTimelock() public view { - assertEq(vault.ownerOverrideTimelock(), initialOwnerOverrideLimit); + assertEq(vault.ownerOverrideTimelock(), initialOwnerOverrideTimelock); } function testOwnerCanIncreaseOverrideTimelock() public { vm.prank(vaultOwner); - vault.increaseOwnerOverrideTimelockLimit(5 days); + vault.increaseOwnerOverrideTimelock(5 days); assertEq(vault.ownerOverrideTimelock(), 5 days); } function testOwnerCanDecreaseOverrideTimelock() public { vm.prank(vaultOwner); - vault.decreaseOwnerOverrideTimelockLimit(24 hours); + vault.decreaseOwnerOverrideTimelock(24 hours); assertEq(vault.ownerOverrideTimelock(), 24 hours); } } diff --git a/test/components/roles/SignerRoleTest.t.sol b/test/components/roles/SignerRoleTest.t.sol index 1ad93f2..805d355 100644 --- a/test/components/roles/SignerRoleTest.t.sol +++ b/test/components/roles/SignerRoleTest.t.sol @@ -35,18 +35,29 @@ contract SignerRoleTest is MultiSigEnterpriseVaultTest { vault.addSigner(invalidSigner); } - function testOwnerCannotUpdateThresholdWithoutApproval() public { + function testOwnerCannotAddSignerWithoutApproval() public { vm.prank(vaultOwner); vault.addSigner(firstSigner); vm.prank(vaultOwner); vault.addSigner(secondSigner); vm.prank(vaultOwner); + assertEq(vault.totalSigners(), 2); + + vm.prank(vaultOwner); + vm.expectRevert(); vault.addSigner(vaultDeployer); - assertEq(vault.totalSigners(), 3); + } + + function testOwnerCannotUpdateThresholdWithoutApproval() public { + vm.prank(vaultOwner); + vault.addSigner(firstSigner); + vm.prank(vaultOwner); + vault.addSigner(secondSigner); + assertEq(vault.totalSigners(), 2); vm.prank(vaultOwner); vm.expectRevert(); - vault.ownerUpdateSignatoryThreshold(5); + vault.updateSignatoryThreshold(5); } function testSignerCannotAddAnotherSigner() public { @@ -74,6 +85,9 @@ contract SignerRoleTest is MultiSigEnterpriseVaultTest { assertEq(signers.length, 0); // Verify the signer was removed via `_users` + vm.prank(vaultOwner); + assertEq(vault.totalUsers(), 1); + vm.prank(vaultOwner); vm.expectRevert(); vault.getUserProfile(firstSigner); @@ -93,5 +107,9 @@ contract SignerRoleTest is MultiSigEnterpriseVaultTest { vm.prank(vaultOwner); vm.expectRevert(); vault.addSigner(firstSigner); + + vm.prank(vaultOwner); + assertEq(vault.totalUsers(), 2); + assertEq(vault.totalSigners(), 1); } } diff --git a/test/mocks/MockERC20Token.sol b/test/mocks/MockERC20Token.sol index 5981822..45e1ff3 100644 --- a/test/mocks/MockERC20Token.sol +++ b/test/mocks/MockERC20Token.sol @@ -8,7 +8,9 @@ import '@openzeppelin/contracts/token/ERC20/ERC20.sol'; * @dev A mock ERC20 token for testing purposes. */ contract MockERC20Token is ERC20 { - constructor() ERC20('Mock Token', 'MOCK') { - _mint(msg.sender, 1000 ether); + constructor( + address owner + ) ERC20('Mock Token', 'MOCK') { + _mint(owner, 1000 ether); } }