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

`MartenitsaVoting::announceWinner` will announce only one winner in case of a tie

kostov

Summary

MartenitsaVoting::announceWinner will announce only one winner in case of a tie because of the algorithm used to find the most voted token.

Vulnerability Details

The announceWinner algorithm works that in a case of a tie between few users, only the first user who was voted for will be the winner.
Lets say Jack and Chasy both make beautifull martenitsa tokens and they tie with 10 votes each.
Ann voted for Jack, and she was the first to vote for either Jack or Chasy, because of that Jack's tokenId is before the Chasy's tokenId
in MartenitsaVoting::voteCounts array.
Now is time to announce the winner, as we said Jack and Chasy have the most votes - 10 each.
After voteCounts[_tokendIds[i]] equals Jack token, maxVotes will be set to 10 and winnerTokenId will be set to Jack's token.
Now votesCounts[_tokenIds[i]] equals Chasy token, but we won't go inside the if to set winnerTokenId because this check fails if (voteCounts[_tokenIds[i]] > maxVotes) { - we will be comparing if(10 > 10) which is false and poor Chasy won't get any reward although she has the same amount of votes as Jack.

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);
}

Impact

Although there might be a few winners, at the end only one of them will get the reward and be announced as winner(the fisrt who was voted for).

Proof of Code

Paste the following test in MartenitsaVoting.sol and run forge test --match-test testAnnounceWinnerTie

function testAnnounceWinnerTie() public listMartenitsa {
//List 1 more martenitsa
vm.startPrank(jack);
martenitsaToken.createMartenitsa("copy");
marketplace.listMartenitsaForSale(1, 1 wei);
vm.stopPrank();
​
//Chasy votes vor Jack martenitsa
vm.prank(chasy);
voting.voteForMartenitsa(1);
vm.stopPrank();
​
//Bob votes for Chasy martenitsa
vm.prank(bob);
voting.voteForMartenitsa(0);
vm.stopPrank();
​
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])));
//Although it isa tie, the winner will be Jack because his martenitsa fisrt went into voteCounts array
assert(winner == jack);
}

Tools Used

Manual Review

Unit Tests

Recommendations

As The voting takes place only once. we can crate a mapping of the winnerTokenIds;

//mapping MartenitsaTokenId -> count of votes
mapping(uint256 => uint256) public voteCounts;
//mapping user address -> if this address is already voted
mapping(address => bool) public hasVoted;
+ //mapping winnerTokenIds
+ mapping(uint256 => bool) public winnerTokenIds;

Then change the alghorithm slightly so we can allow for a few winners.

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) {
+ if (voteCounts[_tokenIds[i]] >= maxVotes) {
maxVotes = voteCounts[_tokenIds[i]];
- winnerTokenId = _tokenIds[i];
+ winnerTokenIds[_tokenIds[i]] = true;
}
}
​
}

Then split the reward between the winners or find another way to decide a winner.

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.