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

Incorrect tracking of accumulated rewards in `MartenitsaMarketplace::collectReward`

irondevx

Summary

Incorrect tracking of accumulated rewards allows for manipulation of reward distribution and users getting way more HealthTokens than they are eligible to.

function collectReward() external {
// ...
uint256 amountRewards = (count / requiredMartenitsaTokens) - _collectedRewards[msg.sender];
if (amountRewards > 0) {
@> _collectedRewards[msg.sender] = amountRewards;
healthToken.distributeHealthToken(msg.sender, amountRewards);
}
}

Vulnerability Details

MartenitsaMarketplace::collectReward incorrectly resets the count of collected rewards (_collectedRewards) using the = operator instead of properly accumulating them with the += operator. This results in inaccurate tracking of the cumulative number of HealthToken a user has claimed, leading to potential over-issuance of rewards when users manipulate their token holdings.

Impact

This vulnerability allows a user to claim more HealthToken than entitled by adjusting their MartenitsaToken count before each reward claim. The incorrect reset (=) of _collectedRewards means that if a user's total eligible rewards decrease or remain constant, the count of previously collected rewards can effectively be "forgotten," allowing the user to exploit the reward system.

Tools Used

Foundry

Proof of code

Code Add the following code to the `MartenitsaMarketplace.t.sol` file:
function _getMartenitsas(uint256 amount, uint256 tokenIdToStart) internal returns (uint256) {
uint256 tokenId = tokenIdToStart;
​
vm.startPrank(chasy);
for(uint256 i = 0; i < amount; i++){
martenitsaToken.createMartenitsa(string(abi.encodePacked("bracelet-", i)));
marketplace.listMartenitsaForSale(tokenId, 1 wei);
martenitsaToken.approve(address(marketplace), tokenId);
marketplace.makePresent(bob, tokenId);
tokenId++;
}
vm.stopPrank();
​
return tokenId;
}
​
function test__CollectReward__ImproperTrackingOfPreviousRewards() public {
// ********** Setup **********
assertEq(healthToken.balanceOf(bob), 0);
assertEq(martenitsaToken.balanceOf(bob), 0);
​
uint256[] memory martenitsasCountToAdd = new uint256[](3);
martenitsasCountToAdd[0] = 6; // should yield in 2 HealthToken (total=2)
martenitsasCountToAdd[1] = 3; // should yield in 1 HealthToken (total=3)
martenitsasCountToAdd[2] = 9; // should yield in 3 HealthToken (total=6)
​
uint256 tokenId = 0;
uint256 totalCount = 0;
​
for(uint256 i = 0; i < martenitsasCountToAdd.length; i++){
uint256 currCountToAdd = martenitsasCountToAdd[i];
totalCount += currCountToAdd;
​
tokenId = _getMartenitsas(currCountToAdd, tokenId);
assertEq(martenitsaToken.balanceOf(bob), totalCount);
​
vm.prank(bob);
marketplace.collectReward();
}
​
// actual: 8 vs expected: 6
assertEq(healthToken.balanceOf(bob), (totalCount / 3) * 10 ** 18);
​
/**
* EXPLANATION:
*
* In case of `_collectedRewards[msg.sender] = amountRewards;` (vulnerable code)
* 1. Bob has 6 MartenitsaToken
* 1.a. Bob collects rewards
* 1.b. `_collectedRewards[msg.sender] = 0`, amountRewards = 2 - 0 = 2
* 1.c. --> _collectedRewards[msg.sender] = 2
*
* 2. Bob gets 3 more MartenitsaToken (total=9)
* 2.a. Bob collects rewards
* 2.b. `_collectedRewards[msg.sender] = 2`, amountRewards = 3 - 2 = 1
* 2.c. --> _collectedRewards[msg.sender] = 1 (*issue* - should be `+= 1`, so it would be 3)
*
* EXPLOIT
* 3. Bob gets 9 more MartenitsaToken (total=18)
* 3.a. Bob collects rewards
* 3.b. `_collectedRewards[msg.sender] = 1`, amountRewards = 6 - 1 = 5
* 3.c. --> _collectedRewards[msg.sender] = 5
*
* --------------> healthToken.balanceOf(bob) = 8 (incorrect)
​
* In case of `_collectedRewards[msg.sender] += amountRewards;` (fixed code)
* 1. Bob has 6 MartenitsaToken
* 1.a. Bob collects rewards
* 1.b. `_collectedRewards[msg.sender] = 0`, amountRewards = 2 - 0 = 2
* 1.c. --> _collectedRewards[msg.sender] += 2
* 1.d. --> _collectedRewards[msg.sender] == 2
*
* 2. Bob gets 3 more MartenitsaToken (total=9)
* 2.a. Bob collects rewards
* 2.b. `_collectedRewards[msg.sender] = 2`, amountRewards = 3 - 2 = 1
* 2.c. --> _collectedRewards[msg.sender] += 1
* 2.d. --> _collectedRewards[msg.sender] == 3
*
* 3. Bob gets 9 more MartenitsaToken (total=18)
* 3.a. Bob collects rewards
* 3.b. `_collectedRewards[msg.sender] = 3`, amountRewards = 6 - 3 = 3
* 3.c. --> _collectedRewards[msg.sender] += 3
* 3.d. --> _collectedRewards[msg.sender] == 6
*
* --------------> healthToken.balanceOf(bob) = 6 (correct)
*/
}

Recommendations

Change the assignment operation in the MartenitsaMarketplace::collectReward function to correctly accumulate the total rewards a user has claimed:

function collectReward() external {
// ...
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.