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

`MartenitsaVoting::voteCounts` and `MartenitsaVoting::hasVoted` are not reset after the winner is announced

robertodf99

Summary

The mappings where the count of votes are stored and where it is recorded if the users have particpated in the current voting is not reset after the winner is announced. This means that users can only vote once and the likelihood of early minted tokens of being selected as future winners is higher since they will accrue points since their first voting phase.

Vulnerability Details

This vulnerability has two main implications, first, martenitsas with lower token IDs will begin next rounds with the already accumulated points and therefore with some advantage over newly minted or previously less voted tokens, reducing the fairness of the competition. Second, once users participate in a voting, if they want to particpate in future votings they will have to do it from a different address.

Impact

To illustrate the first implication, imagine the following scenario:

  • chasy mints token ID 0 before the first voting phase begins.

  • The voting begins and bob and someUser vote for her token. Token ID 0 has accumulated 2 points.

  • The voting concludes and chasy is announced as the winner.

  • After finishing the first voting, jack mints token ID 1.

  • The new voting begins and now only anotherUser votes and decides to vote for token ID 1. Token ID 1 has 1 vote.

  • The voting concludes and since the voting count has not been previously reset, chasy is announced again as the winner with no votes in this phase.

See PoC below.

Code

Place this into MartenitsaVoting.t.sol.

function test_votesNotReset() public listMartenitsa {
// someUser and bob vote for alice token
address someUser = makeAddr("someUser");
vm.prank(someUser);
voting.voteForMartenitsa(0);
vm.prank(bob);
voting.voteForMartenitsa(0);
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);
​
// 2nd round starts
voting.startVoting();
​
// 2nd round, jack mints a new token
vm.startPrank(jack);
martenitsaToken.createMartenitsa("bracelet");
marketplace.listMartenitsaForSale(1, 1 wei);
vm.stopPrank();
​
// now a third user votes for jack
address anotherUser = makeAddr("anotherUser");
vm.prank(anotherUser);
voting.voteForMartenitsa(1);
​
// since jack has the only vote in the second voting and it has concluded, he should be the winner, however...
vm.warp(block.timestamp + 1 days + 1);
vm.recordLogs();
voting.announceWinner();
​
entries = vm.getRecordedLogs();
winner = address(uint160(uint256(entries[0].topics[2])));
assert(winner == chasy);
}

For the second implication the following scenario can be considered:

  • chasy mints token ID 0 and the voting begins.

  • bob votes for token ID 0.

  • The voting concludes and chasy is announced as the winner.

  • The second voting begins and no one mints a new token.

  • bob wants to vote again for chasy's token, however the transaction reverts as the mapping MartenitsaVoting::hasVoted is still set to true.

See PoC below.

Code

Place this into MartenitsaVoting.t.sol.

function test_userCanOnlyVoteOnce() public listMartenitsa {
vm.prank(bob);
voting.voteForMartenitsa(0);
vm.warp(block.timestamp + 1 days + 1);
voting.announceWinner();
​
// 2nd round starts
voting.startVoting();
​
// 2nd round, bob votes again for the same token, but he cannot
vm.expectRevert("You have already voted");
vm.prank(bob);
voting.voteForMartenitsa(0);
}

Tools Used

Foundry and manual review.

Recommendations

The fix to reset the vote count is easy and it does not require to change the logic of the contract, however for the partcipants votes it requires to add an extra array and a for loop which are described in detail in the code below.

Code

Adjust this in MartenitsaVoting.

...
+ address[] voters;
...
function voteForMartenitsa(uint256 tokenId) external {
...
hasVoted[msg.sender] = true;
+ voters.push(msg.sender);
...
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];
}
+ delete voteCounts[_tokenIds[i]];
}
​
list = _martenitsaMarketplace.getListing(winnerTokenId);
_healthToken.distributeHealthToken(list.seller, 1);
hasVoted[msg.sender] = false;
​
+ uint256 votersLength = voters.length;
​
+ for (uint256 i; i < votersLength; i++) {
+ hasVoted[voters[i]] = false;
+ }
​
emit WinnerAnnounced(winnerTokenId, list.seller);
}
...
Updates

Lead Judging Commences

bube Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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