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

Missing access control on `MartenitsaToken::updateCountMartenitsaTokensOwner`, leads to anyone updating others holding count of MartenitsaToken and cause DoS in buying token

shikhar229169

Summary

updateCountMartenitsaTokensOwner function lags necessary access control to only allow MarketPlace contract to update the one's holding count of MartenitsaToken, which allows anyone to update others holding count of Martenitsa Token and will also allow producers to not be able to sell their Martenitsa Token under the scenario when the countMartenitsaTokensOwner is dropped down to 0.

Vulnerability Details

The vulnerability is present in the MartenitsaToken::updateCountMartenitsaTokensOwner function as it lags necessary access control check on it thus allowing anyone to update countMartenitsaTokensOwner.
The function takes in the owner for which to either increment or decrement the count of martenitsa token count, and as it lags the access control on it anyone can call it and update other's token counts.

@> 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");
}
}

Impact

  • Incorrect result by function MartenitsaToken::getCountMartenitsaTokensOwner.

  • By calling the function updateCountMartenitsaTokensOwner for a producer who has their listing in the marketplace, the attacker will be able to bring the count of their token holding to 0, thus making it impossible for the producer to sell their MartenitsaToken as when a user buys a NFT, the marketplace will be decreasing the count of the producer's token and as their token count of manipulated to 0, the operation will revert. Thus producer will face difficulty in selling their token and for the user to buy the producer's token.

  • MartenitsaMarketplace::collectReward uses Martenitsa::getCountMartenitsaTokensOwner to query for the total count of token a address owns. As countMartenitsaTokensOwner can be updated by anyone, the collectReward function will get incorrect output, thus allows anyone to mint unlimited health token.

PoC

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

Run the test:

forge test --mt testMissingAccessControl_On_updateCountMartenitsaTokensOwner_MessesUpEverything
function testMissingAccessControl_On_updateCountMartenitsaTokensOwner_MessesUpEverything() public createMartenitsa {
// chasy producer now has 1 martenitsa token
​
uint256 tokenId = martenitsaToken.getNextTokenId() - 1;
​
// they list it for sell on the marketplace
vm.startPrank(chasy);
martenitsaToken.approve(address(marketplace), tokenId);
marketplace.listMartenitsaForSale(tokenId, 1 ether);
vm.stopPrank();
​
// chasy has 1 token
assert(martenitsaToken.getCountMartenitsaTokensOwner(chasy) == 1);
​
// a user maliciously decrements the token count of chasy
vm.prank(makeAddr("attacker"));
martenitsaToken.updateCountMartenitsaTokensOwner(chasy, "sub");
​
// now the count is 0
assert(martenitsaToken.getCountMartenitsaTokensOwner(chasy) == 0);
​
​
// kit tries to buy the martenitsa token
hoax(makeAddr("kit"), 10 ether);
​
vm.expectRevert();
marketplace.buyMartenitsa{value: 1 ether}(tokenId);
}

Tools Used

Manual Review, Foundry Unit Test

Recommendations

Add an access control to only allow MartenitsaMarketplace contract to update countMartenitsaTokensOwner by calling updateCountMartenitsaTokensOwner.

diff --git a/src/MartenitsaToken.sol b/src/MartenitsaToken.sol
index 040ce8a..fa84a6c 100644
--- a/src/MartenitsaToken.sol
+++ b/src/MartenitsaToken.sol
@@ -8,6 +8,7 @@ contract MartenitsaToken is ERC721, Ownable {
uint256 private _nextTokenId;
address[] public producers;
+ address public martenitsaMarketplace;
mapping(address => uint256) public countMartenitsaTokensOwner;
mapping(address => bool) public isProducer;
@@ -54,12 +55,17 @@ contract MartenitsaToken is ERC721, Ownable {
return tokenDesigns[tokenId];
}
+ function setMartenitsaMarketplace(address _martenitsaMarketplace) public onlyOwner {
+ martenitsaMarketplace = _martenitsaMarketplace;
+ }
+
/**
* @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 {
+ require(msg.sender == martenitsaMarketplace, "Only MartenitsaMarketplace is allowed to perform updations");
if (keccak256(abi.encodePacked(operation)) == keccak256(abi.encodePacked("add"))) {
countMartenitsaTokensOwner[owner] += 1;
} else if (keccak256(abi.encodePacked(operation)) == keccak256(abi.encodePacked("sub"))) {
Updates

Lead Judging Commences

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

Missing access control

Support

FAQs

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