chore: refactoring

script/DeploySimple.s.sol

@@ -126,7 +126,8 @@ contract CrispVotingScript is Script {
             committeeSize: crispEnvVariables.committeeSize,
             paramSet: crispEnvVariables.paramSet,
             crispProgramAddress: crispEnvVariables.crispProgramAddress,
-            computeProviderParams: crispEnvVariables.computeProviderParams
+            computeProviderParams: crispEnvVariables.computeProviderParams,
+            votingSettings: crispEnvVariables.votingSettings
         });
     }
 

src/CrispVoting.sol

@@ -9,7 +9,6 @@ import {
 } from "@aragon/osx-commons-contracts/src/plugin/extensions/proposal/ProposalUpgradeable.sol";
 import {IVotesUpgradeable} from "@openzeppelin/contracts-upgradeable/governance/utils/IVotesUpgradeable.sol";
 import {IProposal} from "@aragon/osx-commons-contracts/src/plugin/extensions/proposal/IProposal.sol";
-import {SafeCastUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/math/SafeCastUpgradeable.sol";
 import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
 import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 
@@ -18,14 +17,14 @@ import {E3, IE3Program} from "./IE3.sol";
 import {ICrispVoting} from "./ICrispVoting.sol";
 import {ICRISP} from "./ICRISP.sol";
 
-/// @title My Upgradeable Plugin
-/// @notice A plugin that exposes a permissioned function to store a number and a function that makes the DAO execute an action.
-/// @dev In order to call setNumber() the caller needs to hold the MANAGER_PERMISSION
-/// @dev In order for resetDaoMetadata() to work, the plugin needs to hold EXECUTE_PERMISSION_ID on the DAO
+/// @title CrispVoting
+/// @notice An Aragon OSx governance plugin that runs private, encrypted votes through Enclave's
+/// CRISP E3 program. Proposal creation registers an E3 request with Enclave; once the tally is
+/// decrypted and published by the CRISP program, the proposal can be executed if it meets the
+/// quorum and winning-option criteria.
+/// @dev In order for executed actions to run, the plugin needs to hold EXECUTE_PERMISSION_ID on the DAO.
 /// @notice This plugin is inspired by MACI's voting plugin - https://github.com/privacy-ethereum/maci-voting-plugin-aragon/blob/main/src/MaciVoting.sol
 contract CrispVoting is PluginUUPSUpgradeable, ProposalUpgradeable, ICrispVoting {
-    /// @notice used to cast uint256 to uint64 safely
-    using SafeCastUpgradeable for uint256;
     /// @notice used to perform safe ERC20 operations
     using SafeERC20 for IERC20;
 
@@ -88,6 +87,13 @@ contract CrispVoting is PluginUUPSUpgradeable, ProposalUpgradeable, ICrispVoting
         paramSet = _params.paramSet;
         crispProgramAddress = _params.crispProgramAddress;
         computeProviderParams = _params.computeProviderParams;
+
+        _updateVotingSettings(_params.votingSettings);
+    }
+
+    /// @inheritdoc ICrispVoting
+    function updateVotingSettings(VotingSettings calldata _votingSettings) external auth(MANAGER_PERMISSION_ID) {
+        _updateVotingSettings(_votingSettings);
     }
 
     /// @notice Creates a new E3 request in Enclave
@@ -128,6 +134,10 @@ contract CrispVoting is PluginUUPSUpgradeable, ProposalUpgradeable, ICrispVoting
             }
         }
 
+        /// @notice Validate and normalise the dates, enforcing the configured minimum duration.
+        /// The validated values feed both the Enclave input window and the stored parameters.
+        (_startDate, _endDate) = _validateProposalDates(_startDate, _endDate);
+
         {
             /// @notice Decode the data
             (uint256 _allowFailureMap, uint256 numOptions, uint256 creditMode, uint256 credits) =
@@ -160,7 +170,7 @@ contract CrispVoting is PluginUUPSUpgradeable, ProposalUpgradeable, ICrispVoting
             // take it from the caller
             enclaveFeeToken.safeTransferFrom(_msgSender(), address(this), fee);
             // approve the enclave contract to take the fee
-            enclaveFeeToken.approve(address(enclave), fee);
+            enclaveFeeToken.forceApprove(address(enclave), fee);
 
             // send the request to Enclave
             (uint256 e3Id,) = enclave.request(requestParams);
@@ -171,7 +181,8 @@ contract CrispVoting is PluginUUPSUpgradeable, ProposalUpgradeable, ICrispVoting
                 numOptions: numOptions,
                 startDate: _startDate,
                 endDate: _endDate,
-                snapshotBlock: block.number,
+                // snapshot the previous block so voting power is read from a finalized block
+                snapshotBlock: block.number - 1,
                 minVotingPower: votingSettings.minProposerVotingPower,
                 minParticipation: votingSettings.minParticipation
             });
