There is no specific end time for the balloting process

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L259-L279

Vulnerability details

Proof of Concept

After 45 days, proposals can be passed to the protocol. Users can then vote for the proposals.

These proposals have a minimum end time, but this end time is not enforced.

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

After the minimum end time, users can still vote for their proposals.

This minimum end time is for the DAO to call finalize ballot. The responsibility lies on the DAO to call finalize ballot and end the ballot.

        function canFinalizeBallot( 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.ballotMinimumEndTime )
            return false;

However, there are a few reasons why the DAO cannot call finalize ballot immediately after the minimum end time.

For example, if there are many proposals ongoing at the same time, the DAO may not be able to keep track of which proposals have passed the minimum end time

Also, the DAO may run into some issues when calling finalize ballot. The DAO may also be incentivized to not call finalize ballot if the result isn't what they want.

Lastly, the gas fees immediately after the minimum end time may be too high, and the DAO might wait for gas fees to be lower to call.

This is an issue because if there is no time limit for a ballot, the ballot result can easily switch from one side to another. Let's say there are 50100 YES and 40900 NO for a particular proposal when it reaches minimum end time. The ideal case is for the proposal to be passed with a YES (Approved). Due to delays, the ballot number changes to 50500 YES and 51000 NO. Now, the proposal will be passed with a NO (Not Approved).

This creates an unfair situation for the voters because they might think that they have successfully passed the ballot when in fact the votes may be leaning to the other side after the minimum duration.

Impact

Ballot has no actual end time, users can keep voting until the DAO decides to call finalize ballot, which may create unfair situations for the voters.

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend enforcing a maximum duration for a ballot as well. After this maximum duration, no votes can be cast anymore. This makes sure that voting is a fair process.

Assessed type

Context

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) disputed

[othernet-global]

It is acceptable that the ballot remains open for voting until it has been finalized. It is likely that one of the users in favor of the winning ballot will be motivated to finalize it fairly quickly.

[c4-judge]

Picodes marked the issue as duplicate of #840

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[cryptostaker2]

Hi Picodes,

Sponsor mentions:

It is acceptable that the ballot remains open for voting until it has been finalized. It is likely that one of the users in favor of the winning ballot will be motivated to finalize it fairly quickly.

The reverse could be true as well, the losing ballot will be motivated to finalize it fairly quickly.

Also, the point is about something uncontrollable happening (gas fees too high).

Having a ballot that doesn't really set an end date would be quite a hassle if the ballot is extremely close together. Since anyone can finalize the process, if the ballot is winning 50001 to 49999, a user can just stake some SALT and frontrun the finalizing action to make the ballot a lost instead.

To ensure fairness, an end date will be good.

Sponsor also mentions:

This is acceptable behavior and more critical proposal approvals (contract change and webiste update) trigger an automatic confirmation ballot.

I can't find the function that triggers an automatic confirmation. All the ballot seems to route to the finalizeBallot() function.

Thanks for reviewing!

[c4-sponsor]

othernet-global (sponsor) acknowledged

[othernet-global]

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

[Picodes]

@cryptostaker2 Although I fully agree with you that the current design has undesired side effects and that automatically closing the votes based on a timestamp known in advance would be "fairer", I fail to see how this fulfill the definition of Medium severity: "Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements."

There is no leak of value, the functionality is working well, to me it's just the design that could be improved a bit to avoid unexpected edge cases