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

`_collectedRewards[msg.sender] = amountRewards;` overwrites the previous value stored in _collectedRewards[msg.sender] with the new value amountRewards instead of adding up

tobezzi

Summary

The MartenitsaMarketplace::collectRewards function overwrites the _collectedRwards instead of adding to it. The formula use to calculating the result amountRewards = (count / requiredMartenitsaTokens) - _collectedRewards[msg.sender]; is subtracting the collected tokens and the overwriting the collected result.

Vulnerability Details

Let say Bob has gathered 3 Martenitsa tokens and want to collect his reward. If we use the formula amountRewards = (count / requiredMartenitsaTokens) - _collectedRewards[msg.sender];. Bob gets 1 Health token as expected. Since the required Martenitsa token for 1 health token is 3.

If Bob comes back after some days with 6 Martenitsa tokens, he expects to get 2 Health token, but instead he gets 1.

  • lets use the formula; amountRewards = (count / requiredMartenitsaTokens) - _collectedRewards[msg.sender];

  • count = 6 Martenitsa token

  • requiredMarenitsaToken = 3

  • collectReward of Bob = 1

applying the formula
= (6/3) -1 = 2 - 1 = 1;

Now, amountRewards = 1 and it is push into bob's collected reward as seen below

if (amountRewards > 0) {
_collectedRewards[msg.sender] = amountRewards;
healthToken.distributeHealthToken(msg.sender, amountRewards);
}

This occurs because after subtracting the initial reward of bob, the amountRewards is pushed into the mapping of the collectRewards instead of adding it

Impact

This could make your users feel like you are cheating them and they will call it a scam. It could also render the platform useless since the endpoint is HealthToken, and it's supposed to be 3: 1 with Martenitsa token but instead they are stuck with undervalued health token

proof of Concept

function testRewards() public {
uint256 count = 9;
uint256 initialRewards = 1;
​
uint256 requiredMartenitsaTokens = 3;
uint256 expectedRewardon9Tokens = 3;
​
//using the exact formula from collectReward function
uint256 amountRewards = (count / requiredMartenitsaTokens) - initialRewards;
if (amountRewards > 0) {
initialRewards = amountRewards;
}
​
// since it 3: 1, with 9 Martinetsa token I should get 3 Health token but got 2 instead
assert(initialRewards ==2);
assert(initialRewards != expectedRewardon9Tokens);
}

Tools Used

manual review

Recommendations

Add the rewards

​
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.