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

`EggVault` owner can change nft address, blocking user withdrawals

0xkemah

Description: The EggVault::setEggNFT function allows the owner of the vault to set the contract address of the Eggstravanganza NFT at any time after the EggVault contract has been deployed. If the NFT address is changed to a different or invalid contract, users who deposited their NFTs may be unable to withdraw them, effectively locking their assets in the vault.

Impact: Users who deposited their NFTs in the vault may be unable to withdraw it, if the NFT address is changed after their deposits.

Proof of Code:

  • User deposits NFT into the vault

  • Vault's owner updates the contract address of the NFT

  • User attempts to withdraw NFT but withdrawal fails because the vault points to a new NFT contract address

Code:

function test_UsersCannotWithdrawNFTs() public {
// mint nft to bob
vm.prank(address(game));
nft.mintEgg(address(bob), 5);
​
// bob transfers nft to the vault
vm.startPrank(bob);
nft.approve(address(game), 5);
game.depositEggToVault(5);
vm.stopPrank();
​
// check that the vault owns the nft
assert(nft.ownerOf(5) == address(vault));
​
// vault's owner sets a new nft contract address
EggstravaganzaNFT newNft = new EggstravaganzaNFT("NewEggstravaganza", "NEW_EGG");
vault.setEggNFT(address(newNft));
​
// bob tries to withdraw the nft
// this should revert because the vault's owner changed the nft contract
vm.prank(bob);
vm.expectRevert();
vault.withdrawEgg(5);
​
// check that the nft is not owned by bob
assert(nft.ownerOf(5) != address(bob));
​
// check that the nft is owned by the vault
assert(nft.ownerOf(5) == address(vault));
}
​

Tools Used: Manual Review

Recommended Mitigation: To prevent this, the EggVault::setEggNFT function should be removed and the EggstravaganzaNFT address should be set directly in the constructor of EggVault. This approach eliminates the need to inherit the Ownable contract in EggVault, as no other functions would require access control through the onlyOwner modifier.

- function setEggNFT(address _eggNFTAddress) external onlyOwner { ... }
- constructor() Ownable(msg.sender) {}
​
+ constructor(address _eggNFT) Ownable(msg.sender) {
+ eggNFT = EggstravaganzaNFT(_eggNFT);
}
Updates

Lead Judging Commences

m3dython Lead Judge about 2 months ago
Submission Judgement Published
Invalidated
Reason: Design choice
Assigned finding tags:

Trusted Owner

Owner is trusted and is not expected to interact in ways that would compromise security

Support

FAQs

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