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

Unlimited HealthToken Rewards and Reward Manipulation in MartenitsaMarketPlace

kostov

Summary

Users can call MartenitsaToken.sol::updateCountMartenitsaTokensOwner as many times as they wish.

This leads to two scenarios:

  1. Users may increase their MartenitsaToken count without limit, allowing them to receive an unlimited amount of HealthTokens.

  2. Users may decrease the MartenitsaToken count of other users, potentially preventing them from receiving HealthToken rewards.

Vulnerability Details

Bob calls MartenitsaToken::updateCountMartenitsaTokensOwner with his address as owner argument and "add" as operation argument, as many times as he wants to increase his MartenitsaToken count in the MartenitsaToken::countMartenitsaTokensOwner mapping.

function updateCountMartenitsaTokensOwner(address owner, string memory operation) external {
if (keccak256(abi.encodePacked(operation)) == keccak256(abi.encodePacked("add"))) {
countMartenitsaTokensOwner[owner] += 1;
} else if (keccak256(abi.encodePacked(operation)) == keccak256(abi.encodePacked("sub"))) {
countMartenitsaTokensOwner[owner] -= 1;
} else {
revert("Wrong operation");
}
}

When a user calls MartenitsaMarketPlace::collectReward, they receive a HealthToken depending on their MartenitsaToken count.
This function retrieves the user's MartenitsaToken count from the MartenitsaToken::countMartenitsaTokensOwner mapping by invoking MartenitsaToken::getCountMartenitsaTokensOwner.
Since users can increase their MartenitsaToken count without limit, they can receive an unlimited amount of HealthToken.

To continue our example, then Bob will call MartenitsaMarketPlace::collectReward to receive free HealthTokens.
The amount of HealToken that Bob will receive will be the count of his MartenitsaToken divided by 3,
however, that does not really matter, as he can increase his MartenitsaToken count as much as he wants.

Another possible attack is that Bob calls MartenitsaToken::updateCountMartenitsaTokensOwner with "sub" as operation argument and
owner can by any address which has MartenitsaToken. He calls the function as many times untill the address MartenitsaToken count becomes 0.

function collectReward() external {
require(!martenitsaToken.isProducer(msg.sender), "You are producer and not eligible for a reward!");
uint256 count = martenitsaToken.getCountMartenitsaTokensOwner(msg.sender); //problem with this function in MartenitsaToken.sol
uint256 amountRewards = (count / requiredMartenitsaTokens) - _collectedRewards[msg.sender];
​
if (amountRewards > 0) {
_collectedRewards[msg.sender] = amountRewards;
healthToken.distributeHealthToken(msg.sender, amountRewards)

Impact

There are only two ways to get a HealthToken - by winning the MartenitsaVoting or by getting 3 MartenitsaTokens and swapping them for HelathTokens in
MartenitsaMarketPlace.

By letting the users to increase/decrease their and other usres MartenitsaTokens count this can lead to a few problems:
-Practically this means users can mint unlimited amount of HealthTokens, which will lead to their devaluation.
-This will make a fewer producer want to take part of the MartenitsaVoting contest, as the reward(HealthToken) will be meaningless.
-Users can change the MartenitsaTokens count of other users and exclude them from getting their rewards, eventhough they actually could be a MartenitsaToken holders.

Proof of Code

Paste this code in MartenitsaMarketPlace and run forge test --match-test testGetUnlimitedRewards -vvv

function testGetUnlimitedRewards() public {
vm.startPrank(bob);
​
uint256 i = 0;
uint256 martenitsaToHealthTokenRatio = 3;
uint256 amountOfFreeMartenitsaTokens = 99;
​
for (i; i < amountOfFreeMartenitsaTokens; i++) {
martenitsaToken.updateCountMartenitsaTokensOwner(address(bob), "add");
}
​
uint256 bobMartenitsaTokensBalance = martenitsaToken.getCountMartenitsaTokensOwner(address(bob));
console.log("Bob amount of martenitsa tokens: ", bobMartenitsaTokensBalance);
//assert health tokens count
assert(bobMartenitsaTokensBalance == amountOfFreeMartenitsaTokens);
​
//collect reward
marketplace.collectReward();
​
//check all healthTokens are stolen
uint256 bobHealthTokensBalance = healthToken.balanceOf(address(bob));
uint256 expectedBobHealthTokensBalance = (amountOfFreeMartenitsaTokens / martenitsaToHealthTokenRatio) * 1e18;
console.log("Bob amount of health tokens ", bobHealthTokensBalance);
assert(bobHealthTokensBalance == expectedBobHealthTokensBalance);
}

Tools Used

Manual Review

Unit tests

Recommendations

Remove the MartenistaToken::updateCountMartenitsaTokensOwner function and use ERC721::balanceOf instead.

Or if you want to keep the function -
Add MartenitsaMarketPlace in the MartenitsaToken constructor and add a require in MartenistaToken::updateCountMartenitsaTokensOwner to ensure the function can only be called by the MartenitsaMarketPlace.

function updateCountMartenitsaTokensOwner(address owner, string memory operation) external {
+ require(
msg.sender == address(_martenitsaMarketplace),
"Unable to call this function"
);
​
if (keccak256(abi.encodePacked(operation)) == keccak256(abi.encodePacked("add"))) {
countMartenitsaTokensOwner[owner] += 1;
} else if (keccak256(abi.encodePacked(operation)) == keccak256(abi.encodePacked("sub"))) {
countMartenitsaTokensOwner[owner] -= 1;
} else {
revert("Wrong operation");
}
}
​
Updates

Lead Judging Commences

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

Missing access control

Support

FAQs

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