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

Improper handling of tied votes in `MartenitsaVoting::announceWinner`

paprikrumplikas

Summary

MartenitsaVoting::announceWinner fails to properly address scenarios where multiple NFTs receive the same highest number of votes.

Vulnerability Details

MartenitsaVoting::announceWinner is supposed to

  • identify the NFT(s) with the highest number of votes at the end of the voting period,

  • announce it/them as the winner(s), and

  • reward the the owner(s) of the NFT(s) with 1 healthToken.

However, the current implementation does not handle cases where two or more NFTs receive the highest but equal number of votes.

The test below simulates a voting scenario where:

  • Two NFTs (token ID 0 and token ID 1) are listed and receive an equal number of votes (1 each).

  • The voting concludes, and the winner is announced.

  • Despite the tie, only the owner of token ID 0 receives the reward.

Proof of Code
function testProtocolCantHandleTiedVotes() public {
uint256 price = 1 ether;
address voter1 = makeAddr("voter1");
address voter2 = makeAddr("voter2");
​
vm.startPrank(jack);
martenitsaToken.createMartenitsa("bracelet");
marketplace.listMartenitsaForSale(0, price);
vm.stopPrank();
​
vm.startPrank(chasy);
martenitsaToken.createMartenitsa("bracelet");
marketplace.listMartenitsaForSale(1, price);
vm.stopPrank();
​
voting.startVoting();
​
vm.prank(voter1);
voting.voteForMartenitsa(0);
​
vm.prank(voter2);
voting.voteForMartenitsa(1);
​
vm.warp(block.timestamp + 1 days + 1 seconds);
​
voting.announceWinner();
​
// NFTs with tokenId=0 and tokenID=1 both have 1 vote (highest vote count)
assert(voting.getVoteCount(0) == 1);
assert(voting.getVoteCount(1) == 1);
​
console.log(healthToken.balanceOf(jack));
console.log(healthToken.balanceOf(chasy));
​
// only the owner of tokenId=0 is rewarded
assert(healthToken.balanceOf(jack) == 1 ether);
assert(healthToken.balanceOf(chasy) == 0);
}

Impact

In cases when mulitple NFTs have the highest but equal number of votes (i.e. tied winners), the contract arbitrarily rewards the NFT that appears first in the iteration that still has the maximum votes, while the other "winners" will not get any rewards.

Tools Used

Manual review, Foundry.

Recommendations

Adjust the code in MartenitsaVoting as follows:

- function announceWinner() external onlyOwner {
- require(block.timestamp >= startVoteTime + duration, "The voting is active");
-
- uint256 winnerTokenId;
- uint256 maxVotes = 0;
-
- for (uint256 i = 0; i < _tokenIds.length; i++) {
- if (voteCounts[_tokenIds[i]] > maxVotes) {
- maxVotes = voteCounts[_tokenIds[i]];
- winnerTokenId = _tokenIds[i];
- }
- }
-
- list = _martenitsaMarketplace.getListing(winnerTokenId);
- _healthToken.distributeHealthToken(list.seller, 1);
​
- emit WinnerAnnounced(winnerTokenId, list.seller);
- }
​
+ function announceWinner() external onlyOwner {
+ require(block.timestamp >= startVoteTime + duration, "The voting is active");
​
+
+ uint256 maxVotes = 0;
​
+ // First, determine the maximum number of votes
+ for (uint256 i = 0; i < _tokenIds.length; i++) {
+ if (voteCounts[_tokenIds[i]] > maxVotes) {
+ maxVotes = voteCounts[_tokenIds[i]];
+ }
+ }
+
+ // Now, distribute rewards to all listings with the maximum votes
+ for (uint256 i = 0; i < _tokenIds.length; i++) {
+ if (voteCounts[_tokenIds[i]] == maxVotes) {
+ list = _martenitsaMarketplace.getListing(_tokenIds[i]);
+ _healthToken.distributeHealthToken(list.seller, 1);
+ emit WinnerAnnounced(_tokenIds[i], list.seller);
+ }
+ }
+
+ }
​
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.