First Flight #13: Baba Marta
First Flight #13
Beginner FriendlyFoundryGameFi
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

`MartenitsaVoting::announceWinner` function miscalculates the winner of a voting event, causing the first one in the `MartenitsaVoting::tokenIds` array to always be the winner

0xShiki

Vulnerability Details

The MartenitsaVoting::announceWinner function is used to calculate the winner of a voting event. The winner is the NFT with the most votes. However, if 2 or more NFTs have the same amount of votes, the first one in the MartenitsaVoting::tokenIds array will always be the winner. This is because the MartenitsaVoting::announceWinner function uses the maxVotes variable to store the maximum number of votes, and the winner variable to store the winner NFT ID. If the current NFT has more votes than the maxVotes variable, the winner variable is updated with the current NFT ID. However, if the current NFT has the same amount of votes as the maxVotes variable, the winner variable is not updated, and the first NFT in the MartenitsaVoting::tokenIds array is always the winner.

Impact

The winner of the voting event is always the first NFT in the MartenitsaVoting::tokenIds array if 2 or more NFTs have the same amount of votes, which leads to unfair results in voting events.

Tools Used

Manual Review

Proof of Code

Add this test to MartenitsaVoting.t.sol:

PoC
function testWrongWinnerIfTwoHaveSameVotes() public {
// Chasy lists a martenitsa for sale
vm.startPrank(chasy);
martenitsaToken.createMartenitsa("bracelet");
marketplace.listMartenitsaForSale(0, 1 wei);
vm.stopPrank();
​
// Jack lists a martenitsa for sale
vm.startPrank(jack);
martenitsaToken.createMartenitsa("bracelet");
marketplace.listMartenitsaForSale(1, 1 wei);
vm.stopPrank();
​
// Jack votes for Chasy's martenitsa
vm.prank(jack);
voting.voteForMartenitsa(0);
​
// Bob votes for Jack's martenitsa
vm.prank(bob);
voting.voteForMartenitsa(1);
​
// Announcing winner
vm.warp(block.timestamp + 1 days + 1);
vm.recordLogs();
voting.announceWinner();
​
console.log("NFT 0 votes >>> ", voting.getVoteCount(0));
console.log("NFT 1 votes >>> ", voting.getVoteCount(1));
Vm.Log[] memory entries = vm.getRecordedLogs();
address winner = address(uint160(uint256(entries[0].topics[2])));
assert(winner == chasy);
}

The logs from this test show how Jack votes for Chasy's martenitsa, and Bob votes for Jack's martenitsa. The winner of the voting event is Chasy's martenitsa, even though Jack's martenitsa has the same amount of votes.

Recommendations

Consider implementing a different logic for MartenitsaVoting::announceWinner function. For instance tokenIds, which have the same amount of votes can be stored in a newly created array. Then the winning NFT can be selected from the newly create array, by implementing Chainlink VRF for determining the winner randomly.

Updates

Lead Judging Commences

bube Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Tie in voting is not considered

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.