Lines of code

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

Vulnerability details

Impact

finalizeBallot and user voting are in the same block, and the order in which transactions are executed affects the voting results.

Proof of Concept

    function finalizeBallot( uint256 ballotID ) external nonReentrant
        {
        // Checks that ballot is live, and minimumEndTime and quorum have both been reached
        require( proposals.canFinalizeBallot(ballotID), "The ballot is not yet able to be finalized" );
        Ballot memory ballot = proposals.ballotForID(ballotID);
        if ( ballot.ballotType == BallotType.PARAMETER )
            _finalizeParameterBallot(ballotID);
        else if ( ballot.ballotType == BallotType.WHITELIST_TOKEN )
            _finalizeTokenWhitelisting(ballotID);
        else
            _finalizeApprovalBallot(ballotID);
        }

DAO#finalizeBallot and Proposals#castVote can be executed in the same block.

Since on the chain, the order of execution of transactions is uncertain, if finalizeBallot is castVote occurs in the same block, finalizeBallot may be executed before castVote, which invalidates the user's vote.

If the finalizeBallot for some ballots is 50/50, then the result of the last vote is important and will affect the final vote result. A malicious user can use front-running to preempt a transaction and manipulate voting results.

There is no time limit on voting in castVote:

    function castVote( uint256 ballotID, Vote vote ) external nonReentrant
        {
        Ballot memory ballot = ballots[ballotID];

        // Require that the ballot is actually live
        require( ballot.ballotIsLive, "The specified ballot is not open for voting" );

        // Make sure that the vote type is valid for the given ballot
        if ( ballot.ballotType == BallotType.PARAMETER )
            require( (vote == Vote.INCREASE) || (vote == Vote.DECREASE) || (vote == Vote.NO_CHANGE), "Invalid VoteType for Parameter Ballot" );
        else // If a Ballot is not a Parameter Ballot, it is an Approval ballot
            require( (vote == Vote.YES) || (vote == Vote.NO), "Invalid VoteType for Approval Ballot" );

        // Make sure that the user has voting power before proceeding.
        // Voting power is equal to their userShare of STAKED_SALT.
        // If the user changes their stake after voting they will have to recast their vote.

        uint256 userVotingPower = staking.userShareForPool( msg.sender, PoolUtils.STAKED_SALT );
        require( userVotingPower > 0, "Staked SALT required to vote" );

        // Remove any previous votes made by the user on the ballot
        UserVote memory lastVote = _lastUserVoteForBallot[ballotID][msg.sender];

        // Undo the last vote?
        if ( lastVote.votingPower > 0 )
            _votesCastForBallot[ballotID][lastVote.vote] -= lastVote.votingPower;

        // Update the votes cast for the ballot with the user's current voting power
        _votesCastForBallot[ballotID][vote] += userVotingPower;

        // Remember how the user voted in case they change their vote later
        _lastUserVoteForBallot[ballotID][msg.sender] = UserVote( vote, userVotingPower );

        emit VoteCast(msg.sender, ballotID, vote, userVotingPower);
        }

ballot.ballotIsLive is set to false after finalizeBallot is executed:

    function finalizeBallot( uint256 ballotID ) external nonReentrant
        {
        // Checks that ballot is live, and minimumEndTime and quorum have both been reached
        require( proposals.canFinalizeBallot(ballotID), "The ballot is not yet able to be finalized" );

        Ballot memory ballot = proposals.ballotForID(ballotID);

        if ( ballot.ballotType == BallotType.PARAMETER )
@>          _finalizeParameterBallot(ballotID);
        else if ( ballot.ballotType == BallotType.WHITELIST_TOKEN )
            _finalizeTokenWhitelisting(ballotID);
        else
            _finalizeApprovalBallot(ballotID);
        }

    function _finalizeParameterBallot( uint256 ballotID ) internal
        {
        Ballot memory ballot = proposals.ballotForID(ballotID);

        Vote winningVote = proposals.winningParameterVote(ballotID);

        if ( winningVote == Vote.INCREASE )
            _executeParameterChange( ParameterTypes(ballot.number1), true, poolsConfig, stakingConfig, rewardsConfig, stableConfig, daoConfig, priceAggregator );
        else if ( winningVote == Vote.DECREASE )
            _executeParameterChange( ParameterTypes(ballot.number1), false, poolsConfig, stakingConfig, rewardsConfig, stableConfig, daoConfig, priceAggregator );

        // Finalize the ballot even if NO_CHANGE won
@>      proposals.markBallotAsFinalized(ballotID);

        emit BallotFinalized(ballotID, winningVote);
        }

    function markBallotAsFinalized( uint256 ballotID ) external nonReentrant
        {
        require( msg.sender == address(exchangeConfig.dao()), "Only the DAO can mark a ballot as finalized" );

        Ballot storage ballot = ballots[ballotID];

        // Remove finalized whitelist token ballots from the list of open whitelisting proposals
        if ( ballot.ballotType == BallotType.WHITELIST_TOKEN )
            _openBallotsForTokenWhitelisting.remove( ballotID );

        // Remove from the list of all open ballots
        _allOpenBallots.remove( ballotID );

@>      ballot.ballotIsLive = false;

        // Indicate that the user who posted the proposal no longer has an active proposal
        address userThatPostedBallot = _usersThatProposedBallots[ballotID];
        _userHasActiveProposal[userThatPostedBallot] = false;

        delete openBallotsByName[ballot.ballotName];

        emit BallotFinalized(ballotID);
        }

Tools Used

vscode, manual

Recommended Mitigation Steps

finalizeBallot and user voting should not be in the same block

Assessed type

Governance

code-423n4 / 2024-01-salty-findings

finalizeBallot may have a race condition #681