[M-02] Finalization uses a vote snapshot but dynamically recalculates quorum with the current total stake

[softstackio]

Severity: Medium
Likelihood: Medium

Description:

During the finalize step of a proposal, votes are counted from a fixed user stakeAmount snapshot taken at the end of the Voting phase.

    function _snapshotStakes(uint64 daoEpoch) private {
        address[] memory daoEpochVoters = _daoEpochVoters[daoEpoch].values();

        for (uint256 i = 0; i < daoEpochVoters.length; ++i) {
            address voter = daoEpochVoters[i];
          uint256 stakeAmount = stakingHbbft.stakeAmountTotal(voter);

            daoEpochStakeSnapshot[daoEpoch][voter] = stakeAmount;
        }
    }

However, the quorum and threshold checks are performed using the current totalStakingAmount rather than the snapshot value.

    function quorumReached(ProposalType _type, VotingResult memory result) public view returns (bool) {
        uint256 requiredExceeding;
       uint256 totalStakedAmount = _getTotalStakedAmount();

        if (_type == ProposalType.ContractUpgrade) {
            requiredExceeding = totalStakedAmount * (50 * 100) / 10000;
        } else {
            requiredExceeding = totalStakedAmount * (33 * 100) / 10000;
        }

        return result.stakeYes >= result.stakeNo + requiredExceeding;
    }

This creates a discrepancy: even though votes are locked-in at the Voting phase end, the effective quorum requirement can shift due to the total stake amount changes happening after the voting concludes but before the proposal is finalized.

Impact:

As a result:

• Validators who initially voted "Yes" could find their proposal failing if other participants increase their stake after voting ends, thereby raising the quorum threshold.
• Validators who initially voted "Yes" are discouraged from increasing their stake until finalize() is called. And this can happen each time they vote Yes for a proposal.
• Participants who voted "No" are incentivized to increase their stake post-vote to push the proposal below the required quorum.
• Even abstainers who increase their stake inadvertently raise the quorum target and may cause a proposal to fail.

This will encourage strategic stake manipulation.

Proof of Concept:

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

    function testQuorumStakeManipulationBetweenVotingAndSnapshot() public {
        address proposer = users[2];
        address voter = users[3];

        // Set initial stake
        mockValidatorSet.add(voter, voter, true);
        mockValidatorSet.add(users[4], users[4], true);
        mockStaking.setStake(voter, 10 ether);

        assertEq(mockStaking.totalStakedAmount(), 10 ether);

        // Create a proposal
        address[] memory targets = new address[](1);
        targets[0] = users[1];

        uint256[] memory values = new uint256[](1);
        values[0] = 100 ether;

        bytes[] memory callDatas = new bytes[](1);
        callDatas[0] = "";

        uint256 proposalId = createProposal(proposer, "Test Proposal", targets, values, callDatas);

        // Switch to Voting phase
        switchPhase();

        // Voter casts a vote
        vm.prank(voter);
        dao.vote(proposalId, Vote.Yes);

        // Switch phase to end Voting
        switchPhase(); //@note users stake snapshot is set here but not the total staked amount

        VotingResult memory res = dao.countVotes(proposalId);
        uint256 requiredExceeding = mockStaking.totalStakedAmount() * (33 * 100) / 10000;
        assertGt(res.stakeYes, res.stakeNo + requiredExceeding); // Proposal is passing at this point

        // A user massively increases their stake before finalization
        mockStaking.setStake(users[4], 1000 ether);

        // Get the vote counts
        VotingResult memory result = dao.countVotes(proposalId);
        requiredExceeding = mockStaking.totalStakedAmount() * (33 * 100) / 10000;
        assertLt(result.stakeYes, result.stakeNo + requiredExceeding); // Now the proposal is not passing

        // Finalize proposal
        dao.finalize(proposalId);

        // Check if the increased stake affected voting power
        assertEq(uint256(dao.getProposal(proposalId).state), uint256(ProposalState.Declined)); // not passing
    }

Recommendation

The total stake amount has to be snapshotted in _snapshotStakes during the phase switch. This will make sure that the quorum calculation is fair and coherent. You can follow OZ implementation.