Trick or Treat
First Flight #27
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

Unsafe NFT transfer

cryptolappi

Summary

The SpookySwap contract uses unsafe NFT transfer methods (_mint and _transfer) instead of their safe counterparts (_safeMint and _safeTransfer). This can lead to permanent loss of NFTs when they are transferred to contracts that don't support ERC721 token reception.

Vulnerability Details

The contract implements NFT transfers in two critical functions:

  1. In the mintTreat internal function:

function mintTreat(address recipient, Treat memory treat) internal {
uint256 tokenId = nextTokenId;
_mint(recipient, tokenId); // Unsafe minting
_setTokenURI(tokenId, treat.metadataURI);
nextTokenId += 1;
}

  1. In the resolveTrick function:

function resolveTrick(uint256 tokenId) public payable nonReentrant {
// ... payment validation ...
_transfer(address(this), msg.sender, tokenId); // Unsafe transfer
// ... cleanup code ...
}

The vulnerability arises because these functions don't verify whether the recipient can handle ERC721 tokens. According to the ERC721 standard, contracts must implement the onERC721Received function to receive tokens. Without this check, tokens can be permanently locked in contracts that don't support ERC721.

Example of a vulnerable scenario:

contract VulnerableContract {
function buyTreat(address spookySwap, string memory treatName) external payable {
SpookySwap(spookySwap).trickOrTreat(treatName);
// NFT will be lost as this contract cannot handle ERC721 tokens
}
}

Impact

  • Permanent loss of NFTs when transferred to incompatible contracts

  • No recovery mechanism for lost tokens

  • Potential financial loss for users who accidentally send NFTs to contract addresses

  • Deterioration of user trust in the platform

Tools Used

  • Manual review

Recommendations

  1. Replace unsafe transfer methods with their safe counterparts:

// In mintTreat function
function mintTreat(address recipient, Treat memory treat) internal {
uint256 tokenId = nextTokenId;
_safeMint(recipient, tokenId); // Safe minting
_setTokenURI(tokenId, treat.metadataURI);
nextTokenId += 1;
}
​
// In resolveTrick function
function resolveTrick(uint256 tokenId) public payable nonReentrant {
// ... payment validation ...
_safeTransfer(address(this), msg.sender, tokenId); // Safe transfer
// ... cleanup code ...
}
Updates

Appeal created

bube Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Use of `_mint` instead of `safeMint`

Use of `_transfer` instead of `safeTransfer`

Support

FAQs

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