user might vote to the wrong proposal due to reorg

[c4-bot-6]

Lines of code

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

Vulnerability details

Impact

In current implemenatation, the ballotID is calculated as nextBallotID which increase one everytime. There is a scenario that:

1. Alice creates a proposal whose ballotID is x
2. Users A votes approval for Alice's proposal
3. Bob creates a new proposal whose ballotID is x + 1
4. A blockchain re-org happens, Bob's propsal's ballotID is x, and Alice's propsal's ballotID is x+1 now
5. When User A's vote gets executed, the votes apply on Bob's proposal instead of Alice's

In the case above, User A is supposed to vote for Alice's proposal, but he votes for Bob's proposal after re-org

Proof of Concept

ballotID is calculated as nextBallotID++

 81         function _possiblyCreateProposal( string memory ballotName, BallotType ballotType, address address1, uint256 number1, string memory string1, string memory string2 ) internal returns (uint256 ballotID)
 82                 {
     ...
106
107                 // Add the new Ballot to storage
108                 ballotID = nextBallotID++; <<<--- here ballotID is calculated
109                 ballots[ballotID] = Ballot( ballotID, true, ballotType, ballotName, address1, number1, string1, string2, ballotMinimumEndTime );
110                 openBallotsByName[ballotName] = ballotID;
111                 _allOpenBallots.add( ballotID );
112
    ...
118                 }

Tools Used

VS

Recommended Mitigation Steps

Calculate ballotID as a hash of the proposal properties. This way, votes cannot be misdirected.

Assessed type

Other

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[Picodes]

Downgrading to Low as users can still change their vote but interesting report

[c4-judge]

Picodes marked the issue as grade-a

[c4-judge]

Picodes marked the issue as grade-b

[crazy4linux]

Hi @Picodes , thanks for your judging. I want to add some additional information about the issue You mentioned that

Downgrading to Low as users can still change their vote but interesting report

Say we have two proposals, a good proposal X(for doing some good to the users or the system), and a bad proposal X + 1(for stealing tokens/assets to the malicious user), a user Alice is supposed to vote for the good proposal X, but due to the reorg, she votes for the bad proposal. Of course she can votes for the good proposal again, but it will be difficult for her to cancel votes on the malicious proposal. And if she doesn't cancel votes on the malicious proposal, the bad things might happen

[Picodes]

@crazy4linux the user has time to override its previous vote, no? I still think considering the voting period, the frequency of proposals and the fact that it doesn't really make sense to vote before the proposal creation transaction was included on chain Low severity seems appropriate.