[M-01] Phase duration will be shortened each time switchPhase is delayed

[softstackio]

Severity: Medium
Likelihood: High

Description:

When switchPhase is called, the new phase's start and end timestamps are derived from the previous phase's end timestamp :

     uint64 newPhaseStart = daoPhase.end + 1;
     daoPhase.start = newPhaseStart;
     daoPhase.end = newPhaseStart + DAO_PHASE_DURATION;
     daoPhase.phase = newPhase;

If block.timestamp is already past the previous end, the new phase will in reality be shorter than the intended DAO_PHASE_DURATION. In the worst case, block.timestamp could surpass newPhaseStart + DAO_PHASE_DURATION immediately, allowing multiple rapid switchPhase calls and effectively skipping phases. This leads to irregular and potentially manipulated proposal cycles.

Impact:

• Phases lasts less than 2 weeks.
• In the worst case scenario, phases can become significantly shorter than intended, giving insufficient time for proposals to be created, voted on, or executed as expected.
• The governance process becomes unpredictable and possibly unfair, undermining trust in the DAO's decision-making mechanism.

Proof of Concept:

You can execute the following test by running forge test --mt testSwitchPhaseDurationReducedDueToDelay -vv:

    function testSwitchPhaseDurationReducedDueToDelay() public {
        uint64 DAO_PHASE_DURATION = dao.DAO_PHASE_DURATION();

        // Get the current phase end time
        (, uint64 currentEnd,,) = dao.daoPhase();

        // Simulate a delay: Advance time by 1 hour after the current phase end
        uint64 timeAfterEnd = currentEnd + 1 hours;
        vm.warp(timeAfterEnd);

        // Assume switchPhase is called after the delay
        dao.switchPhase();

        // Get the new phase start and end times
        (uint64 newStart, uint64 newEnd,,) = dao.daoPhase();

        // The new phase end is set to newStart + DAO_PHASE_DURATION
        uint64 expectedEnd = newStart + DAO_PHASE_DURATION;

        // However, since block.timestamp > newStart, the actual duration remaining is less than 2 weeks

        // Calculate the remaining duration of the new phase
        uint64 remainingDuration = newEnd > uint64(block.timestamp) ? newEnd - uint64(block.timestamp) : 0;

        // The remaining duration should be less than DAO_PHASE_DURATION
        assertTrue(remainingDuration < DAO_PHASE_DURATION);

        // Verify that the new phase will last less than the expected DAO_PHASE_DURATION
        uint64 expectedRemainingDuration = expectedEnd - uint64(block.timestamp);
        assertEq(remainingDuration, expectedRemainingDuration);

        // Logging for more clarity
        emit log_named_uint("Current Timestamp:", uint64(block.timestamp));
        emit log_named_uint("New Phase Start:", newStart);
        emit log_named_uint("New Phase End:", newEnd);
        emit log_named_uint("DAO_PHASE_DURATION(2 weeks):", DAO_PHASE_DURATION);
        emit log_named_uint("Remaining Duration of New Phase:", remainingDuration);
    }

Recommendation

Ensure that the new phase duration is calculated from the current block.timestamp rather than from daoPhase.end.

    function switchPhase() external {
        if (block.timestamp < daoPhase.end) {
            return;
        }

        Phase newPhase = daoPhase.phase == Phase.Proposal ? Phase.Voting : Phase.Proposal;

-       uint64 newPhaseStart = daoPhase.end + 1;
-       daoPhase.start = newPhaseStart;
+       daoPhase.start = block.timestamp;
-       daoPhase.end = newPhaseStart + DAO_PHASE_DURATION;
+       daoPhase.end = block.timestamp + DAO_PHASE_DURATION;
        daoPhase.phase = newPhase;

[SurfingNerd]

We are discussing this.
The implication of the suggested solution, we would not stay at fixed time windows anymore, like the Phase start is always at 12:00 PM.
It would start to slightly shift each phase,
12:01 PM,
12:02 PM,
12:05 PM,
...

Depending on the timing of block creations.