No proposal time limit traps sponsors of unpopular proposals

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L396-L397 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L281-L281

Vulnerability details

Impact

A staker is restricted to sponsoring a single proposal at any given time. This proposal remains active until a predetermined duration has elapsed, and it has received the sufficient number of votes to reach a quorum, after which it can be finalized. However, if a staker sponsors a proposal that fails to attract the necessary quorum of votes, it remains indefinitely in an active state. This situation effectively locks the staker in a position where they cannot propose any new initiatives, as they are stuck with an unresolved proposal.

There is no mechanism for sponsors to withdraw or cancel their proposals. So when a proposal is unable to achieve quorum, the sponsor is left in a predicament where they are unable to further participate in the governance through the initiation of new proposals.

Furthermore, there is a noticeable lack of motivation for stakers to vote against proposals that are not on track to meet the quorum. As these proposals cannot pass without achieving the required quorum, resulting in a situation where voting against such proposals does not offer any tangible benefit.

Proof of Concept

Steps

1. Staker creates a valid proposal
2. Sufficient time passes
3. Insufficient votes are received to reach quorum
4. Staker has an active proposal they cannot close (preventing create a new proposal)

A staker can have only one active proposal

Within Proposals stakers are identified as users and when creating a proposal there is a check Proposals::_possiblyCreateProposal()

            // Make sure that the user doesn't already have an active proposal
            require( ! _userHasActiveProposal[msg.sender], "Users can only have one active proposal at a time" );

With the state being pushed later in Proposals::_possiblyCreateProposal()

        // Remember that the user made a proposal
        _userHasActiveProposal[msg.sender] = true;
        _usersThatProposedBallots[ballotID] = msg.sender;

A proposal can only be finalized after sufficient time has passed

The check to ensure the minimum time has passed in Proposals::canFinalizeBallot()

        // Check that the minimum duration has passed
        if (block.timestamp < ballot.ballotMinimumEndTime )
            return false;

A proposal can only be finalized after quorum is reached

The check to ensure quorum is reached in Proposals::canFinalizeBallot()

        // Check that the required quorum has been reached
        if ( totalVotesCastForBallot(ballotID) < requiredQuorumForBallotType( ballot.ballotType ))
            return false;

Test Case

User sponsors a proposal that fails to reach quorum cannot be finalized and with no other way to close will have an active proposal, preventing further proposals. Add the test case to [Proposals.t.sol](https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/tests/Proposals.t.sol#L1717) ```solidity function test_quorum_needed_to_finalize_ballot() external { uint256 ballotID = 1; uint256 stakeAmount = 250000 ether; // Fund Alice, Bob and Charlie with equal SALT vm.startPrank( DEPLOYER ); salt.transfer( alice, stakeAmount ); salt.transfer( bob, stakeAmount ); salt.transfer( charlie, stakeAmount ); vm.stopPrank(); // Alice, Bob and Charlie all stake their equal amounts of SALT _stakeSalt(alice, stakeAmount); _stakeSalt(bob, stakeAmount); _stakeSalt(charlie, stakeAmount); // Alice proposes a ballot vm.prank(alice); proposals.proposeCountryInclusion("US", "proposed ballot"); // Alice votes YES vm.prank(alice); proposals.castVote( ballotID, Vote.YES ); // Now, we allow some time to pass in order to finalize the ballot vm.warp(block.timestamp + daoConfig.ballotMinimumDuration()); // The ballot cannot be finalized as quorum is not reached assertFalse(proposals.canFinalizeBallot(ballotID), "Ballot cannot be finalized"); assertGt(proposals.requiredQuorumForBallotType(BallotType.INCLUDE_COUNTRY),stakeAmount, "Quorum exceeds stakeAmount" ); assertTrue( proposals.userHasActiveProposal(alice), "Alice has an active proposal"); } function _stakeSalt(address wallet, uint256 amount ) private { vm.startPrank( wallet ); salt.approve(address(staking), amount); staking.stakeSALT(amount); vm.stopPrank(); } ```

Tools Used

Manual review, Foundry test

Recommended Mitigation Steps

Allow the proposals to be closed (equivalent to finalized as NO or NO_CHANGE), which would allow the sponsor to afterward make a different proposal.

(This feature would also generally allow removing dead proposals)

Add a time field to Ballot in IProposals

    // The earliest timestamp at which a ballot can end. Can be open longer if the quorum has not yet been reached for instance.
    uint256 ballotMinimumEndTime;

+   // The earliest timestamp at which a ballot can be closed without quorum being reached.
+   uint256 ballotCloseTime;
    }

Populate the ballotCloseTime in Proposal::_possiblyCreateProposal , using a constant in this example, it could always another DAO configuration option.

        // Make sure that a proposal of the same name is not already open for the ballot
        require( openBallotsByName[ballotName] == 0, "Cannot create a proposal similar to a ballot that is still open" );
        require( openBallotsByName[ string.concat(ballotName, "_confirm")] == 0, "Cannot create a proposal for a ballot with a secondary confirmation" );

        uint256 ballotMinimumEndTime = block.timestamp + daoConfig.ballotMinimumDuration();
+       uint256 ballotCloseTime = ballotMinimumEndTime + 1 weeks;       

        // Add the new Ballot to storage
        ballotID = nextBallotID++;
+       ballots[ballotID] = Ballot( ballotID, true, ballotType, ballotName, address1, number1, string1, string2, ballotMinimumEndTime );
+       ballots[ballotID] = Ballot( ballotID, true, ballotType, ballotName, address1, number1, string1, string2, ballotMinimumEndTime, ballotCloseTime );
        openBallotsByName[ballotName] = ballotID;
        _allOpenBallots.add( ballotID );        

Add a function to return whether a proposal can be closed to Proposal

+   function canCloseBallot( uint256 ballotID ) external view returns (bool)
+       {
+        Ballot memory ballot = ballots[ballotID];
+        if ( ! ballot.ballotIsLive )
+           return false;
+
+        // Check that the minimum duration has passed
+        if (block.timestamp < ballot.ballotCloseTime )
+            return false;
+
+        return true;
+       }

Add a function to close a ballot without any side effect to DAO

+   function closeBallot( uint256 ballotID ) external nonReentrant
+       {
+       // Checks that ballot is live and closeTime has passed
+       require( proposals.canCloseBallot(ballotID), "The ballot is not yet able to be closed" ); 
+
+       // No mutation from the propsal
+       _finalizeApprovalBallot(ballotID);
+       }

Assessed type

Governance

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) confirmed

[othernet-global]

There is now a default 30 day period after which ballots can be removed by any user.

https://github.com/othernet-global/salty-io/commit/758349850a994c305a0ab9a151d00e738a5a45a0

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report