Eggstravaganza
First Flight #37
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

[L-03] Unsafe ERC721 token minting/transferring and lack of onERC721Received in `EggVault` contract

vladich0x

Summary

[L-03] Unsafe ERC721 token minting/transferring and lack of onERC721Received in EggVault contract

Vulnerability Details

The EggstravaganzaNFT contract used by EggHuntGame is minting ERC721 tokens in an unsafe manner. The contract uses the _mint() function instead of _safeMint(), creating new egg NFTs. In addition the contracts EggHuntGame and EggVault are transferring the NFTs using the transferFrom() function instead of safeTransferFrom(). This all violates the ERC721 safety standards.

EggstravaganzaNFT contract:

function mintEgg(address to, uint256 tokenId) external returns (bool) {
require(msg.sender == gameContract, "Unauthorized minter");
// should be used _safeMint() instead in order to prevent NFTs from being locked in contracts that do not support ERC-721 standart
_mint(to, tokenId);
totalSupply += 1;
return true;
}

EggHuntGame contract:

function depositEggToVault(uint256 tokenId) external {
require(eggNFT.ownerOf(tokenId) == msg.sender, "Not owner of this egg");
// The player must first approve the transfer on the NFT contract.
// use safeTransferFrom() instead
eggNFT.transferFrom(msg.sender, address(eggVault), tokenId);
eggVault.depositEgg(tokenId, msg.sender);
}

EggVault contract:

function withdrawEgg(uint256 tokenId) public {
require(storedEggs[tokenId], "Egg not in vault");
require(eggDepositors[tokenId] == msg.sender, "Not the original depositor");
storedEggs[tokenId] = false;
delete eggDepositors[tokenId];
​
// use safeTransferFrom() instead
eggNFT.transferFrom(address(this), msg.sender, tokenId);
emit EggWithdrawn(msg.sender, tokenId);
}

Impact

  • The contracts EggstravaganzaNFT, EggHuntGame and EggVault doesn't validate whether the recipients are capable of handling ERC721 tokens

  • This violates ERC721 standards which recommends using:
    _safeMint() instead of _mint()
    safeTransferFrom() instead of transferFrom()

Tools Used

Manual review

Recommendations

Use _safeMint() instead of _mint() in function EggstravaganzaNFT::mintEgg():

function mintEgg(address to, uint256 tokenId) external returns (bool) {
require(msg.sender == gameContract, "Unauthorized minter");
- _mint(to, tokenId);
+ _safeMint(to, tokenId);
totalSupply += 1;
return true;
}

Implement the IERC721Receiver interface in EggVault contract:

+ import {IERC721Receiver} from "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
​
- contract EggVault is Ownable {
+ contract EggVault is IERC721Receiver, Ownable {
/*
* Existing functionality
*/
+ function onERC721Received(
+ address, /*operator*/
+ address, /*from*/
+ uint256, /*tokenId*/
+ bytes calldata /*data*/
+ ) external pure override returns (bytes4) {
+ return this.onERC721Received.selector;
+ }
}

Use safeTransferFrom() instead of transferFrom() in function EggHuntGame::depositEggToVault():

function depositEggToVault(uint256 tokenId) external {
require(eggNFT.ownerOf(tokenId) == msg.sender, "Not owner of this egg");
// The player must first approve the transfer on the NFT contract.
- eggNFT.transferFrom(msg.sender, address(eggVault), tokenId);
+ eggNFT.safeTransferFrom(msg.sender, address(eggVault), tokenId);
eggVault.depositEgg(tokenId, msg.sender);
}

Use safeTransferFrom() instead of transferFrom() in function EggVault::withdrawEgg():

function withdrawEgg(uint256 tokenId) public {
require(storedEggs[tokenId], "Egg not in vault");
require(eggDepositors[tokenId] == msg.sender, "Not the original depositor");
storedEggs[tokenId] = false;
delete eggDepositors[tokenId];
​
- eggNFT.transferFrom(address(this), msg.sender, tokenId);
+ eggNFT.safeTransferFrom(address(this), msg.sender, tokenId);
emit EggWithdrawn(msg.sender, tokenId);
}
Updates

Lead Judging Commences

m3dython Lead Judge 7 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Unsafe ERC721 Transfer

NFTs are transferred to contracts without onERC721Received implementation.

Support

FAQs

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