@@ -194,16 +205,27 @@ contract CrispVoting is PluginUUPSUpgradeable, ProposalUpgradeable, ICrispVoting
 
     /// @inheritdoc IProposal
     function execute(uint256 _proposalId) external {
+        if (!_proposalExists(_proposalId)) {
+            revert NonexistentProposal(_proposalId);
+        }
+
         Proposal storage proposal = proposals[_proposalId];
 
+        // the voting window must have closed before a proposal can be executed
+        if (block.timestamp < proposal.parameters.endDate) {
+            revert ProposalExecutionForbidden(_proposalId);
+        }
+
         uint256[] memory tallyCounts = ICRISP(crispProgramAddress).decodeTally(proposal.e3Id);
-        proposal.tally.counts = tallyCounts;
 
-        // check if we can execute it3
-        if (!_canExecute(_proposalId)) {
+        // check if we can execute it using the freshly decoded tally
+        if (!_canExecute(_proposalId, tallyCounts)) {
             revert ProposalExecutionForbidden(_proposalId);
         }
 
+        /// @notice store the final tally
+        proposal.tally.counts = tallyCounts;
+
         /// @notice we set the proposal as executed so it cannot be executed again
         proposal.executed = true;
 
@@ -220,10 +242,21 @@ contract CrispVoting is PluginUUPSUpgradeable, ProposalUpgradeable, ICrispVoting
     }
 
     /// @notice Returns whether the proposal has succeeded or not.
+    /// @dev A proposal has succeeded if it has already been executed or if it currently meets the
+    /// execution criteria (quorum and the winning-option rules). This is independent of whether the
+    /// proposal has actually been executed.
     /// @param _proposalId The id of the proposal.
     /// @return Whether the proposal has succeeded or not.
     function hasSucceeded(uint256 _proposalId) external view returns (bool) {
-        return proposals[_proposalId].executed;
+        if (!_proposalExists(_proposalId)) {
+            revert NonexistentProposal(_proposalId);
+        }
+
+        if (proposals[_proposalId].executed) {
+            return true;
+        }
+
+        return _canExecute(_proposalId);
     }
 
     /// @notice Returns the proposal data for a given proposal ID.
@@ -280,9 +313,10 @@ contract CrispVoting is PluginUUPSUpgradeable, ProposalUpgradeable, ICrispVoting
         return _canExecute(_proposalId);
     }
 
-    /// @notice Get the custom proposal parameters ABI
+    /// @notice Get the custom proposal parameters ABI.
+    /// @dev Mirrors the `_data` payload decoded in `createProposal`.
     function customProposalParamsABI() external pure returns (string memory) {
-        return "(uint256 allowFailureMap, uint8 voteOption, bool tryEarlyExecution)";
+        return "(uint256 allowFailureMap, uint256 numOptions, uint256 creditMode, uint256 credits)";
     }
 
     /// @notice Get the tally result
@@ -326,7 +360,22 @@ contract CrispVoting is PluginUUPSUpgradeable, ProposalUpgradeable, ICrispVoting
         return winnerIndex;
     }
 
-    /// @notice Validates and returns the proposal vote dates.
+    /// @notice Validates and stores the voting settings.
+    /// @dev `minParticipation` is a ratio expressed against `RATIO_BASE`, so it cannot exceed it.
+    /// @param _votingSettings The voting settings to store.
+    function _updateVotingSettings(VotingSettings memory _votingSettings) internal {
+        if (_votingSettings.minParticipation > RATIO_BASE) {
+            revert RatioOutOfBounds({limit: RATIO_BASE, actual: _votingSettings.minParticipation});
+        }
+
+        votingSettings = _votingSettings;
+
+        emit VotingSettingsUpdated(
+            _votingSettings.minProposerVotingPower, _votingSettings.minParticipation, _votingSettings.minDuration
+        );
+    }
+
+    /// @notice Validates and returns the proposal vote dates, enforcing the minimum duration.
     /// @param _start The start date of the proposal vote. If 0, the current timestamp is used
     /// and the vote starts immediately.
     /// @param _end The end date of the proposal vote. If 0, `_start + minDuration` is used.
