Fix I-03: snapshot extension parameters at proposal creation (#77)

* feat: snapshot extension parameters at proposal creation

* use checkpoints

* - Clean up `_votingPeriodExtensionThresholdTriggered`
- Add OZ-style timpoint based getter for `votingPeriodExtension` and `minorVetoExtensionThresholdPct`
- Add unit tests

---------

Co-authored-by: wildmolasses <changes@gmail.com>

Author: funkyenough

src/BasicCouncilVetoGovernor.sol

@@ -11,6 +11,8 @@ import {
   GovernorTimelockControl,
   TimelockController
 } from "@openzeppelin/contracts/governance/extensions/GovernorTimelockControl.sol";
+import {Checkpoints} from "@openzeppelin/contracts/utils/structs/Checkpoints.sol";
+import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol";
 
 // Internal Dependencies
 import {GovernorVetoOverride} from "src/extensions/GovernorVetoOverride.sol";
@@ -297,4 +299,19 @@ contract BasicCouncilVetoGovernor is
   {
     return true;
   }
+
+  /// @dev Resolves the conflict between GovernorExtendVetoPeriod and
+  /// GovernorVotesVetoThresholdFraction which both define this function with identical
+  /// implementations.
+  function _optimisticUpperLookupRecent(Checkpoints.Trace208 storage ckpts, uint256 timepoint)
+    internal
+    view
+    virtual
+    override(GovernorExtendVetoPeriod, GovernorVotesVetoThresholdFraction)
+    returns (uint256)
+  {
+    // Both parent implementations are identical, delegate to GovernorVotesVetoThresholdFraction
+    return GovernorVotesVetoThresholdFraction._optimisticUpperLookupRecent(ckpts, timepoint);
+  }
 }
+

src/extensions/GovernorExtendVetoPeriod.sol

@@ -4,6 +4,8 @@ pragma solidity ^0.8.30;
 // External Dependencies
 import {IGovernor, Governor} from "@openzeppelin/contracts/governance/Governor.sol";
 import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";
+import {Checkpoints} from "@openzeppelin/contracts/utils/structs/Checkpoints.sol";
+import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol";
 
 /// @title GovernorExtendVetoPeriod
 /// @author [ScopeLift](https://scopelift.co)
