Summary
The EggHuntGame contract contains a critical vulnerability in the depositEggToVault function that can lead to failed transactions. This vulnerability stems from the function not verifying if the player has approved the NFT transfer before attempting to transfer the NFT to the vault.
Vulnerability Details
The vulnerability exists in the EggHuntGame contract's depositEggToVault function. The function accepts one parameter:
tokenId: The ID of the NFT to be deposited into the vault
The critical issue is that the function assumes the player has already approved the NFT transfer but doesn't verify this assumption. The function:
Checks if the caller owns the NFT
Attempts to transfer the NFT from the player to the vault
Calls the vault's depositEgg function
However, if the player hasn't approved the transfer, the transferFrom call will revert, causing the entire transaction to fail. This is particularly problematic because:
Impact
Medium Severity: This vulnerability can lead to failed transactions and wasted gas
User Experience: Poor user experience as transactions fail without clear error messages
Gas Inefficiency: Users may waste gas on failed transactions
Proof of Concept
The vulnerability can be demonstrated with the following test case:
contract EggGameTest is Test {
EggstravaganzaNFT nft;
EggVault vault;
EggHuntGame game;
address owner;
address alice;
address bob;
error OwnableUnauthorizedAccount(address account);
function setUp() public {
owner = address(this);
alice = address(0x1);
bob = address(0x2);
nft = new EggstravaganzaNFT("Eggstravaganza", "EGG");
vault = new EggVault();
game = new EggHuntGame(address(nft), address(vault));
nft.setGameContract(address(game));
vault.setEggNFT(address(nft));
}
function testDepositEggWithoutApproval() public {
uint256 tokenId = 1;
vm.prank(address(game));
nft.mintEgg(alice, tokenId);
vm.startPrank(alice);
vm.expectRevert();
game.depositEggToVault(tokenId);
assertEq(nft.ownerOf(tokenId), alice);
}
}
run the test
forge test -vvvv --mt testDepositEggWithoutApproval
the result as follows
Ran 1 test for test/EggHuntGameTest.t.sol:EggGameTest
[PASS] testDepositEggWithoutApproval() (gas: 101313)
Traces:
[101313] EggGameTest::testDepositEggWithoutApproval()
├─ [0] VM::prank(EggHuntGame: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a])
│ └─ ← [Return]
├─ [71582] EggstravaganzaNFT::mintEgg(ECRecover: [0x0000000000000000000000000000000000000001], 1)
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: ECRecover: [0x0000000000000000000000000000000000000001], tokenId: 1)
│ └─ ← [Return] true
├─ [0] VM::startPrank(ECRecover: [0x0000000000000000000000000000000000000001])
│ └─ ← [Return]
├─ [0] VM::expectRevert(custom error f4844814:)
│ └─ ← [Return]
├─ [11784] EggHuntGame::depositEggToVault(1)
│ ├─ [642] EggstravaganzaNFT::ownerOf(1) [staticcall]
│ │ └─ ← [Return] ECRecover: [0x0000000000000000000000000000000000000001]
│ ├─ [5608] EggstravaganzaNFT::transferFrom(ECRecover: [0x0000000000000000000000000000000000000001], EggVault: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 1)
│ │ └─ ← [Revert] ERC721InsufficientApproval(0xF62849F9A0B5Bf2913b396098F7c7019b51A820a, 1)
│ └─ ← [Revert] ERC721InsufficientApproval(0xF62849F9A0B5Bf2913b396098F7c7019b51A820a, 1)
├─ [642] EggstravaganzaNFT::ownerOf(1) [staticcall]
│ └─ ← [Return] ECRecover: [0x0000000000000000000000000000000000000001]
├─ [0] VM::assertEq(ECRecover: [0x0000000000000000000000000000000000000001], ECRecover: [0x0000000000000000000000000000000000000001]) [staticcall]
│ └─ ← [Return]
└─ ← [Stop]
Tools Used
Mitigation
To fix this problem, implement the following changes:
Add an approval check in the depositEggToVault function:
function depositEggToVault(uint256 tokenId) external {
require(eggNFT.ownerOf(tokenId) == msg.sender, "Not owner of this egg");
require(
eggNFT.isApprovedForAll(msg.sender, address(this)) ||
eggNFT.getApproved(tokenId) == address(this),
"NFT transfer not approved"
);
eggNFT.transferFrom(msg.sender, address(eggVault), tokenId);
eggVault.depositEgg(tokenId, msg.sender);
}
