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

Missing ERC721Receiver Implementation in EggVault

molaratai

Summary

The NFT transfers to the vault rely on direct transferFrom calls:

//EggHuntGame.sol
​
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);
eggVault.depositEgg(tokenId, msg.sender);
​

Vulnerability Details

The EggVault contract interacts with ERC721 tokens but does not implement the IERC721Receiver interface with the onERC721Received function. Currently, the contract successfully receives and manages NFTs through direct transferFrom calls rather than safeTransferFrom. However, this approach deviates from ERC-721 best practices.

Impact

Low. Testing confirms that the EggVault correctly receives, stores, and transfers NFTs despite lacking the onERC721Received function. However, this implementation:

  1. Makes the contract incompatible with safeTransferFrom calls

  2. Deviates from the ERC-721 standard recommendations

  3. May cause issues if future integrations assume standard compliance

Tools Used

Manual review

Recommendations

Implement the IERC721Receiver interface in the EggVault contract:

pragma solidity ^0.8.23;
​
import "@openzeppelin/contracts/access/Ownable.sol";
import "./EggstravaganzaNFT.sol";
+ import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
​
- contract EggVault is Ownable{
+ contract EggVault is Ownable, IERC721Receiver {
/// @notice Reference to the EggstravaganzaNFT contract.
EggstravaganzaNFT public eggNFT;
​
​
/// @notice Mapping to track deposited eggs (tokenId => deposited status).
mapping(uint256 => bool) public storedEggs;
/// @notice Mapping to record who deposited each egg.
mapping(uint256 => address) public eggDepositors;
​
event EggDeposited(address indexed depositor, uint256 tokenId);
event EggWithdrawn(address indexed withdrawer, uint256 tokenId);
​
constructor()Ownable(msg.sender){}
​
+ function onERC721Received(
+ address operator,
+ address from,
+ uint256 tokenId,
+ bytes calldata data
+ ) external override returns (bytes4) {
+ return this.onERC721Received.selector;
+ }

This change would ensure the vault follows the ERC-721 standard completely and would be compatible with both transferFrom and safeTransferFrom operations.

Updates

Lead Judging Commences

m3dython Lead Judge 5 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.