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

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

robertodf99

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.

Code

Place this in MartenitsaToken.t.sol.

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.

Code

Place this in MartenitsaToken.t.sol.

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();
}
...
Updates

Lead Judging Commences

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

ERC721 `transferFrom` not overriden

Support

FAQs

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

Give us feedback!