Inheritance Instead of Importing ERC721 Token Contract in `MartenitsaEvent` Causes Previous Producers and Produced NFTs to be Ignored During Event

Severity

High Risk

Relevant GitHub Links

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

Description: the MartenitsaEvent contract inherits the MartenitsaToken contract so it itself is an ERC721 token which introduces its own state variables such as _nextTokenId , producers, countMartenitsaTokensOwner, isProducer, tokenDesigns which ignores previous values stored in the main MartenitsaToken contract.

@> contract MartenitsaEvent is MartenitsaToken {
       HealthToken private _healthToken;
       uint256 public eventStartTime;
    .
    .
    .
}

This approach is not a design choice, as evidenced by the deployment script (BaseTest), where a new MartenitsaToken is used as the ERC721 token instead of MartenitsaEvent. Additionally, the marketplace uses the Token contract, not the Event one, as its token input.

BaseTest contract

    contract BaseTest is Test {
    
        MartenitsaToken public martenitsaToken;
        HealthToken public healthToken;
        MartenitsaMarketplace public marketplace;
        MartenitsaVoting public voting;
        MartenitsaMarketplace.Listing list;
        MartenitsaEvent public martenitsaEvent;
        .
        .
        .
        function setUp() public {
            jack = makeAddr("jack");
            chasy = makeAddr("chasy");
            bob = makeAddr("bob");
            producers.push(jack);
            producers.push(chasy);

            vm.deal(bob, 5 ether);

@>          martenitsaToken = new MartenitsaToken();
            healthToken = new HealthToken();
@>          marketplace = new MartenitsaMarketplace(address(healthToken), address(martenitsaToken));
            voting = new MartenitsaVoting(address(marketplace), address(healthToken));
@>          martenitsaEvent = new MartenitsaEvent(address(healthToken));
            
            healthToken.setMarketAndVotingAddress(address(marketplace), address(voting));
            martenitsaToken.setProducers(producers);
      
        }
        .
        .
        .
}

Impact: This issue leads to several problems for the event contract:

MartenitsaEvent::joinEvent uses the MartenitsaEvent::isProducer mapping to check if the user is not a producer. However, this mapping is empty, and previous producers are in MartenitsaToken::isProducer. If producers buy a HealthToken, they can enter the event, which is not crucial for project functionality but is against documentation.
MartenitsaEvent::joinEvent uses the MartenitsaEvent::_addProducer() function to add users as producers during event time, allowing them to create new NFTs. However, users cannot mint NFTs if they call MartenitsaToken::createMartenitsa; it will fail because they are not a producer in this contract. If they accidentally call MartenitsaEvent::createMartenitsa, there will be two instances of the ERC721 contract with the same token IDs.
if users happen to create any tokens in MartenitsaEvent::createMartenitsa, they wont be able to list them to market during the event period.

Proof of Concept: To prove this concept, add the following test to the existing test suite:

Event starts.
User gets 3 NFTs, mints a HealthToken.
User joins the event and becomes a producer.
User mints new NFTs in MartenitsaEvent::createMartenitsa but is unable to list it to MarketPlace.
User is unable to mint any NFTs in MartenitsaToken::createMartenitsa.

    function testInheritenceIsWrong2() public activeEvent eligibleForReward {
        // activeEvent and eligibleForReward ensures steps 1 and 2
        // so bob has 3 nfts now and the event is started
        vm.startPrank(bob);
        marketplace.collectReward(); // mints healthtoken
        healthToken.approve(address(martenitsaEvent), 1 ether); 
        martenitsaEvent.joinEvent(); // joins the event and becomes producer step 3
        //creating in token contract fails / step 5
        vm.expectRevert("You are not a producer!");
        martenitsaToken.createMartenitsa("bracelets");
        assertEq(martenitsaToken.balanceOf(bob), 3); // still has 3 original nfts in this contract

        //creating in event contract succeeds / step 4
        martenitsaEvent.createMartenitsa("bracelets");
        assertEq(martenitsaEvent.balanceOf(bob), 1); // just has 1 new nft in this contract

        // bob is unable to list his newly created NFT
        vm.expectRevert("You are not a producer!");
        marketplace.listMartenitsaForSale(0, 1 ether);
    }

Recommended Mitigation: Instead of inheriting the Token contract, get the address as input and create it as a state variable in the constructor. This way, the ERC721 contract will be the same for all NFTs, whether created during or before the event. here is a possible recomendation:

-contract MartenitsaEvent is MartenitsaToken {
+contract MartenitsaEvent {

    HealthToken private _healthToken;
+   MartenitsaToken private _martenitsaToken;
    uint256 public eventStartTime;
    uint256 public eventDuration;
    uint256 public eventEndTime;
    uint256 public healthTokenRequirement = 10 ** 18;
    address[] public participants;

    mapping(address => bool) private _participants;

    event EventStarted(uint256 indexed startTime, uint256 indexed eventEndTime);
    event ParticipantJoined(address indexed participant);

-    constructor(address healthToken) onlyOwner {
+    constructor(address healthToken, address martenitsaToken) onlyOwner {

        _healthToken = HealthToken(healthToken);
+       _martenitsaToken = MartenitsaToken(martenitsaToken)
    }

    /**
     * @notice Function to start an event.
     * @param duration The duration of the event.
     */
    function startEvent(uint256 duration) external onlyOwner {
        eventStartTime = block.timestamp;
        eventDuration = duration;
        eventEndTime = eventStartTime + duration;
        emit EventStarted(eventStartTime, eventEndTime);
    }

    /**
     * @notice Function to join to event. Each participant is a producer during the event.
     * @notice The event should be active and the caller should not be joined already.
     * @notice Producers are not allowed to participate.
     */

    function joinEvent() external {
        require(block.timestamp < eventEndTime, "Event has ended");
        require(
            !_participants[msg.sender],
            "You have already joined the event"
        );
        require(
-           !isProducer[msg.sender],
+           !_martenitsaToken.isProducer(msg.sender),

            "Producers are not allowed to participate"
        );
        require(
            _healthToken.balanceOf(msg.sender) >= healthTokenRequirement,
            "Insufficient HealthToken balance"
        );

        _participants[msg.sender] = true;
        participants.push(msg.sender);
        emit ParticipantJoined(msg.sender);

        bool success = _healthToken.transferFrom(
            msg.sender,
            address(this),
            healthTokenRequirement
        );
        require(success, "The transfer is not successful");
-       _addProducer(msg.sender);
+       _martenitsaToken._addProducer(msg.sender);
    }

    /**
     * @notice Function to remove the producer role of the participants after the event is ended.
     */
    function stopEvent() external onlyOwner {
        require(block.timestamp >= eventEndTime, "Event is not ended");
        for (uint256 i = 0; i < participants.length; i++) {
            isProducer[participants[i]] = false;
        }
    }

    /**
     * @notice Function to get information if a given address is a participant in the event.
     */
    function getParticipant(address participant) external view returns (bool) {
        return _participants[participant];
    }

    /**
     * @notice Function to add a new producer.
     * @param _producer The address of the new producer.
     */
+   // remove this function from here and add it to Token Contract  
-   function _addProducer(address _producer) internal {
-       isProducer[_producer] = true;
-       producers.push(_producer);
-   }
}