MartenitsaMarketplace::getListing reverting due to token bought leads to winner of MartenitsaVoting never receiving their HealthToken reward

Severity

High Risk

Relevant GitHub Links

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

Summary

The MartenitsaMarketplace::getListing reverts in case the token is not for sale, therefore the martenitsa token which was on sale earlier when bought will get unlisted and getListing function will revert.

In MartenitsaVoting contract when the voting period is over, the winner is announced by owner, but if the token is unlisted from sale on the marketplace when a buyer buys it, the getListing function will revert and as a result of which the winner can never be announced and will not receive their HealthToken reward.

Vulnerability Details

The vulnerability is present in the MartenitsaVoting contract where it uses MartenitsaMarketplace::getListing function to get the address of producer associated with the winning martenitsa token id, but getListing function reverting upon the token getting unlisted when one buys it will lead to winner not getting their HealthToken reward and there will no winner announced as announceWinner function will revert due to revert from MartenitsaMarketplace.

    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

• Winner will never be announced.
• Winner will not receive their HealthToken reward from the MartenitsaVoting.

PoC

Add the test in the file: test/MartenitsaVoting.t.sol

Run the test:

forge test --mt test_WinnerTokenBeingBought_LeadsTo_RevertInAnnounceWinner

    function test_WinnerTokenBeingBought_LeadsTo_RevertInAnnounceWinner() public {
        // Martenitsa created and listed
        vm.startPrank(chasy);
        martenitsaToken.createMartenitsa("bracelet");
        martenitsaToken.approve(address(marketplace), 0);
        marketplace.listMartenitsaForSale(0, 1 wei);
        vm.stopPrank();

        // voting started by owner
        voting.startVoting();

        // bob votes for token 0
        vm.prank(bob);
        voting.voteForMartenitsa(0);

        // voting period ends
        vm.warp(block.timestamp + voting.duration());

        // bob buys the token 0
        vm.prank(bob);
        marketplace.buyMartenitsa{value: 1 wei}(0);

        // owner tries to announce winner
        vm.expectRevert("Token is not listed for sale");
        voting.announceWinner();
    }

Tools Used

Manual Review, Foundry Unit Test

Recommendations

When a token receives their first vote, store the seller address in the voting contract.

diff --git a/src/MartenitsaVoting.sol b/src/MartenitsaVoting.sol
index 992f49e..93a1f27 100644
--- a/src/MartenitsaVoting.sol
+++ b/src/MartenitsaVoting.sol
@@ -20,6 +20,8 @@ contract MartenitsaVoting is Ownable {
     //mapping user address -> if this address is already voted
     mapping(address => bool) public hasVoted;
 
+    mapping(uint256 tokenId => address candidate) public tokenIdToCandidate;
+
     event WinnerAnnounced(uint256 indexed winnerTokenId, address indexed winner);
     event Voting(uint256 indexed startTime, uint256 indexed duration);
 
@@ -46,6 +48,10 @@ contract MartenitsaVoting is Ownable {
         list = _martenitsaMarketplace.getListing(tokenId);
         require(list.forSale, "You are unable to vote for this martenitsa");
 
+        if (tokenIdToCandidate[tokenId] == address(0)) {
+            tokenIdToCandidate[tokenId] = list.seller;
+        }
+
         hasVoted[msg.sender] = true;
         voteCounts[tokenId] += 1; 
         _tokenIds.push(tokenId);
@@ -67,8 +73,7 @@ contract MartenitsaVoting is Ownable {
             }
         }
 
-        list = _martenitsaMarketplace.getListing(winnerTokenId);
-        _healthToken.distributeHealthToken(list.seller, 1);
+        _healthToken.distributeHealthToken(tokenIdToCandidate[winnerTokenId], 1);
 
         emit WinnerAnnounced(winnerTokenId, list.seller);
     }