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

Frontrunning EggVault::depositEgg() allows anyone to withdraw NFTs that they do not own.

vanshikag

Summary

EggVault::depositEgg() is a public function that updates the internal mapping of egg ID to depositor (eggDepositors). This function requires that the NFT has already been transferred and is currently owned by the vault. There are no other checks in this function regarding the original owner of the NFT. This means anyone can frontrun the function to insert themselves as depositors of the NFT in the eggDepositors mapping. EggVault::withdrawEgg() uses this mapping to allow withdrawals of the NFTs, which means the NFTs can be withdrawn by malicious users.

// anyone can use their address as the depositor in this function
function depositEgg(uint256 tokenId, address depositor) public {
require(eggNFT.ownerOf(tokenId) == address(this), "NFT not transferred to vault");
require(!storedEggs[tokenId], "Egg already deposited");
storedEggs[tokenId] = true;
eggDepositors[tokenId] = depositor;
emit EggDeposited(depositor, tokenId);
}

Impact

Users can steal NFTs from other users.

Proof Of Concept

The test file already contains a proof of concept for this.

forge test --mt testVaultDepositAndWithdraw -vvvv

Relevant test portion:

function testVaultDepositAndWithdraw() public {
// Mint an egg directly to the vault (using game contract privileges).
vm.prank(address(game));
nft.mintEgg(address(vault), 10);
// Ensure the vault owns token 10.
assertEq(nft.ownerOf(10), address(vault));
​
//@> Egg owned by the vault can be deposited by alice. alice did not own this egg at any point.
vault.depositEgg(10, alice);
assertTrue(vault.isEggDeposited(10));
// The depositor recorded should be alice.
assertEq(vault.eggDepositors(10), alice);
.
.
.
vm.prank(alice);
//@> alice can now withdraw the egg
vault.withdrawEgg(10);
// After withdrawal, alice should be the owner again.
assertEq(nft.ownerOf(10), alice);
}

Tools

Manual Review

Recommendation

Access control on EggVault::depositEgg() can fix this vulnerability such that only the game contract can call this function. This would make EggHuntGame::depositEggToVault() the only function that can call depositEgg(), which finalizes the deposit right after the transfer.

Updates

Lead Judging Commences

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