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

Unsafe NFT Transfer Due to Missing Ownership Verification in NFT Claiming Function

udogodwin2k22

Summary

in the claimNft function, the contract attempts to transfer an NFT without verifying ownership, also violating the ERC721 specification requirements. This could lead to failed transactions and potential token lockup scenarios. The function assumes that if the contract has ERC20 tokens to burn and balances are correct, it must also have the NFT to transfer.

Vulnerability Details

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

The vulnerable implementation is in the claimNft function:

function claimNft(address nftAddress) external {
if(nftAddress == address(0)) {
revert TokenDivider__NftAddressIsZero();
}
ERC20Info storage tokenInfo = nftToErc20Info[nftAddress];
if(balances[msg.sender][tokenInfo.erc20Address] < erc20ToMintedAmount[tokenInfo.erc20Address]) {
revert TokenDivider__NotEnoughErc20Balance();
}
ERC20ToGenerateNftFraccion(tokenInfo.erc20Address).burnFrom(msg.sender, erc20ToMintedAmount[tokenInfo.erc20Address]);
balances[msg.sender][tokenInfo.erc20Address] = 0;
erc20ToMintedAmount[tokenInfo.erc20Address] = 0;
emit NftClaimed(nftAddress);
​
IERC721(nftAddress).safeTransferFrom(address(this), msg.sender, tokenInfo.tokenId);
}

The function uses safeTransferFrom, which according to the ERC721 spec:

  1. Throws if _from is not the current owner

  2. Throws if _to is the zero address

  3. Throws if _tokenId is not a valid NFT

  4. When transfer is complete, this function checks if _to is a smart contract (code size > 0). If so, it calls onERC721Received on _to

However, from the line in claimNFTfunction,

IERC721(nftAddress).safeTransferFrom(address(this), msg.sender, tokenInfo.tokenId);

The implementation doesn't check if the contract actually owns the NFT before attempting the transfer. According to the ERC-721 specification, safeTransferFrom should throw if _from is not the current owner.

From ERC-721 Documentation:

/// @dev Throws unless `msg.sender` is the current owner, an authorized
/// operator, or the approved address for this NFT. Throws if `_from` is
/// not the current owner. Throws if `_to` is the zero address. Throws if
/// `_tokenId` is not a valid NFT.
function safeTransferFrom(address _from, address _to, uint256 _tokenId, bytes data) external payable;

The current implementation violates this specification by not verifying if the contract (address(this)) is the current owner of the NFT using ownerOf(tokenId) before attempting the transfer.

Without the ownership check, the function would:

  1. Burn the user's ERC20 tokens

  2. Set their balance to 0

  3. Try to transfer an NFT that the contract doesn't own

  4. The transfer would fail since the contract doesn't own the NFT

  5. The entire transaction would revert

Impact

If users attempt to claim an NFT that the contract no longer owns, their ERC20 tokens could become permanently locked. This would result in the transaction reverting after tokens are burned, causing users to waste gas on failed transactions, while the ERC20 tokens remain in circulation without any backing NFTs.

Tools Used

Manual Review

Recommendations

To allow users to claim NFTs without the risk of permanently locking their ERC20 tokens or wasting gas on failed transactions, the contract should verify ownership of the NFT before proceeding with the transfer. By ensuring the contract still owns the NFT, users can avoid unnecessary token burns and revert errors.

function claimNft(address nftAddress) external {
if(nftAddress == address(0)) {
revert TokenDivider__NftAddressIsZero();
}
ERC20Info storage tokenInfo = nftToErc20Info[nftAddress];
// Add ownership verification
if(IERC721(nftAddress).ownerOf(tokenInfo.tokenId) != address(this)) {
revert TokenDivider__ContractNotOwner();
}
if(balances[msg.sender][tokenInfo.erc20Address] < erc20ToMintedAmount[tokenInfo.erc20Address]) {
revert TokenDivider__NotEnoughErc20Balance();
}
// Perform the NFT transfer first
IERC721(nftAddress).safeTransferFrom(address(this), msg.sender, tokenInfo.tokenId);
// Then update state
ERC20ToGenerateNftFraccion(tokenInfo.erc20Address).burnFrom(msg.sender, erc20ToMintedAmount[tokenInfo.erc20Address]);
balances[msg.sender][tokenInfo.erc20Address] = 0;
erc20ToMintedAmount[tokenInfo.erc20Address] = 0;
emit NftClaimed(nftAddress);
}

This implementation ensures that the contract's ownership of the NFT is verified before burning the user’s ERC20 tokens and transferring the NFT, preventing unnecessary gas expenditure and maintaining a secure and predictable claim process.

Updates

Lead Judging Commences

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

Support

FAQs

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