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

`MartenitsaMarketplace::collectReward` Calculates the Already Collected `healthTokens` Incorrectly

4rdiii

[H-2] MartenitsaMarketplace::collectReward calculates the already paid healthTokens incorrectly

Description: The MartenitsaMarketplace::collectRewards function is designed to calculate the amount of healthTokens that have already been paid by a user. However, it appears to be calculating this amount incorrectly, leading to discrepancies in the rewards distribution and potentially allowing users to collect more rewards than they are entitled to.

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; // this line is wrong it should be += instead of =
healthToken.distributeHealthToken(msg.sender, amountRewards);
}
}

Impact: This issue can lead to significant financial losses for the marketplace and its users, as users may collect more rewards than they have earned. It can also undermine trust in the marketplace's reward system, affecting user engagement and the overall health of the ecosystem.

Proof of Concept: Exploit Steps:

  1. User buys 3 nfts and calls the collectReward => gets 1 healthToken, has 3 nfts and 1 tokens now ✅

  2. User buys another 3 nfts and calls the collectReward again => gets 1 healthToken, has 6 nfts and 2 tokens now ✅

  3. User buys another 3 nfts and calls the collectReward again => gets 2 healthToken, has 9 nfts and 4 tokens now ❌

Here is the proof of concept just add this to the test suit:

function make3NftsAndDealThemToBob(
string memory name1,
string memory name2,
string memory name3,
uint256 tokenIdCount
) internal {
//helper function
vm.startPrank(jack);
martenitsaToken.createMartenitsa(name1);
martenitsaToken.createMartenitsa(name2);
martenitsaToken.createMartenitsa(name3);
martenitsaToken.approve(address(marketplace), tokenIdCount + 0);
martenitsaToken.approve(address(marketplace), tokenIdCount + 1);
martenitsaToken.approve(address(marketplace), tokenIdCount + 2);
marketplace.makePresent(bob, tokenIdCount + 0);
marketplace.makePresent(bob, tokenIdCount + 1);
marketplace.makePresent(bob, tokenIdCount + 2);
vm.stopPrank();
}
​
function testCollectRewardsCalculatesIncorrectly() public {
//bob gets 3 nfts via buying or present
make3NftsAndDealThemToBob("1", "2", "3", 0);
vm.prank(bob);
marketplace.collectReward();
assertEq(martenitsaToken.getCountMartenitsaTokensOwner(bob), 3);
assertEq(healthToken.balanceOf(bob), 1 ether);
//bob gets 3 nfts via buying or present
make3NftsAndDealThemToBob("4", "5", "6", 3);
vm.prank(bob);
marketplace.collectReward();
assertEq(martenitsaToken.getCountMartenitsaTokensOwner(bob), 6);
assertEq(healthToken.balanceOf(bob), 2 ether);
//bob gets 3 nfts via buying or present
make3NftsAndDealThemToBob("7", "8", "9", 6);
vm.prank(bob);
marketplace.collectReward();
assertEq(martenitsaToken.getCountMartenitsaTokensOwner(bob), 9);
assertEq(healthToken.balanceOf(bob), 4 ether); // he gets 2 new healthTokens instead of 1
}

Recommended Mitigation: Review and Correct the Calculation Logic: Thoroughly review the logic used in MartenitsaMarketplace::collectReward to calculate the amount of healthTokens paid by a user. Ensure that the calculation accurately reflects the user's contributions and does not overestimate the amount.

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; // this line is wrong it should be += instead of =
+ _collectedRewards[msg.sender] += amountRewards;
healthToken.distributeHealthToken(msg.sender, amountRewards);
}
}
Updates

Lead Judging Commences

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

_collectedRewards is not updated correctly

Support

FAQs

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