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

Missing onERC721Received Implementation in EggVault Contract

edoscoba

Summary

The EggVault contract fails to implement the onERC721Received interface, which is required for contracts to safely receive ERC721 tokens (NFTs) via the safeTransferFrom function. This omission prevents users from using the recommended safe transfer methods to deposit their NFTs into the vault, forcing them to use the less secure transferFrom method instead.

Vulnerability Details

The ERC721 standard (EIP-721) defines two transfer methods:

  1. transferFrom - A basic transfer that does not check if the receiver can handle ERC721 tokens

  2. safeTransferFrom - A safer transfer method that verifies the receiver's ability to handle ERC721 tokens

When safeTransferFrom is called, the ERC721 contract checks if the recipient is a contract. If it is, it calls the onERC721Received function on the recipient and verifies that the correct magic value (bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))) is returned.

The EggVault contract is designed to hold NFTs but does not implement this required function:

// Missing from EggVault contract
function onERC721Received(
address operator,
address from,
uint256 tokenId,
bytes calldata data
) external returns (bytes4);

Without this implementation, any calls to safeTransferFrom with the vault as the receiver will fail, even though the vault is specifically designed to hold these tokens.

POC

function testSafeTransferReverts() public {
vm.prank(address(game));
nft.mintEgg(alice, 1);
assertEq(nft.ownerOf(1), alice);
// alice attempts to use safeTransferFrom (the recommended method)
vm.prank(alice);
vm.expectRevert(); // This will revert because onERC721Received is not implemented
nft.safeTransferFrom(alice, address(vault), 1);
// alice has to use transferFrom instead (less secure)
vm.prank(alice);
nft.transferFrom(alice, address(vault), 1);
// Verify the transfer worked with the unsafe method
assertEq(nft.ownerOf(1), address(vault));
}

Impact

This vulnerability has several negative impacts:

  1. Restricted Transfer Options: Users are forced to use the less secure transferFrom method instead of the safer safeTransferFrom method.

  2. Integration Failures: Many modern NFT platforms and wallets use safeTransferFrom by default, which would fail when interacting with this vault.

  3. Bad User Experience: Users attempting to use safe transfer methods will encounter errors, leading to confusion and friction.

Tools Used

Manual Review

Recommendations

Use OpenZeppelin's Implementation: Consider inheriting from OpenZeppelin's ERC721Holder contract, which provides a standard implementation of this interface:

import "@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol";
​
contract EggVault is Ownable, ERC721Holder {
// Rest of the contract implementation
}
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.