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

No previous-owner or msg.sender check in `EggVault::depositEgg()` can make anyone the "depositor" of an egg by frontrunning a transaction

unitedcoolness26

Summary

The function EggVault::depositEgg() doesn't have address checks, which can lead to some unexpected NFT ownership claims under certain situations.

Vulnerability Details

EggVault::depositEgg() does not check for who is the caller, nor does it verify who was the previous owner of the NFT that was deposited.
This opens a small attack surface. Someone can execute a frontrunning attack:

  • Suppose a user holding an Egg NFT wants to deposit their NFT into the vault, and they want to do so using the EggVault's function depositEgg(), and don't want to use the EggHuntGame::depositEggToVault() function.

  • First, they transfer the NFT: nft.transferFrom(user, vault, tokenId)

  • Then, they call the deposit function by passing their own address as the depositor: vault.depositEgg(tokenId, user)

  • EggVault::depositEgg() just checks that the current owner of the NFT is itself, and makes the depositor argument the depositor of the NFT.

  • But, in the time between the above two transactions, someone can call the deposit function passing their address as the depositor: vault.depositEgg(tokenId, attacker). The function would execute, making the attacker eligible to withdraw the egg from the vault and become the owner of the NFT.

Proof of concept

Here is a test that can be included in the test/EggHuntGameTest.t.sol::EggGameTest contract:

function testVaultDepositWithdrawAttack() public {
// Mint an egg for alice
uint256 tokenId = 10;
vm.prank(address(game));
nft.mintEgg(alice, tokenId);
assertEq(nft.ownerOf(tokenId), alice);
​
// Alice transfers the egg to the vault
vm.prank(alice);
nft.transferFrom(alice, address(vault), tokenId);
assertEq(nft.ownerOf(tokenId), address(vault));
​
// Bob calls deposit function
vm.prank(bob);
vault.depositEgg(tokenId, bob);
assertTrue(vault.isEggDeposited(tokenId));
assertEq(vault.eggDepositors(tokenId), bob);
​
// Alice's deposit function call reverts
vm.prank(alice);
vm.expectRevert("Egg already deposited");
vault.depositEgg(tokenId, alice);
​
// Bob can withdraw the egg and become its owner, originally "deposited" by Alice
vm.prank(bob);
vault.withdrawEgg(tokenId);
assertFalse(vault.isEggDeposited(tokenId));
assertEq(nft.ownerOf(tokenId), bob);
}

Impact

Loss of NFT ownership

Tools Used

Manual review

Recommendations

I would recommend to modify both the functions EggHuntGame::depositEggToVault() and EggVault::depositEgg(). Only the Vault should be responsible for transfer of the NFT.

Recommended behavior of EggHuntGame::depositEggToVault():

  1. Check that the caller is the owner of the NFT.

  2. Call the Vault contract's depositEgg() function.

// EggHuntGame.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");
​
// Call the vault's deposit function
eggVault.depositEgg(tokenId, msg.sender);
}

Recommended behavior of EggVault::depositEgg():

  1. Check the caller (msg.sender) and address depositor:

    • If the caller is NOT the Game contract, then depositor should be equal to the caller.

    • If the caller is the Game contract, then skip the depositor check.

  2. Transfer NFT from depositor to itself. This ensures these things:

    • If a player directly calls the Vault's depositEgg():

      • As the depositor is the player itself, the player should own the NFT, otherwise this transfer would fail.

      • The player should have approved the NFT transfer to the Vault, for the same reasons as above.

    • If a player deposits via. EggHuntGame::depositEggToVault():

      • The depositor vs. caller check is skipped here, but in EggHuntGame::depositEggToVault(), we are already checking that its caller should be the NFT owner. That's why the player should own the NFT and should have provided approval to the vault to transfer the NFT (same conditions as above).

      • In this case, the vault trusts the game contract with the depositor address.

  3. Rest of the lines in EggVault::depositEgg() are unchanged.

// EggVault.sol
​
// ****** Maintain the game address in state ******
/// @notice Reference to the EggHuntGame contract address.
address public gameAddress;
​
/// @notice Set the EggHuntGame contract address.
function setGameAddress(address _gameAddress) external onlyOwner {
require(_gameAddress != address(0), "Invalid Game address");
gameAddress = _gameAddress;
}
// ************************************************
​
/// @notice Records the deposit of an egg (NFT).
/// The player must first approve the transfer on the NFT contract.
function depositEgg(uint256 tokenId, address depositor) public {
if (msg.sender != gameAddress) {
require(depositor == msg.sender, "Invalid depositor");
}
// Transfer the NFT from the depositor to this contract
eggNFT.transferFrom(depositor, address(this), tokenId);
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 user flow would change:

  • The user would need to approve their owned NFTs to the EggVault contract only, instead of the EggHuntGame contract.

Updates

Lead Judging Commences

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

Give us feedback!