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

`MartenitsaMarketplace::collectReward` sets `_collectedRewards` to current collected amount of HealthToken, which leads to incorrect accountability of collected Health Token

shikhar229169

Summary

The MartenitsaMarketplace::collectReward function allows non-producer users to collect Health Token on the basis of their holding of their MartenitsaToken. But when a user collects their health tokens, the total collected Health Token rewards of the user is updated to current claimed health tokens instead of incrementing it by that claimed amount, thus a user can claim more health tokens due to this lost accountability of claimed Health Tokens.

Vulnerability Details

The vulnerability is present in the MartenitsaMarketplace::collectReward function, when a user claims x amount of health token then the function incorrectly updates the total claimed health token reward of the user to x, thus allowing the user to claim a large amount of Health Tokens, as the current claimed health reward will show lesser amount of health token claimed then the actual claimed 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;
healthToken.distributeHealthToken(msg.sender, amountRewards);
}
}

Impact

A user can claim larger amount of health tokens.

For example:

  • A user has 3 Martenitsa Token, therefore they are eligible to collect 1 Health Token, after claiming _collectedRewards becomes equal to 1.

  • After some time user gets 3 more Martenitsa Token, total = 6, now claimable health token will be (6 / 3) - 1 = 1 Health Token, and _collectedRewards is set to 1.

  • Again user gets 3 more Martenitsa Token, total = 9, now claimable health token will be (9 / 3) - 1 = 2 Health Token, and _collectedRewards is set to 2.

  • We can observe that though the user had 9 Martenitsa Token but still they were able to claim 1 + 1 + 2 = 4 Health Token instead of 9 / 3 = 3 Health Token due to lost accounting of the claimed health token.

  • And as the _collectedRewards is still 2, the user can claim Health Token again and again, as it will be always less than 3 (for 9 Martenitsa Token).

Tools Used

Manual Review

Recommendations

Increment the _collectedRewards by current claimed Health Token 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;
+ _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.