@@ -13,16 +15,20 @@ import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";
 /// (contracts/governance/extensions/GovernorPreventLateQuorum.sol) with behavior adapted for
 /// veto-counting.
 abstract contract GovernorExtendVetoPeriod is Governor {
+  using Checkpoints for Checkpoints.Trace208;
+
   /// @notice Emitted when a proposal deadline is pushed back due to reaching its minor veto
   /// threshold.
-  event ProposalExtended(uint256 indexed proposalId, uint64 extendedDeadline);
+  event ProposalExtended(uint256 indexed proposalId, uint256 extendedDeadline);
 
   /// @notice Emitted when the {_votingPeriodExtension} parameter is changed.
-  event VotingPeriodExtensionSet(uint64 oldVotingPeriodExtension, uint64 newVotingPeriodExtension);
+  event VotingPeriodExtensionSet(
+    uint256 oldVotingPeriodExtension, uint256 newVotingPeriodExtension
+  );
 
   /// @notice Emitted when the {_votingPeriodExtensionThresholdPct} parameter is changed.
   event MinorVetoExtensionThresholdPctSet(
-    uint16 oldVotingPeriodExtensionThresholdPct, uint16 newVotingPeriodExtensionThresholdPct
+    uint256 oldVotingPeriodExtensionThresholdPct, uint256 newVotingPeriodExtensionThresholdPct
   );
 
   /// @dev Reverts when a minor veto extension threshold exceeds the percent denominator.
@@ -31,11 +37,11 @@ abstract contract GovernorExtendVetoPeriod is Governor {
 
   /// @dev The extra time (seconds or blocks, depending on the governor clock mode) that may be
   /// added when the minor veto threshold is met.
-  uint48 private _votingPeriodExtension;
+  Checkpoints.Trace208 private _votingPeriodExtension;
 
   /// @dev The minor threshold in percentage points of veto threshold required to trigger an
   /// extension.
-  uint16 private _minorVetoExtensionThresholdPct;
+  Checkpoints.Trace208 private _minorVetoExtensionThresholdPct;
 
   /// @dev Mapping of proposal ID to extended deadline.
   mapping(uint256 proposalId => uint48) private _extendedDeadlines;
@@ -56,16 +62,31 @@ abstract contract GovernorExtendVetoPeriod is Governor {
     return Math.max(super.proposalDeadline(_proposalId), _extendedDeadlines[_proposalId]);
   }
 
-  /// @notice Returns the current voting period extension duration applied when the minor veto
-  /// threshold is triggered.
-  function votingPeriodExtension() public view virtual returns (uint48) {
-    return _votingPeriodExtension;
+  /// @notice Returns the latest voting period extension duration.
+  function votingPeriodExtension() public view virtual returns (uint256) {
+    return _votingPeriodExtension.latest();
   }
 
-  /// @notice Returns the minor veto threshold expressed in percentage points of the real veto
-  /// threshold that must be reached to extend the voting period.
-  function minorVetoExtensionThresholdPct() public view virtual returns (uint16) {
-    return _minorVetoExtensionThresholdPct;
+  /// @notice Returns the voting period extension duration at a specific timepoint.
+  /// @dev Use {proposalSnapshot} for snapshot-based semantics.
+  function votingPeriodExtension(uint256 _timepoint) public view virtual returns (uint256) {
+    return _optimisticUpperLookupRecent(_votingPeriodExtension, _timepoint);
+  }
+
+  /// @notice Returns the latest minor veto threshold percentage.
+  function minorVetoExtensionThresholdPct() public view virtual returns (uint256) {
+    return _minorVetoExtensionThresholdPct.latest();
+  }
+
+  /// @notice Returns the minor veto threshold percentage at a specific timepoint.
+  /// @dev Use {proposalSnapshot} for snapshot-based semantics.
+  function minorVetoExtensionThresholdPct(uint256 _timepoint)
+    public
+    view
+    virtual
+    returns (uint256)
+  {
+    return _optimisticUpperLookupRecent(_minorVetoExtensionThresholdPct, _timepoint);
   }
 
   /// @dev Returns the minor veto extension threshold denominator. Defaults to 100, but may be
@@ -113,13 +134,14 @@ abstract contract GovernorExtendVetoPeriod is Governor {
     virtual
     returns (bool)
   {
-    if (_minorVetoExtensionThresholdPct == 0) return false;
+    uint256 _proposalSnapshot = proposalSnapshot(_proposalId);
 
+    uint16 _minorThresholdPct = SafeCast.toUint16(minorVetoExtensionThresholdPct(_proposalSnapshot));
+    if (_minorThresholdPct == 0) return false;
     uint256 _minorThreshold = Math.mulDiv(
-      vetoThreshold(proposalSnapshot(_proposalId)),
-      _minorVetoExtensionThresholdPct,
-      minorVetoExtensionThresholdDenominator()
+      vetoThreshold(_proposalSnapshot), _minorThresholdPct, minorVetoExtensionThresholdDenominator()
     );
+
     return proposalVotes(_proposalId) >= _minorThreshold;
   }
 
@@ -135,7 +157,8 @@ abstract contract GovernorExtendVetoPeriod is Governor {
     ) {
       // Lock in the first extension decision even if it does not lengthen the deadline so later
       // tallies cannot attempt to extend the same proposal again.
-      uint48 extendedDeadline = clock() + votingPeriodExtension();
+      uint48 extendedDeadline =
+        clock() + SafeCast.toUint48(votingPeriodExtension(proposalSnapshot(_proposalId)));
 
       if (extendedDeadline > proposalDeadline(_proposalId)) {
         emit ProposalExtended(_proposalId, extendedDeadline);
@@ -149,9 +172,9 @@ abstract contract GovernorExtendVetoPeriod is Governor {
   /// event.
   /// @param _newVotingPeriodExtension Duration to extend when triggered.
   function _setVotingPeriodExtension(uint48 _newVotingPeriodExtension) internal virtual {
-    emit VotingPeriodExtensionSet(_votingPeriodExtension, _newVotingPeriodExtension);
-
-    _votingPeriodExtension = _newVotingPeriodExtension;
+    (uint208 oldValue, uint208 newValue) =
+      _votingPeriodExtension.push(clock(), SafeCast.toUint208(_newVotingPeriodExtension));
+    emit VotingPeriodExtensionSet(oldValue, newValue);
   }
 
   /// @dev Internal setter for {_minorVetoExtensionThresholdPct}. Emits a
@@ -166,9 +189,24 @@ abstract contract GovernorExtendVetoPeriod is Governor {
     if (_newMinorVetoExtensionThresholdPct > minorVetoExtensionThresholdDenominator()) {
       revert GovernorExtendVetoPeriod_InvalidThreshold(_newMinorVetoExtensionThresholdPct);
     }
-    emit MinorVetoExtensionThresholdPctSet(
-      _minorVetoExtensionThresholdPct, _newMinorVetoExtensionThresholdPct
+    (uint208 oldValue, uint208 newValue) = _minorVetoExtensionThresholdPct.push(
+      clock(), SafeCast.toUint208(_newMinorVetoExtensionThresholdPct)
     );
-    _minorVetoExtensionThresholdPct = _newMinorVetoExtensionThresholdPct;
+    emit MinorVetoExtensionThresholdPctSet(oldValue, newValue);
+  }
+
+  /**
+   * @dev Returns the numerator at a specific timepoint.
+   */
+  function _optimisticUpperLookupRecent(Checkpoints.Trace208 storage ckpts, uint256 timepoint)
+    internal
+    view
+    virtual
+    returns (uint256)
+  {
+    // If trace is empty, key and value are both equal to 0.
+    // In that case `key <= timepoint` is true, and it is ok to return 0.
+    (, uint48 key, uint208 value) = ckpts.latestCheckpoint();
+    return key <= timepoint ? value : ckpts.upperLookupRecent(SafeCast.toUint48(timepoint));
   }
 }

src/extensions/GovernorVotesVetoThresholdFraction.sol

@@ -124,6 +124,7 @@ abstract contract GovernorVotesVetoThresholdFraction is GovernorVotes {
   function _optimisticUpperLookupRecent(Checkpoints.Trace208 storage ckpts, uint256 timepoint)
     internal
     view
+    virtual
     returns (uint256)
   {
     // If trace is empty, key and value are both equal to 0.

test/GovernorExtendVetoPeriod.t.sol

@@ -108,6 +108,17 @@ contract GovernorVetoExtensionTest is Test {
     vetoMock.forceTriggerVotingPeriodExtensionThreshold();
     vetoMock.exposed_TallyUpdated(_proposalId);
   }
+
+  function _triggerExtensionInWindow(uint256 _proposalId, uint48 _extension)
+    internal
+    returns (uint256 _timestamp)
+  {
+    uint256 _deadline = vetoMock.proposalDeadline(_proposalId);
+    _timestamp = bound(block.timestamp, _deadline - _extension, _deadline);
+    vm.warp(_timestamp);
+    vetoMock.forceTriggerVotingPeriodExtensionThreshold();
+    vetoMock.exposed_TallyUpdated(_proposalId);
+  }
 }
 
 contract Constructor is GovernorVetoExtensionTest {
@@ -136,6 +147,49 @@ contract VotingPeriodExtension is GovernorVetoExtensionTest {
   function test_ReturnsVotingPeriodExtension() public view {
     assertEq(vetoMock.votingPeriodExtension(), INITIAL_VETO_PERIOD_EXTENSION);
   }
+
+  function testFuzz_PendingProposalUseNewVotingPeriodExtension(
+    address _target,
+    uint256 _value,
+    bytes memory _calldata,
+    uint48 _newExtension
+  ) public {
+    uint48 _oldExtension = uint48(vetoMock.votingPeriodExtension());
+    vm.assume(_oldExtension != _newExtension);
+
+    uint256 _proposalId = _createProposal(_target, _value, _calldata);
+    vm.assume(_newExtension < vetoMock.proposalDeadline(_proposalId));
+    vetoMock.exposed_SetVotingPeriodExtension(_newExtension);
+
+    // Return the timepoint when extension is trigged and voting period is extended
+    uint256 ts = _triggerExtensionInWindow(_proposalId, _newExtension);
+
+    assertEq(vetoMock.proposalDeadline(_proposalId), ts + _newExtension);
+    assertNotEq(vetoMock.proposalDeadline(_proposalId), ts + _oldExtension);
+  }
+
+  function testFuzz_ProposalBeyondActiveUseCheckpointedValueWhenCalculatingProposalDeadline(
+    address _target,
+    uint256 _value,
+    bytes memory _calldata,
+    uint48 _newExtension
+  ) public {
+    uint48 _oldExtension = uint48(vetoMock.votingPeriodExtension());
+    vm.assume(_oldExtension != _newExtension);
+
+    uint256 _proposalId = _createProposal(_target, _value, _calldata);
+    uint256 _snapshot = vetoMock.proposalSnapshot(_proposalId);
+
+    vm.warp(_snapshot + 1);
+
+    vetoMock.exposed_SetVotingPeriodExtension(_newExtension);
+
+    // Return the time when extension is trigged and voting period is extended
+    uint256 ts = _triggerExtensionInWindow(_proposalId, _oldExtension);
+
+    assertEq(vetoMock.proposalDeadline(_proposalId), ts + _oldExtension);
+    assertNotEq(vetoMock.proposalDeadline(_proposalId), ts + _newExtension);
+  }
 }
 
 contract VotingPeriodExtensionThreshold is GovernorVetoExtensionTest {
@@ -223,6 +277,71 @@ contract _votingPeriodExtensionThresholdTriggered is GovernorVetoExtensionTest {
     assertTrue(vetoMock.exposed_VotingPeriodExtensionThresholdTriggered(_proposalId));
   }
 
+  function testFuzz_PendingProposalUseNewVetoExtensionThresholdPct(
+    address _target,
+    uint256 _value,
+    bytes memory _calldata,
+    uint16 _lowThresholdPct,
+    address _voter,
+    uint256 _voteWeight
+  ) public {
+    uint256 _highThresholdPct = vetoMock.minorVetoExtensionThresholdPct();
+    _lowThresholdPct = uint16(bound(_lowThresholdPct, 1, _highThresholdPct - 1));
+
+    vetoMock.exposed_setMinorVetoExtensionThresholdPct(_lowThresholdPct);
+    uint256 _proposalId = _createProposal(_target, _value, _calldata);
+
+    uint256 _lowMinorThreshold = Math.mulDiv(
+      vetoMock.vetoThreshold(vetoMock.proposalSnapshot(_proposalId)),
+      _lowThresholdPct,
+      vetoMock.minorVetoExtensionThresholdDenominator()
+    );
+
+    uint256 _highMinorThreshold = Math.mulDiv(
+      vetoMock.vetoThreshold(vetoMock.proposalSnapshot(_proposalId)),
+      _highThresholdPct,
+      vetoMock.minorVetoExtensionThresholdDenominator()
+    );
+
+    _voteWeight = bound(_voteWeight, _lowMinorThreshold, _highMinorThreshold - 1);
+    _castVoteOnProposal(_proposalId, _voter, _voteWeight);
+
+    assertTrue(vetoMock.exposed_VotingPeriodExtensionThresholdTriggered(_proposalId));
+  }
+
+  function testFuzz_CheckpointedValueUsedWhenCalculatingVetoExtensionThresholdPct(
+    address _target,
+    uint256 _value,
+    bytes memory _calldata,
+    uint16 _highThresholdPct,
+    uint16 _lowThresholdPct,
+    address _voter,
+    uint256 _voteWeight
+  ) public {
+    _highThresholdPct = uint16(
+      bound(_highThresholdPct, 51, vetoMock.minorVetoExtensionThresholdDenominator())
+    );
+    _lowThresholdPct = uint16(bound(_lowThresholdPct, 1, _highThresholdPct - 1));
+
+    vetoMock.exposed_setMinorVetoExtensionThresholdPct(_highThresholdPct);
+    uint256 _proposalId = _createProposal(_target, _value, _calldata);
+    uint256 _proposalSnapshot = vetoMock.proposalSnapshot(_proposalId);
+
+    vm.warp(_proposalSnapshot + 1);
+    vetoMock.exposed_setMinorVetoExtensionThresholdPct(_lowThresholdPct);
+
+    uint256 _highMinorThreshold = Math.mulDiv(
+      vetoMock.vetoThreshold(_proposalSnapshot),
+      _highThresholdPct,
+      vetoMock.minorVetoExtensionThresholdDenominator()
+    );
+
+    _voteWeight = bound(_voteWeight, 1, _highMinorThreshold - 1);
+    _castVoteOnProposal(_proposalId, _voter, _voteWeight);
+
+    assertFalse(vetoMock.exposed_VotingPeriodExtensionThresholdTriggered(_proposalId));
+  }
+
   function test_ExtensionDisabledWhenThresholdIsZero(
     address _target,
     uint256 _value,

test/mocks/GovernorExtendVetoPeriodMock.sol

@@ -7,7 +7,7 @@ import {GovernorVetoCountingSimple} from "src/extensions/GovernorVetoCountingSim
 
 /// @title GovernorExtendVetoPeriodMock
 /// @author [ScopeLift](https://scopelift.co)
-contract GovernorExtendVetoPeriodMock is GovernorExtendVetoPeriod, GovernorVetoCountingSimple {
+contract GovernorExtendVetoPeriodMock is GovernorVetoCountingSimple, GovernorExtendVetoPeriod {
   constructor(uint48 _initialVotingPeriodExtension, uint16 _initialVotingPeriodExtensionThreshold)
     Governor("GovernorExtendVetoPeriodMock")
     GovernorExtendVetoPeriod(_initialVotingPeriodExtension, _initialVotingPeriodExtensionThreshold)