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

The claimNft() transfers NFT ownership, but it does not explicitly verify whether the transfer was successful

0xgo4ko

Summary

The TokenDivider::claimNft() transfers NFT ownership using safeTransferFrom(), but it does not explicitly verify whether the transfer was successful. While OpenZeppelin’s ERC721 guarantees correct behavior, adding an explicit ownership check enhances security and protects against interactions with non-standard ERC721 implementations.

Vulnerability Details

Link: https://github.com/Cyfrin/2025-01-pieces-protocol/blob/4ef5e96fced27334f2a62e388a8a377f97a7f8cb/src/TokenDivider.sol#L147-L167

After burning all ERC-20 fractions, the function transfers the NFT. However, it does not verify that msg.sender successfully became the new owner. In OpenZeppelin’s implementation, safeTransferFrom() updates ownership before calling onERC721Received(), so the risk is low. However, if nftAddress refers to a non-standard ERC721 implementation that does not properly update ownership, the NFT could be lost or remain locked.

Impact

  • Possible issue if interacting with non-compliant or malicious NFT contract that does not correctly update ownership.

  • Could cause asset loss if a faulty ERC721 implementation allows state updates before ownership transfer completes.

Tools Used

Manual review

Recommendations

To enhance security, add an explicit verification step after safeTransferFrom():

function claimNft(address nftAddress) external {
​
...
IERC721(nftAddress).safeTransferFrom(address(this), msg.sender, tokenInfo.tokenId);
​
if (IERC721(nftAddress).ownerOf(tokenInfo.tokenId) != msg.sender) {
revert TokenDivider__NftTransferFailed();
}
}
Updates

Lead Judging Commences

fishy Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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