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

Function `MartenitsaMarketplace::collectReward` fails to update the amount of collected rewards properly.

robertodf99

Summary

When users acquire enough martenitsa tokens they can claim the corresponding amount of health tokens by calling MartenitsaMarketplace::collectReward. This function updates the amount being claimed and it is discounted in future redemptions, however the way in which this value is updated is not correct.

Vulnerability Details

MartenitsaMarketplace::_collectedRewards stores how many health tokens a user has claimed, this value is updated after each function call as indicated below:

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;
healthToken.distributeHealthToken(msg.sender, amountRewards);
}
}

The number of health tokens to which the user is eligible is calculated as the substraction of the coefficient of the total number of owned martenitsas divided by the minimum required to claim one health token and the tokens claimed in the previous call. Since this update mechanism only takes into account the previous snapshot instead of the full history of claims, every health token acquired before the claim n-1 where n represents the current number of claims, is rewarded for free to the user.

Impact

Users have access to unlimited health tokens by just minting 6 martentitsas. The process can be described into more detail as follows:

  • bob acquires three martentisas.

  • bob claims his health token. MartenitsaMarketplace::_collectedRewards is updated to 1.

  • bob acquires another three martentitas.

  • bob claims another health token. MartenitsaMarketplace::_collectedRewards is updated to 1, ignoring the previously claimed health token.

  • Since MartenitsaMarketplace::_collectedRewards will only look at the previous snapshot, now the health token corresponding to claim n-2 will be rewarded for free in the next claim.
    See PoC below.

Code

Place this in MartenitsaMarketplace.t.sol.

function test_userCanClaimFreeHealthTokens() public eligibleForReward {
vm.prank(bob);
marketplace.collectReward();
vm.startPrank(chasy);
for (uint256 i = 3; i < 6; i++) {
martenitsaToken.createMartenitsa("bracelet");
martenitsaToken.approve(address(marketplace), i);
marketplace.makePresent(bob, i);
}
vm.stopPrank();
vm.startPrank(bob);
marketplace.collectReward();
marketplace.collectReward(); // 1st health token is rewarded again
assertEq(healthToken.balanceOf(bob), (2 + 1) * 10 ** 18);
}

Tools Used

Foundry and manual review.

Recommendations

Implement a mechanism which compounds the previously claimed rewards instead of considering only the previous snapshot:

...
if (amountRewards > 0) {
- _collectedRewards[msg.sender] = amountRewards;
+ _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.