Users can directly transfer martenitsas via ´MartenitsaToken::transferFrom´ and their balance will not be updated.

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2024-04-Baba-Marta/blob/5eaab7b51774d1083b926bf5ef116732c5a35cfd/lib/openzeppelin-contracts/contracts/token/ERC721/ERC721.sol#L137-L147

Summary

One of the functionalitites of the Baba Marta protocol is to make presents to friends, for this reason the function MartenitsaMarketplace::makePresent is included. However, the contract MartenitsaToken fails to override the function transferFrom from the imported dependencies. This allows users to transfer martenitsa tokens without their balances in the mapping MartenitsaToken::countMartenitsaTokensOwner being updated.

Vulnerability Details

Users can decide to transfer martenitsa tokens using MartenitsaToken::transferFrom. While the mapping MartenitsaToken::countMartenitsaTokensOwner remains unchanged, the new address will be set as the owner in the inherited mapping from the dependency MartenitsaToken::_owners, leading to incorrect information being stored which can result in unintended functionalities.

Impact

Incorrect information will be stored in the variable MartenitsaToken::countMartenitsaTokensOwner, this can lead to situations such as the following:

• bob acquires three items and transfers one token via MartenitsaToken::transferFrom. While now he is registered as the owner of two tokens and the minimum amount to claim a health token is three, he can still do it with one less. See PoC below.

    function test_BobClaimsHealthTokenWith2Martenitsas()
        public
        eligibleForReward
    {
        vm.startPrank(bob);
        martenitsaToken.transferFrom(bob, chasy, 2);
        marketplace.collectReward();
        assertEq(healthToken.balanceOf(bob), 10 ** 18);
    }

• Another scenario implies bob sending a token to chasy via MartenitsaToken::transferFrom, if chasy is just in possession of this token and tries to make a present to jack. The transaction will throw an arithmetic overflow error since the balance in MartenitsaToken::countMartenitsaTokensOwner is 0 and therefore it will result in -1 tokens owned by chasy after the transfer. See PoC below.

import {Test, console, stdError} from "forge-std/Test.sol";
...
    function test_makePresentReverts() public hasMartenitsa {
        vm.prank(bob);
        martenitsaToken.transferFrom(bob, chasy, 0);
        vm.prank(chasy);
        vm.expectRevert(stdError.arithmeticError);
        marketplace.makePresent(jack, 0);
    }

Tools Used

Foundry and manual review.

Recommendations

Override the function transferFrom so that it reverts upon invocation. This way users have to go through MartenitsaMarketplace::makePresent to transfer their items. See example below:

...
error MartenitsaToken__CannotDirectlyTransfer();
...
    function transferFrom(
        address from,
        address to,
        uint256 tokenId
    ) public override {
        revert MartenitsaToken__CannotDirectlyTransfer();
    }
...