Summary
The EggVault contract acts as a Vault to store NFTs, but does not implement the IERC721Receiver interface so it is unable to receive NFTs.
This bug is not catched by the tests (like testDepositEggToVault test) because the implementation is not correct.
Vulnerability Details
We can deposit NFTs directly to the EggVault calling the EggHuntGame::depositEggToVault function.
But the function is using eggNFT.transferFrom instead of eggNFT.safeTransferFrom, so is not checking if the receiver (in this case the EggVault) receives the NFT.
function depositEggToVault(uint256 tokenId) external {
require(eggNFT.ownerOf(tokenId) == msg.sender, "Not owner of this egg");
@> eggNFT.transferFrom(msg.sender, address(eggVault), tokenId);
eggVault.depositEgg(tokenId, msg.sender);
}
The function sends the NFT to the vault without checking that it has actually arrived, so it seems to have been successful.
But spoiler. It didn't.
Impact
The main functionality of the EggVault is broken, it cannot receive NFTs.
Tools Used
Manual Review
Foundry
PoC
Change the EggHuntGame::depositEggToVault function to use safeTransferFrom and run the EggHuntGameTest.t.sol::testDepositEggToVault
/// @notice Allows a player to deposit their egg NFT into the Egg Vault.
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);
}
You will obtain this as result:
Ran 1 test for test/EggHuntGameTest.t.sol:EggGameTest
[FAIL: ERC721InvalidReceiver(0x2e234DAe75C793f67A35089C9d99245E1C58470b)] testDepositEggToVault() (gas: 160882)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 3.62ms (735.92µs CPU time)
Ran 1 test suite in 11.42ms (3.62ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)
Failing tests:
Encountered 1 failing test in test/EggHuntGameTest.t.sol:EggGameTest
[FAIL: ERC721InvalidReceiver(0x2e234DAe75C793f67A35089C9d99245E1C58470b)] testDepositEggToVault() (gas: 160882)
Encountered a total of 1 failing tests, 0 tests succeeded
Now the code is actually checking that the NFT is sent correctly, and we see that it did not.
Recommendations
Always use safeTransferFrom to avoid this type of situations.
Implement the IERC721Receiver interface on EggVault.sol
/// @notice Allows a player to deposit their egg NFT into the Egg Vault.
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);
}
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);
}
+ import {IERC721Receiver} from "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
- contract EggVault is Ownable {
+ contract EggVault is Ownable, IERC721Receiver {
// All the code
+ function onERC721Received(address, address, uint256, bytes calldata) external pure override returns (bytes4) {
+ return IERC721Receiver.onERC721Received.selector;
+ }
}