@@ -337,21 +386,22 @@ contract CrispVoting is PluginUUPSUpgradeable, ProposalUpgradeable, ICrispVoting
         view
         returns (uint64 startDate, uint64 endDate)
     {
-        uint64 currentTimestamp = block.timestamp.toUint64();
+        // block.timestamp cannot exceed uint64 for ~580 billion years, so the cast is safe.
+        uint64 currentTimestamp = uint64(block.timestamp);
 
         if (_start == 0) {
             startDate = currentTimestamp;
         } else {
             startDate = _start;
 
+            // the vote cannot start in the past, otherwise the minimum duration is meaningless
             if (startDate < currentTimestamp) {
                 revert DateOutOfBounds({limit: currentTimestamp, actual: startDate});
             }
         }
 
-        // Since `minDuration` is limited to 1 year, `startDate + minDuration` can only overflow if
-        // the `startDate` is after `type(uint64).max - minDuration`. In this case, the proposal
-        // creation will revert and another date can be picked.
+        // checked arithmetic: an absurdly large `minDuration` simply reverts here, and the caller
+        // can pick another date. Bounding `minDuration` on update would tighten this further.
         uint64 earliestEndDate = startDate + votingSettings.minDuration;
 
         if (_end == 0) {
@@ -365,19 +415,28 @@ contract CrispVoting is PluginUUPSUpgradeable, ProposalUpgradeable, ICrispVoting
         }
     }
 
