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

`MartenitsaToken::transferFrom` Functions are not Overridden

4rdiii

[M-1] MartenitsaToken::transferFrom Functions are not Overriden and as a Result When Using Them MartenitsaToken::countMartenitsaTokensOwner State Variables For New and Previous Owners Does not Update

Description: The MartenitsaToken contract does not override the safeTransfer and safeTransferFrom functions inherited from the ERC721 standard. As a result, when these functions are used to transfer tokens, the countMartenitsaTokensOwner state variables for both the new and previous owners do not get updated correctly.

Impact: This issue can significantly affect the functionality and integrity of the MartenitsaToken contract, as it relies on accurate tracking of token ownership to manage token transfers and interactions. Incorrect updates to the countMartenitsaTokensOwner state variables can lead to:

  1. Misrepresentation of token ownership, affecting the value and utility of tokens.

  2. Inability to accurately distribute rewards ( such as healthToken) or perform other token-related operations based on ownership.

  3. Potential for fraud or manipulation, as the contract's reliance on accurate ownership data is compromised.

Proof of Concept: Add the code below to your test suit:

function testTransferAndTransferFromNotOverriden() public createMartenitsa {
vm.startPrank(chasy);
uint256 ChasyCountBefore = martenitsaToken.countMartenitsaTokensOwner(
chasy
);
assertEq(ChasyCountBefore, 1);
martenitsaToken.approve(bob, 0);
martenitsaToken.safeTransferFrom(chasy, bob, 0);
uint256 ChasyCountAfter = martenitsaToken.countMartenitsaTokensOwner(
chasy
);
uint256 bobCountAfter = martenitsaToken.countMartenitsaTokensOwner(bob);
assertEq(ChasyCountAfter, 1); // this should be zero, but its not
assertEq(bobCountAfter, 0);
assertEq(martenitsaToken.balanceOf(chasy), 0); // this should be zero, and it is
assertEq(martenitsaToken.balanceOf(bob), 1); // this should be 1, and it is
}

Recommended Mitigation: To fix this issue override the safeTransferFrom function and make it so it updates the countMartenitsaTokensOwner before transfering it to others.

contract MartenitsaToken is ERC721, Ownable {
.
.
.
​
/**
* @notice Function to update the count of martenitsaTokens for a specific address.
* @param owner The address of the owner.
* @param operation Operation for update: "add" for +1 and "sub" for -1.
*/
function updateCountMartenitsaTokensOwner(
address owner,
string memory operation
- ) external {
+ ) public {
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 transferFrom(
+ address from,
+ address to,
+ uint256 tokenId
+ ) public override {
+ updateCountMartenitsaTokensOwner(from, "sub");
+ updateCountMartenitsaTokensOwner(to, "add");
+ super.transferFrom(from, to, tokenId);
+ }
.
.
.
​
}
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.