Summary
Directly depositing into the Egg Vault and using an incorrect depositor address, can lead to complete loss of owned NFT.
Vulnerability Details
Depositing into the Egg Vault can be done either externally through the Egg Hunt Game (with the "depositEggToVault()" function) or directly with the public "depositEgg()" function. The depositEgg() function takes a tokenId and depositor address:
function depositEgg(uint256 tokenId, address depositor) public {
}
If depositing through the game contract, it will provide the correct owner of the NFT (the msg.sender), whereas calling this function directly in the vault and having the user provide a depositor address by themselves, can lead to a completely loss of the owned NFT by incorrectly typing out a depositor address and essentially crediting another address as the owner.
This flaw can be tested with the following Foundry test:
pragma solidity ^0.8.23;
import "forge-std/Test.sol";
import "../src/EggstravaganzaNFT.sol";
import "../src/EggVault.sol";
import "../src/EggHuntGame.sol";
_.----._ _.---.
.-' `-.-' `. How do I deposit my egge?
.' .:''':.`.
.' .:'''':. .' .----. `.
.-./ .' .----. / .-. \ `.
/.-. / .-. \ \ ' O ' | \
|| `. | ' O '/ \ `-' / |
|| ( \ \ `-'/ `-.__ / `.
\`-' ) .-' -- ) `.
`-' _.' ( _.-' _/\ \
`. /\_ `-.____..-' .-' _/ /
`. \_ `-._ _.-'_.-' .'
`--.._ `-._ `-.__..-'_.-' .'
.-' `-._ `--.__..-' _.----'`.
/ `---.......-' _ \ \
/ ( `-._.-` )
/ / _ .- _.-'
( `-._.-' ) (_ .' \
`-._ -. (_.-' |
`. _) __..---'
| `-._) ''...__ .-. __...'''__..---'
*/
contract EggGameTestDeposit 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 testDepositEggDirectlyWithIncorrectDepositor() public {
game.setEggFindThreshold(100);
uint256 duration = 200;
game.startGame(duration);
vm.prank(alice);
game.searchForEgg();
assertEq(game.eggCounter(), 1);
assertEq(game.eggsFound(alice), 1);
vm.startPrank(alice);
nft.transferFrom(alice, address(vault), 1);
vault.depositEgg(1, bob);
vm.stopPrank();
assertEq(vault.eggDepositors(1), bob);
vm.startPrank(alice);
vm.expectRevert("Not the original depositor");
vault.withdrawEgg(1);
vm.startPrank(bob);
vault.withdrawEgg(1);
assertEq(nft.ownerOf(1), bob);
}
}
Impact
Loss of NFT due to incorrect depositor address.
Tools Used
Manual review. Foundry testing.
Recommendations
We can add a check in the Egg Vault to see if depositing an egg is coming from the Game Contract or from a user directly, in which case we will make sure the user is providing a correct depositor address and thus still allowing egg deposits to happen through the game contract or directly into the vault.
To do this, we will do the following:
Our EggVault contract will have the address and type of the EggstravaganzaNFT, which has a public address variable of the EggHuntGame contract. Due to it being public, it will automatically have a getter function added to it allowing us to view it from our Egg Vault. We can put this check into our depositEgg() function after the initial require checks.
if (msg.sender != eggNFT.gameContract()) {
require(msg.sender == depositor, "Only the depositor can deposit");
depositor = msg.sender;
}
In our EggVault.sol, we will now have a completed depositEgg() function of:
function depositEgg(uint256 tokenId, address depositor) public {
require(eggNFT.ownerOf(tokenId) == address(this), "NFT not transferred to vault");
require(!storedEggs[tokenId], "Egg already deposited");
if (msg.sender != eggNFT.gameContract()) {
require(msg.sender == depositor, "Only the depositor can deposit");
depositor = msg.sender;
}
storedEggs[tokenId] = true;
eggDepositors[tokenId] = depositor;
emit EggDeposited(depositor, tokenId);
}
If called through the Egg Hunt Game contract, it will provide the necessary details such as the depositor address. If called directly, it will do a check that if the sender is not the game contract, that the provided depositor in the call is the msg.sender, so as not to have an incorrect address.
This will allow eggs to be deposited either through the game contract or directly by a user into the vault and prevent an incorrect depositor being credited and NFT's being lost.
With the fix, we can do some tests to make sure that depositing works normally, either through the game contract or directly into vault and that it will revert if putting in an incorrect depositor when doing a direct deposit:
pragma solidity ^0.8.23;
import "forge-std/Test.sol";
import "../src/EggstravaganzaNFT.sol";
import "../src/EggVault.sol";
import "../src/EggHuntGame.sol";
_.----._ _.---.
.-' `-.-' `. My egges now deposit correctly! :)
.' .:''':.`.
.' .:'''':. .' .----. `.
.-./ .' .----. / .-. \ `.
/.-. / .-. \ \ ' O ' | \
|| `. | ' O '/ \ `-' / |
|| ( \ \ `-'/ `-.__ / `.
\`-' ) .-' -- ) `.
`-' _.' ( _.-' _/\ \
`. /\_ `-.____..-' .-' _/ /
`. \_ `-._ _.-'_.-' .'
`--.._ `-._ `-.__..-'_.-' .'
.-' `-._ `--.__..-' _.----'`.
/ `---.......-' _ \ \
/ ( `-._.-` )
/ / _ .- _.-'
( `-._.-' ) (_ .' \
`-._ -. (_.-' |
`. _) __..---'
| `-._) ''...__ .-. __...'''__..---'
*/
contract EggGameTestDepositWithFix 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));
}
modifier setGameAndFindEgg() {
game.setEggFindThreshold(100);
uint256 duration = 200;
game.startGame(duration);
vm.prank(alice);
game.searchForEgg();
assertEq(game.eggCounter(), 1);
assertEq(game.eggsFound(alice), 1);
_;
}
function testDepositThroughGameContractWorksNormal() public setGameAndFindEgg {
vm.startPrank(alice);
nft.approve(address(game), 1);
game.depositEggToVault(1);
vm.stopPrank();
assertEq(vault.eggDepositors(1), alice);
}
function testDepositDirectlyIntoEggVaultWorksNormal() public setGameAndFindEgg {
vm.startPrank(alice);
nft.transferFrom(alice, address(vault), 1);
vault.depositEgg(1, alice);
vm.stopPrank();
assertEq(vault.eggDepositors(1), alice);
}
function testDepositDirectlyIntoVaultWithIncorrectDepositorReverts() public setGameAndFindEgg {
vm.startPrank(alice);
nft.transferFrom(alice, address(vault), 1);
vm.expectRevert("Only the depositor can deposit");
vault.depositEgg(1, bob);
vm.stopPrank();
}
}
