- High: 3
- Medium: 3
- Low: 3
https://www.codehawks.com/report/cluseb1bf0001s4tjl2rzajup
Final rank: #24
MartenitsaToken:updateCountMartenitsaTokensOwner user can update the count of martenitsaTokens without any restrictions (validated)
in MartenitsaToken:updateCountMartenitsaTokensOwner users can update the count of martenitsaTokens without any restrictions. Then they will be able to collect as many HealthTokens as they want.
By updating the count of martenitsaTokens without any restriction users will be able to mint HealthToken indefinitely.
- Update count of martenitsaTokens with
MartenitsaToken:updateCountMartenitsaTokensOwner - Collect the number of HealthTokens in proportion to the number of (fake) tokens you have (1 HealthToken for 3 fake MartenitsaTokens) by calling
MartenitsaMarketplace:collectReward.
In this test we will collect 3 HealthToken while we don't have any martinetsaToken.
function testVulnCollectReward() public {
vm.startPrank(bob);
martenitsaToken.updateCountMartenitsaTokensOwner(bob, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(bob, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(bob, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(bob, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(bob, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(bob, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(bob, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(bob, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(bob, "add");
marketplace.collectReward();
assert(healthToken.balanceOf(bob) == 3 * 10 ** 18);
vm.stopPrank();
}HealthToken can be easily minted and user can participate to an event without owning any martenitsaToken
manuel review
MartenitsaToken:updateCountMartenitsaTokensOwner must be only called by MartenitsaMarketplace contract using internal visibility specifier instead of external. Therefore you will have to refactor correctly MartenitsaMarketplace contract.
MartenitsaVoting time assignment of duration variable not correct. in MartenitsaVoting developers have assigned the variable duration 1 days as value instead of the equivalent of 1 day in block time
In MartenitsaVoting developers have assigned the variable duration 1 days as value which is equivalent to 86,400 seconds (24 hours x 60 minutes x 60 seconds).
uint256 public duration = 1 days;Duration variable is used in MartenitsaVoting:voteForMartenitsa and MartenitsaVoting:announceWinner in require control structure of these two functions
/**
* @notice Function to vote for martenitsa of the sale list.
* @param tokenId The tokenId of the martenitsa.
*/
function voteForMartenitsa(uint256 tokenId) external {
require(!hasVoted[msg.sender], "You have already voted");
require(!martenitsaToken.isProducer(msg.sender), "You are producer and not eligible for voting!");
console.log("startVoteTime: %d + duration: %d", startVoteTime, duration);
require(startVoteTime != 0 && block.timestamp < startVoteTime + duration, "The voting is no longer active");
list = _martenitsaMarketplace.getListing(tokenId);
require(list.forSale, "You are unable to vote for this martenitsa");
hasVoted[msg.sender] = true;
voteCounts[tokenId] += 1;
_tokenIds.push(tokenId);
}
/**
* @notice Function to announce the winner of the voting. The winner receive 1 HealthToken.
*/
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);
}The duration time of voting will not be 1 days but 14 days because currently in the Ethereum blockchain, the average time to mine a block is typically around 13-15 seconds.
Based on an average block time of 14 seconds:
- There are 86,400 seconds in a day (24 hours x 60 minutes x 60 seconds).
- Dividing 86,400 seconds by 14 seconds per block gives the approximate number of blocks mined in one day.
So Approximately 6,171 blocks are mined each day on the Ethereum blockchain, based on an average block time of 14 seconds
The voting period will last 14 days instead of 1 day which will compromise the announcement of the winner.
Manuel review
MartenitsaVoting hardcode voting duration to be compliant with blocktimestamp calcul
uint256 public duration = 6171;Or use a function to calculate dynamically the blocktime average and refactor
/**
* @notice Function to start the voting.
*/
function startVoting() public onlyOwner {
startBlock = block.number;
startVoteTime = block.timestamp;
while (startBlock == block.number) {} // wait for the next block
duration = calculateAverageBlockTime(block.number);
startVoteTime += 1;
emit Voting(startVoteTime, duration);
}
/**
* @notice Function to calculate the average blocktime
*/
function calculateAverageBlockTime(uint256 endBlock) public view returns (uint256) {
require(endBlock > startBlock, "End block must be greater than start block");
uint256 endTime = block.timestamp; // Assume current block is the end block for simplicity
uint256 elapsedTime = endTime - startVoteTime;
uint256 blocksCount = endBlock - startBlock;
return elapsedTime / blocksCount;
}MartenitsaVoting:announceWinner don't manage if there is a tie at the end of the voting period. (validated as medium)
MartenitsaVoting:announceWinner don't manage if there is a tie at the end of the voting period, if there is a tie, the winner will be the one with the lowest tokenID
In MartenitsaVoting:announceWinner the for loop iterate on the length of _tokenIDs array and never check if MartenitsaTokenId mapping has an equality regarding the number of votes.
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];
}
}In the case of a tie between participants, selection of the winner is unfair and can lead to user disinterest in the voting system
Manuel review
MartenitsaVoting:announceWinner must be refactored and the best way to get a real randomness for the draw consider to use chainlink VFR
import "@chainlink/contracts/src/v0.8/VRFConsumerBase.sol";Add VRFConsumerBase as inheritance
contract VotingContract is VRFConsumerBase, Ownnable {...}Modifiy the constructor and add some news state variables
bytes32 internal keyHash;
uint256 internal fee;
uint256[] private potentialWinners;
// Parameters for the Goerli Testnet
constructor(address marketplace, address healthToken, address _martenitsaToken)
Ownable(msg.sender)
VRFConsumerBase(
0x8FbB18354d37f7A587C60f1364b8aA2a05f69B90, // VRF Coordinator for Goerli
0x326C977E6efc84E512bB9C30f76E30c160eD06FB // LINK Token for Goerli
)
{
_martenitsaMarketplace = MartenitsaMarketplace(marketplace);
_healthToken = HealthToken(healthToken);
martenitsaToken = IMartenitsaToken(_martenitsaToken);
keyHash = 0x0476f9e5d797756e0f243a643e406a30e46e83c480a91cd96a1769f9c22daa60;
fee = 0.1 * 10 ** 18; // 0.1 LINK
owner = msg.sender; // Setting the owner to the deployer
}Refactor MartenitsaVoting:announceWinner
function announceWinner() external onlyOwner {
require(block.timestamp >= startVoteTime + duration, "The voting is still active");
require(LINK.balanceOf(address(this)) >= fee, "Not enough LINK - please fill contract with LINK");
uint256 maxVotes = 0;
delete potentialWinners; // Clear previous data
for (uint256 i = 0; i < _tokenIds.length; i++) {
uint256 currentVotes = voteCounts[_tokenIds[i]];
if (currentVotes > maxVotes) {
maxVotes = currentVotes;
delete potentialWinners;
potentialWinners.push(_tokenIds[i]);
} else if (currentVotes == maxVotes) {
potentialWinners.push(_tokenIds[i]);
}
}
requestRandomness(keyHash, fee);
}Add chainlink veritable randomness function
function fulfillRandomness(bytes32 requestId, uint256 randomness) internal override {
uint256 randomResult = randomness % potentialWinners.length;
uint256 winnerTokenId = potentialWinners[randomResult];
list = _martenitsaMarketplace.getListing(winnerTokenId);
_healthToken.distributeHealthToken(list.seller, 1);
emit WinnerAnnounced(winnerTokenId, list.seller);
}MartenitsaMarketplace:collectReward in a particular scenario amountRewards can't be correct because _collectedRewards mapping isn't reset if users sell at least 3 martenitsa token. (validated as high)
MartenitsaMarketplace:collectReward in a particular scenario amountRewards can't be correct because _collectedRewards mapping isn't reset if users sell at least 3 martenitsa token during an event and rebuy 3 others later, at this time user won't be able to claim his new healthtoken.
If a user first buys 3 martenitsa tokens, claims his healthtoken, sells his 3 martenitsa tokens during an event and later buys 3 new martenitsa tokens, he will no longer be able to claim a new health token because in MartenitsaMarketplace:collectReward line 104 of MartenitsaMarketplace contract the _collectedRewards is subtracted from the amountRewards and as mapping(address => uint256) private _collectedRewards; mapping is not reset to zero after the first 3 tokens have been sold, the amountRewards will be equal to zero instead of 1.
User won't receive his HealthToken despite having 3 new martenitsa tokens
Manuel review
Track the number of sales per user and decrement by 1 `mapping(address => uint256) private _collectedRewards; every 3 sales of martenitsa token per user.
MartenitsaVoting:voteForMartenitsa producer can vote for himself during a vote event. After listing any producer is able to vote for his martenitsa token.
voteForMartenitsa function don't check if the caller is a producer as demonstrated in the test bellow just after listing his token chasy who is a producer is able to vote for his martenitsa token.
function testProducerVoteForMartenitsa() public listMartenitsa {
vm.prank(chasy);
voting.voteForMartenitsa(0);
assert(voting.hasVoted(chasy) == true);
assert(voting.voteCounts(0) == 1);
}Vote system can be unfair because producer can vote for its creations.
Manuel review
Check if caller is a producer if yes revert the transaction, add an interface to access to isProducer and refactor the constructor of MartenitsaVoting contract
interface IMartenitsaToken {
function isProducer(address producer) external view returns (bool);
}
constructor(address marketplace, address healthToken, address _martenitsaToken) Ownable(msg.sender) {
_martenitsaMarketplace = MartenitsaMarketplace(marketplace);
_healthToken = HealthToken(healthToken);
martenitsaToken = IMartenitsaToken(_martenitsaToken);
}
/**
* @notice Function to vote for martenitsa of the sale list.
* @param tokenId The tokenId of the martenitsa.
*/
function voteForMartenitsa(uint256 tokenId) external {
require(!hasVoted[msg.sender], "You have already voted");
require(!martenitsaToken.isProducer(msg.sender), "You are producer and not eligible for voting!");
require(block.timestamp < startVoteTime + duration, "The voting is no longer active");
list = _martenitsaMarketplace.getListing(tokenId);
require(list.forSale, "You are unable to vote for this martenitsa");
hasVoted[msg.sender] = true;
voteCounts[tokenId] += 1;
_tokenIds.push(tokenId);
}Reentrancy in MartenitsaMarketplace.buyMartenitsa(uint256) external calls before finalizing state. The function makes external calls (martenitsaToken.updateCountMartenitsaTokensOwner) to update token ownership counts before it has completed updating the state of the contract.
The listing corresponding to the token ID is deleted after the external calls. If reentrancy occurs, the function might operate on a state that has already been changed by the reentrant call.
martenitsaToken.updateCountMartenitsaTokensOwner(buyer, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(seller, "sub");
// Clear the listing
delete tokenIdToListing[tokenId];This could potentially allow reentrancy if the external contract called (martenitsaToken) is tricked into calling back into buyMartenitsa.
slither
To prevent reentrancy attacks, you can apply the following techniques:
Checks-Effects-Interactions Pattern: Always update your contract's state (checks and effects) before calling external contracts (interactions). In buyMartenitsa, this means you should handle the deletion of the listing and any updates to internal state variables before making any external calls.
Use Reentrancy Guards: Implement a reentrancy guard, a simple state variable that can prevent a function from being called again while it's still executing. OpenZeppelin provides a ReentrancyGuard contract that you can use to secure your functions.
pragma solidity ^0.8.21;
import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
contract MartenitsaMarketplace is Ownable, ReentrancyGuard {
// Previous contract code...
function buyMartenitsa(uint256 tokenId) external payable nonReentrant {
Listing storage listing = tokenIdToListing[tokenId];
require(listing.forSale, "Token is not listed for sale");
require(msg.value >= listing.price, "Insufficient funds");
address seller = listing.seller;
address buyer = msg.sender;
uint256 salePrice = listing.price;
// Update the listing before making external calls
delete tokenIdToListing[tokenId];
// Perform external calls
martenitsaToken.updateCountMartenitsaTokensOwner(buyer, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(seller, "sub");
emit MartenitsaSold(tokenId, buyer, salePrice);
// Safe transfer of funds
(bool sent,) = seller.call{value: salePrice}("");
require(sent, "Failed to send Ether");
// Transfer the token to the buyer
martenitsaToken.safeTransferFrom(seller, buyer, tokenId);
}
// Remainder of contract...
} MartenitsaToken::createMartenitsa design @param is not properly checked, producer can create a martenitsa token with an empty string as design or with design without any meaning (validated and selected for report)
In MartenitsaToken::createMartenitsa design @param is not properly checked, so a producer can create a martenitsa token with a whitespace as design (It holds a specific ASCII value which is 32) or with a design without any meaning.
The require control structure (L37 of MartenitsaToken.sol) does not correctly control the "Design" input parameter.
function testCreateMartenitsaCalledWithDesignEqualZero() public {
vm.prank(jack);
vm.expectRevert();
martenitsaToken.createMartenitsa(" ");
}require(bytes(design).length > 0, "Design cannot be empty");Martenitsa token can be created with an empty string as design or with a design without any meaning.
Manual review
Create a custom error based on your check DesignToBytes == 0 and DesignToBytes is checks against the hexadecimal values of common whitespace characters: 0x20 - Space 0x5f - Horizontal Tab
So whitespace and horzontal tab won't be accepted as design character but you can add more design rules in the if statement if you decide to authorize only some specific design.
// Custom errors
error MartenitsaToken__DesignLengthIsEmpty();
error MartenistsaToken__IsAWhitespace();
/**
* @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!");
bytes memory designToBytes = bytes(design);
if (designToBytes.length == 0) {
revert MartenitsaToken__DesignLengthIsEmpty(); // Consider an empty string as not only whitespace
}
for (uint256 i = 0; i < designToBytes.length; i++) {
if (designToBytes[i] == 0x20 || designToBytes[i] == 0x5f) {
revert MartenistsaToken__IsAWhitespace();
}
}
uint256 tokenId = _nextTokenId++;
tokenDesigns[tokenId] = design;
countMartenitsaTokensOwner[msg.sender] += 1;
emit Created(msg.sender, tokenId, design);
_safeMint(msg.sender, tokenId);
}MartenitsaVoting:voteForMartenitsa user can vote even startvoting is not started from the genesis block to the 86400 blocks.
MartenitsaVoting:voteForMartenitsa user can vote even startvoting is not started from the genesis block to the 86400 blocks, what represent approximatively 14 days.
In MartenitsaVoting contract even if startVoteTime == 0 ; instead of startVoteTime = block.timestamp; it's possible for users to pass the require control structure in MartenitsaVoting:voteForMartenitsa during the first 86400 blocks (~14days).
/**
* @notice Function to vote for martenitsa of the sale list.
* @param tokenId The tokenId of the martenitsa.
*/
function voteForMartenitsa(uint256 tokenId) external {
require(!hasVoted[msg.sender], "You have already voted");
require(block.timestamp < startVoteTime + duration, "The voting is no longer active");
list = _martenitsaMarketplace.getListing(tokenId);
require(list.forSale, "You are unable to vote for this martenitsa");
hasVoted[msg.sender] = true;
voteCounts[tokenId] += 1;
_tokenIds.push(tokenId);
}Users can vote during the first 14 days even if voting is not started
manuel review
Add a second check in require structure control in MartenitsaVoting:voteForMartenitsa
require(startVoteTime != 0 && block.timestamp < startVoteTime + duration, "The voting is no longer active");MartenitsaEvent:startEvent uint256 public eventEndTime can lead to a unconsiderer revert (through built-in overflow protection) during the execution of startEvent function.
MartenitsaEvent:startEvent uint256 public eventEndTime can lead to unconsiderer revert (through built-in overflow protection) during the execution of startEvent function.. uint256 public eventEndTime get the result of two uint256 variables.
lvalue eventStartTime and eventDuration are declared both as uint256 the result of adding these 2 lvalue can lead to unconsiderer revert (through built-in overflow protection) during the calcul of the eventEndTime lvalue, this is the operation performed on line 33 of the MartenitsaEvent.sol contract
Nota: From version 0.8.0 onward, Solidity includes built-in overflow and underflow protection by default. Arithmetic operations automatically revert if they result in an overflow or underflow.
uint256 public eventStartTime;
uint256 public eventDuration;
uint256 public eventEndTime; eventEndTime = eventStartTime + duration;Unconsiderer revert (through built-in overflow protection) can occur during the calcul of the eventEndTime, owner the owner may not understand that this is the reason for the revert
Manuel review
Using Custom Error Handling
error OverflowDetected(); function startEvent(uint256 duration) external onlyOwner {
eventStartTime = block.timestamp;
eventDuration = duration;
if (eventStartTime + duration < eventStartTime || eventStartTime + duration < duration) { // Check for overflow
revert OverflowDetected();
}
eventEndTime = eventStartTime + duration;
emit EventStarted(eventStartTime, eventEndTime);
}