-    /// @notice Internal checks to determine whether a proposal can be executed or not
+    /// @notice Internal checks to determine whether a proposal can be executed or not.
+    /// @dev Fetches the tally from the CRISP program before delegating to the counts-based check.
     /// @param _proposalId The ID of the proposal to be checked
     /// @return Returns `true` if the proposal can be executed, otherwise false
     function _canExecute(uint256 _proposalId) internal view returns (bool) {
+        uint256[] memory counts = ICRISP(crispProgramAddress).decodeTally(proposals[_proposalId].e3Id);
+        return _canExecute(_proposalId, counts);
+    }
+
+    /// @notice Internal checks to determine whether a proposal can be executed or not, given an
+    /// already-decoded tally. Avoids a redundant `decodeTally` call on the execution path.
+    /// @param _proposalId The ID of the proposal to be checked
+    /// @param counts The decoded tally counts for the proposal
+    /// @return Returns `true` if the proposal can be executed, otherwise false
+    function _canExecute(uint256 _proposalId, uint256[] memory counts) internal view returns (bool) {
         Proposal memory proposal = proposals[_proposalId];
 
         // can't execute twice
         if (proposal.executed) {
             return false;
         }
 
-        uint256[] memory counts = ICRISP(crispProgramAddress).decodeTally(proposal.e3Id);
-
         // Sum all votes for quorum check
         uint256 totalVotes = 0;
         for (uint256 i = 0; i < counts.length;) {

src/ICrispVoting.sol

@@ -23,15 +23,23 @@ interface ICrispVoting {
     /// @notice Thrown when a proposal doesn't exist.
     /// @param proposalId The ID of the proposal which doesn't exist.
     error NonexistentProposal(uint256 proposalId);
-    /// @notice Thrown when the caller doesn't have enough voting power.
-    error NoVotingPower();
-    /// @notice Thrown when the proposal is not in the voting period.
-    /// @param limit The bound limit (start or end date).
-    /// @param actual The actual time.
-    error DateOutOfBounds(uint64 limit, uint64 actual);
     /// @notice Thrown when the number of options is less than 2.
     /// @param numOptions The number of options provided.
     error InvalidOptionCount(uint256 numOptions);
+    /// @notice Thrown when a proposal date is outside the allowed bounds.
+    /// @param limit The bound limit (earliest allowed start or end date).
+    /// @param actual The provided date.
+    error DateOutOfBounds(uint64 limit, uint64 actual);
+    /// @notice Thrown when a ratio value (e.g. `minParticipation`) exceeds the ratio base.
+    /// @param limit The maximum allowed value.
+    /// @param actual The provided value.
+    error RatioOutOfBounds(uint256 limit, uint256 actual);
+
+    /// @notice Emitted when the voting settings are updated.
+    /// @param minProposerVotingPower The minimum voting power needed to create a proposal.
+    /// @param minParticipation The minimum participation required for quorum.
+    /// @param minDuration The minimum duration of a vote.
+    event VotingSettingsUpdated(uint256 minProposerVotingPower, uint32 minParticipation, uint64 minDuration);
 
     /// @notice A struct for the voting settings.
     /// @param minProposerVotingPower The minimum voting power needed to propose a vote.
@@ -76,6 +84,7 @@ interface ICrispVoting {
     /// @param paramSet The parameter set to use.
     /// @param e3Program The address of the E3 Program.
     /// @param e3ProgramParams The ABI encoded computation parameters.
+    /// @param votingSettings The initial voting settings (quorum, proposer power, duration).
     struct PluginInitParams {
         IDAO dao;
         address token;
@@ -84,6 +93,7 @@ interface ICrispVoting {
         uint8 paramSet;
         address crispProgramAddress;
         bytes computeProviderParams;
+        VotingSettings votingSettings;
     }
 
     /// @notice A struct for proposal-related information.
@@ -129,6 +139,10 @@ interface ICrispVoting {
     /// @return The minimum duration of the vote.
     function minDuration() external view returns (uint64);
 
+    /// @notice Updates the voting settings. Requires the `MANAGER_PERMISSION`.
+    /// @param _votingSettings The new voting settings.
+    function updateVotingSettings(VotingSettings calldata _votingSettings) external;
+
     /// @notice Returns the proposal data for a given proposal ID.
     /// @param _proposalId The id of the proposal.
     /// @return The proposal data.
@@ -140,6 +154,8 @@ interface ICrispVoting {
     function getTally(uint256 _proposalId) external view returns (TallyResults memory);
 
     /// @notice Returns the index of the option with the most votes.
+    /// @dev On a tie, or when no votes have been cast, the lowest index (0) is returned. Callers
+    /// should not treat a return value of 0 as a definitive winner without inspecting the tally.
     /// @param _proposalId The id of the proposal.
     /// @return The winning option index.
     function getWinningOption(uint256 _proposalId) external view returns (uint256);

src/setup/CrispVotingSetup.sol

@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: AGPL-3.0-or-later
 
-pragma solidity ^0.8.17;
+pragma solidity ^0.8.29;
 
 import {IDAO, DAO} from "@aragon/osx/core/dao/DAO.sol";
 import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol";
@@ -157,7 +157,7 @@ contract CrispVotingSetup is PluginSetup {
         returns (PermissionLib.MultiTargetPermission[] memory permissions)
     {
         // Request reverting the granted permissions
-        permissions = new PermissionLib.MultiTargetPermission[](2);
+        permissions = new PermissionLib.MultiTargetPermission[](1);
 
         // the plugin has the Execute permission on the DAO. This needs to be revoked.
         permissions[0] = PermissionLib.MultiTargetPermission({

test/builders/SimpleBuilder.sol

@@ -19,7 +19,6 @@ contract SimpleBuilder is TestBase {
     // Parameters to override
     address daoOwner; // Used for testing purposes only
     address[] managers; // daoOwner will be used if eventually empty
-    uint256 initialNumber = 1;
 
     GovernanceERC20 governanceERC20Base;
 
@@ -71,7 +70,10 @@ contract SimpleBuilder is TestBase {
             committeeSize: committeeSize,
             crispProgramAddress: crispProgramAddress,
             paramSet: 0,
-            computeProviderParams: computeProviderParams
+            computeProviderParams: computeProviderParams,
+            votingSettings: ICrispVoting.VotingSettings({
+                minProposerVotingPower: 0, minParticipation: 0, minDuration: 0
+            })
         });
 
         // Plugin

test/crispVoting.t.sol

@@ -30,7 +30,8 @@ contract MyPluginTest is TestBase {
         committeeSize: IEnclave.CommitteeSize(0),
         crispProgramAddress: crispProgramAddress,
         paramSet: 0,
-        computeProviderParams: computeProviderParams
+        computeProviderParams: computeProviderParams,
+        votingSettings: ICrispVoting.VotingSettings({minProposerVotingPower: 0, minParticipation: 0, minDuration: 0})
     });
 
     function setUp() public {

test/fork-tests/crisp.t.sol

@@ -31,7 +31,8 @@ contract MyPluginTestFork is TestBase {
         committeeSize: IEnclave.CommitteeSize(0),
         crispProgramAddress: crispProgramAddress,
         paramSet: 0,
-        computeProviderParams: computeProviderParams
+        computeProviderParams: computeProviderParams,
+        votingSettings: ICrispVoting.VotingSettings({minProposerVotingPower: 0, minParticipation: 0, minDuration: 0})
     });
 
     function setUp() public {