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

Missing access control in `MartenitsaToken::updateCountMartenitsaTokensOwner`

paprikrumplikas

Summary

MartenitsaToken:: updateCountMartenitsaTokensOwner does not have access control: anyone can call this function and can alter the internal accounting of token counts for any address.

Vulnerability Details

MartenitsaToken::updateCountMartenitsaTokensOwner is supposed to update mapping(address => uint256) public countMartenitsaTokensOwner whenever a Marenitsa NFT is minted or changes owners. The mapping countMartenitsaTokensOwner is used for internal accounting and is supposed to reflect how many Martenitsa NFTs are owned by each address (kind of like the standard ERC721 function balanceOf()). However, since MartenitsaToken::updateCountMartenitsaTokensOwner does not have any access control and anybody can call it, this internal accounting can be manipulated.

Consider the following test that demonstrates this vulnerability:

Proof of Code
function testAnyoneCanCallUpdateCountMartenitsaTokensOwner() public {
address user = makeAddr("user");
​
// jack mints 1 NFT
vm.startPrank(jack);
martenitsaToken.createMartenitsa("bracelet");
vm.stopPrank();
​
uint256 userInternalBalance_before = martenitsaToken.getCountMartenitsaTokensOwner(user); // 0
uint256 jackInternalBalance_before = martenitsaToken.getCountMartenitsaTokensOwner(jack); // 1
​
vm.startPrank(user);
martenitsaToken.updateCountMartenitsaTokensOwner(user, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(jack, "sub");
vm.stopPrank();
​
uint256 userInternalBalance_after = martenitsaToken.getCountMartenitsaTokensOwner(user); // 1
uint256 jackInternalBalance_after = martenitsaToken.getCountMartenitsaTokensOwner(jack); // 0
​
// internal accounting has been manipulated
assert(userInternalBalance_after > userInternalBalance_before); // 1 > 0
assert(jackInternalBalance_after < jackInternalBalance_before); // 0 < 1
​
// internal accounting does not match reality
assert(userInternalBalance_after != martenitsaToken.balanceOf(user)); // 1 != 0
assert(jackInternalBalance_after != martenitsaToken.balanceOf(jack)); // 0 != 1
​
// an earlier state of internal accounting matches reality
assert(userInternalBalance_before == martenitsaToken.balanceOf(user)); // 0 = 0
assert(jackInternalBalance_before == martenitsaToken.balanceOf(jack)); // 1 = 1
}

Impact

  1. Internal accounting (martenitsaToken.countMartenitsaTokensOwner()) will not reflect true ownership status (martenitsaToken.balanceOf()).

  2. A malicious user can alter the internal accounting in a way that the contract will think it has a lot more Martenitsa NFTs than it actually does, and then (after meeting all the other requirements for rewards collection) can collect more healthToken rewards from MartenitsaMarketplace::collectRewards than it is entitled to.

Tools Used

Manual review, Foundry.

Recommendations

Consider the following options and select whichever matches your current and future requirements more appropriately:

  1. Replace the countMartenitsaTokensOwner with the the standard ERC721 balanceOf(), as the former appears to duplicate the functionality already provided by the latter (i.e. tracking the number of tokens owned by each address). In this case you can completely remove not only the mapping but also the updateCountMartenitsaTokensOwner function.
    (According to the protocol owner, this is the less favorable option, the protocol prefers to keep the internal accounting.)

  2. Implement access control for MartenitsaToken::updateCountMartenitsaTokensOwner by modifying MartenitsaToken.sol as follows:

+ address marketplaceAddress;
​
+ function setMarketplaceAddress(address _marketplace) external onlyOwner {
+ require(marketplaceAddress == address(0), "Marketplace already set"); // Optional: Ensure only set once
+ marketplaceAddress = _marketplace;
+ }
​
...
​
function updateCountMartenitsaTokensOwner(address owner, string memory operation) external {
+ require(marketplaceAddress != address(0), "Marketplace address is not set yet.");
+ require(msg.sender == marketplaceAddress, "Unauthorized call");
// existing logic
}
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.