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

State management and access control issues due to inappropriate inheritance in `MartenitsaEvent`

paprikrumplikas

Summary

The MartenitsaEvent contract currently from MartenitsaToken, which unnecessarily exposes token management functionalities. This leads to shared state variables that can result in synchronization and access control issues across the codebase. Such an inheritance structure contradicts the principle of separation of concerns between event management and token management.

Vulnerability Details

MartenitsaEvent is supposed to contain event management functionalities, which is a completely separate concern from the NFT token management and producer management functionalities provided by MartenitsaToken.

However, MartenitsaEvent extends MartenitsaToken via inheritance, inappropriately merging two disparate concerns. This inheritance structure gives MartenitsaEvent unnecessary access to the internal mechanisms (e.g. NFT minting method) and state variables (e.g. producer lists) of MartenitsaToken.

This setup causes several problems, probably the biggest of which is that both contracts maintain separate and uncoordinated internal states regarding which addresses are recognized as producers. The test below demonstrates one such state inconcistency between the 2 contacts (one contract recognizes an address as producer, the other not).

Proof of Code
function testInconsistentProducerStates() public {
// MartenitsaToken.sol recognizes Jack as producer
assert(martenitsaToken.isProducer(jack) == true);
// MartenitsaEvent.sol DOES NOT recognize Jack as producer
assert(martenitsaEvent.isProducer(jack) == false);
​
// accordingly, Jack can join an event even though as a producer he is not supposed to
deal(address(healthToken), jack, 1 ether);
martenitsaEvent.startEvent(1 days);
vm.startPrank(jack);
healthToken.approve(address(martenitsaEvent), 1 ether);
martenitsaEvent.joinEvent();
vm.stopPrank();
assert(martenitsaEvent.getParticipant(jack) == true);
}

Impact

  • State Confusion: both contracts will maintain separate and uncoordinated internal states regarding which addresses are recognized as producers. I.e. addresses denoted as producers by MartenitsaToken will not be recognized as producers by MartenitsaEvent and vice versa.

  • Access Control Issues: state confusion causes access control issues, e.g.

-- Addresses recognized as producers by MartenitsaToken are able to join an event via MartenitsaEvent::joinEvent, even though as producers they are not supposed to. This is because MartenitsaEvent::joinEvent only checks its own internal state regarding producer lists:

require(!isProducer[msg.sender], "Producers are not allowed to participate");

-- Addresses recognized as producers by MartenitsaEvent (during the duration of an event) are able to collect rewards via MartentisaMartkerplace::collectRewards even during an event, although they are not supposed to. This is because MartentisaMartkerplace::collectRewards checks only MartenitsaToken's internal state regarding producer lists:

require(!martenitsaToken.isProducer(msg.sender), "You are producer and not eligible for a reward!");

-- Addresses recognized as producers by MartenitsaEvent (during the duration of an event) are not able to create Martinetsa NFTs via MartinetsaToken::createMartinetsa, only via MartinetsaEvent::createMartinetsa which creates further state inconsistencies and accounting issues.

  • Function Exposure: MartenitsaEvent gains access to NFT-specific functions that are irrelevant to its primary role of managing events. This not only adds unnecessary complexity to MartenitsaEvent but also increases the attack surface of the codebase.

Tools Used

Manual review, Foundry.

Recommendations

MartenitsaEvent should not inherit from MartenitsaToken. Instead, it should contain an instance of MartenitsaToken to allow explicit and controlled use of its functionalities. This change will limit MartenitsaEvent to necessary interactions with MartenitsaToken and prevent inadvertent exposure of token management features.

Consider the following modifications as a rough starting point for a fix. Note that the bug at hand is severe and fixing it requires rethinking the protocol design, thorough testing, and preferably another round of audit.

