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

Critical Access Control Vulnerability in EggVault.setEggNFT()

monteago30

Summary

The setEggNFT() function in the EggVault contract can be called at any time by the vault owner, allowing them to change the NFT contract address even after eggs have been deposited.

Vulnerability Details

The setEggNFT() function has insufficient access controls:

function setEggNFT(address _eggNFTAddress) external onlyOwner {
require(_eggNFTAddress != address(0), "Invalid NFT address");
eggNFT = EggstravaganzaNFT(_eggNFTAddress);
}

This function can be called by the vault owner at any time to change the NFT contract address. This presents several serious issues:

  1. The owner can point the vault to a malicious NFT contract after users have deposited eggs

  2. This could enable the owner to steal all deposited NFTs by redirecting withdrawals

  3. There's no initialization flag to prevent changing the NFT after the vault is in use

Impact

Impact: High. This vulnerability enables:

  • Complete theft of all NFTs by the vault owner

  • Ability to bypass all security measures in the system

  • No guarantee that deposited eggs remain linked to their original contract

Tools Used

Manual code review and "Proof of Code" conducted with the testNFTAddressChangeAttack() function:

function testNFTAddressChangeAttack() public {
// Initial state
assertEq(s_eggNFT.ownerOf(1), address(s_eggVault)); // Vault owns the egg
assertTrue(s_eggVault.isEggDeposited(1)); // Egg is registered in the vault
assertEq(s_eggVault.eggDepositors(1), s_player); // Player is the registered depositor
​
// The vault owner can change the NFT contract at any time
vm.prank(s_vaultOwner);
s_eggVault.setEggNFT(address(s_eggMaliciousNFT));
​
// We need to handle the fact that token ID 1 doesn't exist in the malicious contract
// Instead of trying to withdraw (which will fail), let's demonstrate the issue differently
​
console.log("Vault Owner Attack Successful:");
console.log("Original NFT contract:", address(s_eggNFT));
console.log("Malicious NFT contract set in vault:", address(s_eggVault.eggNFT()));
console.log("Original egg (token ID 1) is still held by the vault:", address(s_eggNFT.ownerOf(1)));
console.log("The original egg is now inaccessible to the player since the vault points to a different NFT contract");
​
}

Recommendations

Here's the recommended code for the setEggNFT function to fix the access control vulnerability:

The key improvements in this implementation:

  1. Added an initialized boolean flag to ensure the function can only be called once

  2. Added an event to provide transparency when the NFT address is set

  3. Added a clear error message when attempting to initialize twice

This pattern, known as the "initialization pattern," ensures that critical parameters can only be set once, preventing the vault owner from changing the NFT contract after users have deposited their eggs. This is a simple but effective way to protect users from the vulnerability without introducing complex upgrade patterns.

// State variable to track initialization
bool private initialized;
​
// Updated function with initialization check
function setEggNFT(address _eggNFTAddress) external onlyOwner {
require(!initialized, "Vault: already initialized");
require(_eggNFTAddress != address(0), "Invalid NFT address");
eggNFT = EggstravaganzaNFT(_eggNFTAddress);
initialized = true;
emit EggNFTSet(_eggNFTAddress);
}
​
// Event to log the setting of the NFT address
event EggNFTSet(address indexed nftAddress);
Updates

Lead Judging Commences

m3dython Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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

Give us feedback!