Incorrect Winner Announcement Handling in announceWinner Function

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2024-04-Baba-Marta/blob/main/src/MartenitsaVoting.sol#L57-L74

Summary

The announceWinner function in the MartenitsaVoting.sol contract does not handle tie scenarios properly. In cases where multiple tokens receive the same maximum number of votes, only the first token encountered is announced as the winner, neglecting other tokens with the same vote count.

Vulnerability Details

The vulnerability lies in the logic of the announceWinner function, which does not handle tie scenarios properly. In cases where multiple tokens receive the same maximum number of votes, only the first token encountered is announced as the winner, neglecting other tokens with the same vote count.

Proof of Concept (POC) to be added to MartenitsaVoting.t.sol:

function testVoteForMartenitsaTwice() public listMartenitsa {
  vm.startPrank(chasy);
  martenitsaToken.createMartenitsa("bracelet2");
  marketplace.listMartenitsaForSale(1, 1 wei);
  vm.stopPrank();

  vm.prank(address(uint160(1)));
  voting.voteForMartenitsa(0);

  vm.prank(address(uint160(2)));
  voting.voteForMartenitsa(1);

  assert(voting.voteCounts(0) == 1);
  assert(voting.voteCounts(1) == 1);
}

function testAnnounceWinnerWrongly() public {
  testVoteForMartenitsaTwice();
  vm.warp(block.timestamp + 1 days + 1);
  vm.recordLogs();
  voting.announceWinner();

  Vm.Log[] memory entries = vm.getRecordedLogs();
  address winner = address(uint160(uint256(entries[0].topics[2])));
  assert(winner == chasy);
}

Impact

This vulnerability can lead to incorrect determination of winners in tie scenarios, resulting in unfair outcomes and potentially undermining the integrity and credibility of the voting process. It may also impact user trust in the platform and the reliability of voting results.

Tools Used

manual code review.

Recommendations

To address this vulnerability and ensure fair determination of winners in tie scenarios, it is recommended to modify the announceWinner function to handle tie situations appropriately. Consider implementing logic to identify and announce all tied tokens as winners if they share the maximum vote count.

Example:

function announceWinner() external onlyOwner {
  require(block.timestamp >= startVoteTime + duration, "The voting is active");

  uint256 maxVotes = 0;
  address[] memory winners;

  // Find the maximum number of votes
  for (uint256 i = 0; i < _tokenIds.length; i++) {
    if (voteCounts[_tokenIds[i]] > maxVotes) {
      maxVotes = voteCounts[_tokenIds[i]];
    }
  }

  // Identify all tokens with the maximum vote count as winners
  for (uint256 i = 0; i < _tokenIds.length; i++) {
    if (voteCounts[_tokenIds[i]] == maxVotes) {
      winners.push(_tokenIds[i]);
    }
  }

  // Distribute rewards to all winners
  for (uint256 i = 0; i < winners.length; i++) {
    list = _martenitsaMarketplace.getListing(winners[i]);
    _healthToken.distributeHealthToken(list.seller, 1);
    emit WinnerAnnounced(winners[i], list.seller);
  }
}