Implemented Fix Suggestion
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.21;
​
import {HealthToken} from "./HealthToken.sol";
import {MartenitsaToken} from "./MartenitsaToken.sol";
​
- contract MartenitsaEvent is MartenitsaToken {
+ contract MartenitsaEvent {
+ MartenitsaToken private _martenitsaToken;
HealthToken private _healthToken;
​
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 martenitsaToken, address healthToken) onlyOwner {
+ _martenitsaToken = MartenitsaToken(martenitsaToken);
_healthToken = HealthToken(healthToken);
}
​
/**
* @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], "Producers are not allowed to participate");
+ require(!_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);
​
}
​
/**
* @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;
+ _martenitsaToken.removeProducer(participants[i]);
+ // note that `martenitsaToken.producers` also has to be updated, but that is considered a separate bug
}
}
​
/**
* @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.
*/
function _addProducer(address _producer) internal {
- isProducer[_producer] = true;
- producers.push(_producer);
+ // note: creating an array every time just to add a single producer might be less efficient.
+ // instead, consider adding a method in MartenitsaToken
+ address[] memory newProducers = new address[](1);
+ newProducers[0] = _producer;
+ _martenitsaToken.setProducers(newProducers);
}
}
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.21;
​
import "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import "@openzeppelin/contracts/access/Ownable.sol";
​
contract MartenitsaToken is ERC721, Ownable {
uint256 private _nextTokenId;
address[] public producers;
​
+ address internal _eventContractAddress;
​
​
mapping(address => uint256) public countMartenitsaTokensOwner;
mapping(address => bool) public isProducer;
mapping(uint256 => string) public tokenDesigns;
​
event Created(address indexed owner, uint256 indexed tokenId, string indexed design);
​
constructor() ERC721("MartenitsaToken", "MT") Ownable(msg.sender) {}
​
​
​
+ function setEventContractAddress(address _eventAddress) external onlyOwner {
+ require(_eventContractAddress == address(0), "Event contract address already set"); // Optional: Ensure only set once
+ _eventContractAddress = _eventAddress;
+ }
​
/**
* @notice Function to set producers.
* @param _producersList The addresses of the producers.
*/
function setProducers(address[] memory _producersList) public onlyOwner{
for (uint256 i = 0; i < _producersList.length; i++) {
+ if(!isProducer[_producersList[i]]){
isProducer[_producersList[i]] = true;
producers.push(_producersList[i]);
+ }
}
}
​
+ // note: this is costly... this should be optimized
+ function removeProducer(address _producer) public {
+ require(msg.sender == _eventContractAddress);
+ require(isProducer[_producer], "Not a producer");
+ isProducer[_producer] = false;
+ for (uint256 i = 0; i < producers.length; i++) {
+ if (producers[i] == _producer) {
+ producers[i] = producers[producers.length - 1];
+ producers.pop();
+ break;
+ }
+ }
+ }
​
/**
* @notice Function to create a new martenitsa. Only producers can call the function.
* @param design The type (bracelet, necklace, Pizho and Penda and other) of martenitsa.
*/
function createMartenitsa(string memory design) external {
require(isProducer[msg.sender], "You are not a producer!");
require(bytes(design).length > 0, "Design cannot be empty");
uint256 tokenId = _nextTokenId++;
tokenDesigns[tokenId] = design;
countMartenitsaTokensOwner[msg.sender] += 1;
​
emit Created(msg.sender, tokenId, design);
​
_safeMint(msg.sender, tokenId);
}
​
/**
* @notice Function to get the design of a martenitsa.
* @param tokenId The Id of the MartenitsaToken.
*/
function getDesign(uint256 tokenId) external view returns (string memory) {
require(tokenId < _nextTokenId, "The tokenId doesn't exist!");
return tokenDesigns[tokenId];
}
​
/**
* @notice Function to update the count of martenitsaTokens for a specific address.
* @param owner The address of the owner.
* @param operation Operation for update: "add" for +1 and "sub" for -1.
*/
function updateCountMartenitsaTokensOwner(address owner, string memory operation) external {
if (keccak256(abi.encodePacked(operation)) == keccak256(abi.encodePacked("add"))) {
countMartenitsaTokensOwner[owner] += 1;
} else if (keccak256(abi.encodePacked(operation)) == keccak256(abi.encodePacked("sub"))) {
countMartenitsaTokensOwner[owner] -= 1;
} else {
revert("Wrong operation");
}
}
​
/**
* @notice Function to get the count of martenitsaTokens for a specific address.
* @param owner The address of the owner.
*/
function getCountMartenitsaTokensOwner(address owner) external view returns (uint256) {
return countMartenitsaTokensOwner[owner];
}
​
/**
* @notice Function to get the list of addresses of all producers.
*/
function getAllProducers() external view returns (address[] memory) {
return producers;
}
​
/**
* @notice Function to get the next tokenId.
*/
function getNextTokenId() external view returns (uint256) {
return _nextTokenId;
}
}

Note that a better, more elegant and simpler fix might be the following:

  • remove the inheritance

  • do not manipulate the state variables of MartenitsaMarketplace to grant producer privilages to event participants for the duration of an event, but instead:

-- completely separate participant and producer statuses, and use only the participants array and _participants mapping for participants (and producer state variables only for real producers set by MartenitsaMarketplace),

-- in all 5 contracts, whereever an action is gated for producers or non-producers, add a similar gate to the participants by referencing the particpants state variable of MartenitsaEvent. (For this to work, importing MartenitsaEventwill be needed in all affected contracts.)

This solution completely separates the handling of producers from participants. As such, it has the additional benefit that whenever an event ends, and the producer-like privilages of participants have to be revoked, it is enough to simply reset the participants array and _participants mapping.

Updates

Lead Judging Commences

bube Lead Judge
over 1 year ago
bube Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

MartenitsaToken and MartenitsaEvent have different addresses

Support

FAQs

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