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

Users are denied `HealthToken` rewards

funkornaut

Summary

Users who have 3 or more MartenitsaTokens and collectRewards then gift their MartenitsaTokens and then acquire 3 new MartenitsaTokens will be denied HealthToken rewards.

Vulnerability Details

Users can only collect HealthTokens by owning 3 or more MartenitsaTokens. The more MartenitsaTokens the more HealthTokens a user can acquire. However, this is untrue in the scenario where a user owns 3+MartenitsaTokens, collects rewards, gifts all their MartenitsaTokens, and then mints 3+ new MartenitsaTokens.

This user should be eligible to claim 1 more HealthToken from their new MartenitsaTokens. When they go to collect their new rewards they will be unable to because their MartenitsaMarketplace::_collectedRewards mapping will be equal to 1 from their previous claim.

function collectReward() external {
...
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);
}
}
//@audit count = 3, _collectedRewards = 1 --> ((3/2 - 1) = 0 amountRewards

On their second claim, the users amountRewards = 0 and they will not be sent the HealthTokens they are entitled to.

Impact

Users are unable to claim HealthTokens when they should be able to. This issue is compounded with larger MartenitsaToken holders. In the current implementation, users must accumulate more MartenitsaTokens to claim more rewards. For example, a MartenitsaTokens holder had a balance of X and claimed HealthTokens from all X tokens. That user would only be able to claim more HealthTokens once they acquired X + 3 more tokens.

Proof of Code

Add this test to the MartenitsaMarketplace.t.sol and run to see it will fail when a user tries to rightfully claim HealthTokens

function test_CollectRewards() public eligibleForReward{
address chet = makeAddr("chet"); //create new user to gift MT to
​
vm.startPrank(bob);
marketplace.collectReward(); //bob collects the rewards from MT 0,1,2
assert(healthToken.balanceOf(bob) == 1e18);
​
martenitsaToken.approve(address(marketplace), 0);
martenitsaToken.approve(address(marketplace), 1);
martenitsaToken.approve(address(marketplace), 2);
marketplace.makePresent(chet, 0);
marketplace.makePresent(chet, 1);
marketplace.makePresent(chet, 2);
vm.stopPrank();
​
vm.startPrank(chasy); //create 3 more MTs and give them to bob
martenitsaToken.createMartenitsa("bracelet");
martenitsaToken.createMartenitsa("bracelet");
martenitsaToken.createMartenitsa("bracelet");
marketplace.listMartenitsaForSale(3, 1 wei);
marketplace.listMartenitsaForSale(4, 1 wei);
marketplace.listMartenitsaForSale(5, 1 wei);
martenitsaToken.approve(address(marketplace), 3);
martenitsaToken.approve(address(marketplace), 4);
martenitsaToken.approve(address(marketplace), 5);
marketplace.makePresent(bob, 3);
marketplace.makePresent(bob, 4);
marketplace.makePresent(bob, 5);
vm.stopPrank();
​
vm.prank(bob);
marketplace.collectReward(); //bob tried to collect rewards from MT 3,4,5 but won't get any more HT
console2.log(healthToken.balanceOf(bob));
assert(healthToken.balanceOf(bob) == 2e18); //will fail
}
}

Tools Used

Manual Review, Foundry

Recommendations

Create a mapping of "eligible rewards" and increment or decrement by the proper amount on transfers of MartenitsaTokens.

Calculate the actual rewards a user is able to collect based on that mapping eligibleRewards[msg.sender] / 3

Updates

Lead Judging Commences

bube Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Incorrect logic in collectReward

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.