c4-bot-3 commented 7 months ago

Lines of code

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

Vulnerability details

Impact

Unexpected result can occur because the result of the winningParameterVote function would be NO_CHANGE when the increaseTotal and decreaseTotal are the same.

Proof of Concept

In _finalizeParameterBallot function of DAO contract, the winningParameterVote function of Proposals contract is called.

    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 winningParameterVote( uint256 ballotID ) external view returns (Vote)
    {
    mapping(Vote=>uint256) storage votes = _votesCastForBallot[ballotID];

    uint256 increaseTotal = votes[Vote.INCREASE];
    uint256 decreaseTotal = votes[Vote.DECREASE];
    uint256 noChangeTotal = votes[Vote.NO_CHANGE];

    if ( increaseTotal > decreaseTotal )
    if ( increaseTotal > noChangeTotal )
        return Vote.INCREASE;

    if ( decreaseTotal > increaseTotal )
    if ( decreaseTotal > noChangeTotal )
        return Vote.DECREASE;

    return Vote.NO_CHANGE;
    }

However, in winningParameterVote function, when the increaseTotal and decreaseTotal are the same, it returns the NO_CHANCE even if the noChangeTotal is 0. This can produce unexpected results for ballots with intense voting competition, and in the worst case, it can be exploited by malicious users.

Tools Used

VS Code

Recommended Mitigation Steps

Should decide how return the result when the increaseTotal and decreaseTotal are the same and update the winningParameterVote function with that logic.

Assessed type

Other

Picodes commented 7 months ago

This is more a design decision as it seems fair to say that "NO_CHANGE" wins in case of equality.

c4-judge commented 7 months ago

Picodes changed the severity to QA (Quality Assurance)