First Flight #5: Santa's List
First Flight #5
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: low
Valid

`buyPresent()` function mints to wrong address

0x141414

Summary

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.

Tools Used

Forge

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();
}
​
Updates

Lead Judging Commences

equious Auditor
almost 2 years ago
inallhonesty Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

buyPresent should send to presentReceiver

Support

FAQs

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