low

`MartenitsaVoting::announceWinner` function does not check if there are no vo...

Selected Submission

MartenitsaVoting::announceWinner function does not check if there are no votes, causing a participant to become a winner and receive a HealthToken

Severity

Low Risk

Relevant GitHub Links

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

Vulnerability Details

Users can vote for the best MartenitsaToken NFT, which is listed for sale on the MartenitsaMarketplace. The MartenitsaVoting::announceWinner function is used to calculate the winner of the voting event. The winner is the NFT with the most votes. However, the MartenitsaVoting::announceWinner function does not check if there are no votes, which a participant can receive a HealthToken even if there are no votes for the winner's NFT, and become the winner.

Impact

A participant can become a winner of the event and receive a HealthToken even if there are no votes for the winner's NFT, leading to unfair results in voting events.

Tools Used

Manual Review

Proof of Code

Add this test to MartenitsaVoting.t.sol:

PoC
function testIfThereAreNoVotes() 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();

    // Announcing winner
    vm.warp(block.timestamp + 1 days + 1);
    vm.recordLogs();
    voting.announceWinner();

    console.log("Chasy's Health Token balance: ", healthToken.balanceOf(chasy));
    console.log("Jack's Health Token balance: ", healthToken.balanceOf(jack));

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

As we can see from the logs of this test the HealthToken balance of Chasy is 1. Jack's HealthToken balance is 0, because he is not the winner.

Recommendations

Consider adding a require statement to check if maxVotes, which is declared in Martenitsa::announceWinner function, is greater than 0:

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

+       require(maxVotes > 0, "There are no votes");

        list = _martenitsaMarketplace.getListing(winnerTokenId);
        _healthToken.distributeHealthToken(list.seller, 1);

        emit WinnerAnnounced(winnerTokenId, list.seller);
    }