Pieces Protocol
First Flight #32
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Severity: low
Invalid

Unnecessary check when transferring NFT in `TokenDivider::divideNft`

marcoroccon

Description:
When calling the safeTransferFrom function defined in the ERC721 standard, it is not necessary to add additional checks on whether the transaction has succedeed or not, since the standard contract already reverts in case there are no conditions to transfer the token.

Impact:
Adding additional checks could cause an increase in gas costs, along with a more complex and less readable codebase.

Tools Used:
Manual review

Recommended Mitigation:
It is recommended to remove the additional check in TokenDivider::divideNft function. This operation is safe since we can rely on the standard implementation of the ERC721 contract.

function divideNft(address nftAddress, uint256 tokenId, uint256 amount) onlyNftOwner(nftAddress, tokenId) onlyNftOwner(nftAddress ,tokenId) external {
​
if(nftAddress == address(0)) { revert TokenDivider__NftAddressIsZero(); }
if(amount == 0) { revert TokenDivider__AmountCantBeZero(); }
​
ERC20ToGenerateNftFraccion erc20Contract = new ERC20ToGenerateNftFraccion(
string(abi.encodePacked(ERC721(nftAddress).name(), "Fraccion")),
string(abi.encodePacked("F", ERC721(nftAddress).symbol())));
​
erc20Contract.mint(address(this), amount);
address erc20 = address(erc20Contract);
​
IERC721(nftAddress).safeTransferFrom(msg.sender, address(this), tokenId, "");
​
- if(IERC721(nftAddress).ownerOf(tokenId) == msg.sender) { revert TokenDivider__NftTransferFailed(); }
​
balances[msg.sender][erc20] = amount;
nftToErc20Info[nftAddress] = ERC20Info({erc20Address: erc20, tokenId: tokenId});
erc20ToMintedAmount[erc20] = amount;
erc20ToNft[erc20] = nftAddress;
​
emit NftDivided(nftAddress, amount, erc20);
bool transferSuccess = IERC20(erc20).transfer(msg.sender, amount);
if(!transferSuccess) {
revert TokenDivider__TransferFailed();
}
}

This way, the custom error TokenDivider__NftTransferFailed can be removed, as it is not used elsewhere.

- error TokenDivider__NftTransferFailed();
Updates

Lead Judging Commences

fishy Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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