Ballot winning at timestamp ballot.ballotMaximumEndTime can be front-run by manuallyRemoveBallot() and deleted

[c4-bot-1]

Lines of code

https://github.com/othernet-global/salty-io/blob/main/src/dao/DAO.sol#L256

Vulnerability details

Summary

Nobody, not even the owner of the proposal should be allowed to manuallyRemoveBallot() once it has been approved.

Attack Vector

• Proposal attains approved status i.e. proposals.ballotIsApproved(ballotID) = true at timestamp equal to or greater than ballot.ballotMaximumEndTime.
• Attacker monitoring the mempool front-runs any call to finalizeBallot() and calls manuallyRemoveBallot() to delete the ballot.

Another less likely, but nevertheless possible scenario is where the owner of the proposal themself decide to delete the ballot even after all users have voted & helped it win.

Such behaviour should not be allowed and the protocol can check that only unapproved ballots can be manually removed.

Recommended Mitigation Steps

Add the following check:

    // Remove a ballot from voting which has existed for longer than the DAOConfig.ballotMaximumDuration
    function manuallyRemoveBallot( uint256 ballotID ) external nonReentrant
        {
        Ballot memory ballot = proposals.ballotForID(ballotID);

        require( block.timestamp >= ballot.ballotMaximumEndTime, "The ballot is not yet able to be manually removed" );
+       if ( ballot.ballotType == BallotType.PARAMETER )
+         require( proposals.winningParameterVote(ballotID) != Vote.NO_CHANGE, "The ballot has already approved a parameter change" );
+       else
+         require( !proposals.ballotIsApproved(ballotID), "The ballot is already approved" );

        // Mark the ballot as no longer votable and remove it from the list of open ballots
        proposals.markBallotAsFinalized(ballotID);
        }

Assessed type

Other

[Picodes]

This is a partial dup of #82. The way I understand the current design is that it works as intended because the ballot needs to reach the requirements and be finalized between ballotMinimumEndTime and ballotMaximumEndTime to be considered successful.

[t0x1cC0de]

Thank you for the comments.

I myself could not make out from any description anywhere that this is indeed the case -

"The way I understand the current design is that it works as intended because the ballot needs to reach the requirements and be finalized between ballotMinimumEndTime and ballotMaximumEndTime to be considered successful."

My reasons were the following -

• Users are allowed by the protocol to vote even after ballotMaximumEndTime. There is no check inside castVote() function which gives the indication that voting is not allowed past ballotMaximumEndTime.
• finalizeBallot() needs to be explicitly called after getting a majority. Once again, there wasn't/isn't any constraint from protocol's side regarding how late this can be called inside the function itself. With the addition of manuallyRemoveBallot() in its current form, it seems the winning side is now expected to call finalizeBallot() before ballotMaximumEndTime, otherwise someone else calling manuallyRemoveBallot() is considered fair game?

I felt this was not really what the protocol intended, hence the bug report. Will let you and sponsor form an opinion on this.

Please note that I had filed Report #2 under the same assumptions as above, which is downgraded to QA. If my assumptions are incorrect then it's fine, no worries. Else you may want to reconsider the severity of that report too.

[othernet-global]

Yes, this is by design. Essentially finalizeBallot() needs to be called if approved before ballotMaximumEndTime is reached. It affords some protection against spammed proposals (such as send SALT), which are now limited to one per week. In a governance attack, only some of the spammed proposals would be executed and the rest could be manually removed by any user.

[c4-sponsor]

othernet-global (sponsor) disputed

[Picodes]

@t0x1cC0de Following the sponsor's answer I'll downgrade this to QA

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)