Summary
Vulnerability Details
address hawks = makeAddr("hawks");
function testCantCollectPresentIfAlreadyCollectedAfterGivingOwnPresent() public {
vm.startPrank(santa);
santasList.checkList(user, SantasList.Status.NICE);
santasList.checkTwice(user, SantasList.Status.NICE);
vm.stopPrank();
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
vm.startPrank(user);
santasList.collectPresent();
santasList.safeTransferFrom(user, hawks, 0);
vm.expectRevert(SantasList.SantasList__AlreadyCollected.selector);
santasList.collectPresent();
}
[⠒] Compiling...
[⠊] Compiling 1 files with 0.8.22
[⠒] Solc 0.8.22 finished in 2.83s
Compiler run successful!
Running 1 test for test/unit/SantasListTest.t.sol:SantasListTest
[FAIL. Reason: call did not revert as expected] testCantCollectPresentIfAlreadyCollectedAfterGivingOwnPresent() (gas: 175731)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 4.02ms
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)
Failing tests:
Encountered 1 failing test in test/unit/SantasListTest.t.sol:SantasListTest
[FAIL. Reason: call did not revert as expected] testCantCollectPresentIfAlreadyCollectedAfterGivingOwnPresent() (gas: 175731)
Encountered a total of 1 failing tests, 0 tests succeeded
Above terminal output shows that testCantCollectPresentIfAlreadyCollectedAfterGivingOwnPresent() test case is failing because collectPresent() function is not reverting when User is calling collectPresent() more than once.
Above vulnerability appears by passing this check in collectPresent() function.
* @notice Collect your present if you are nice or extra nice. You get extra presents if you are extra nice.
* - Nice: Collect an NFT
* - Extra Nice: Collect an NFT and a SantaToken
* This should not be callable until Christmas 2023 (give or take 24 hours), and addresses should not be able to collect more than once.
*/
function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
@> if (balanceOf(msg.sender) > 0) {
@> revert SantasList__AlreadyCollected();
@> }
if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
_mintAndIncrement();
return;
} else if (
s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE
&& s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE
) {
_mintAndIncrement();
i_santaToken.mint(msg.sender);
return;
}
revert SantasList__NotNice();
}
Impact
Tools Used
Recommendations
we can fix this vulnerability by adding a mapping to check if the present is already delivered to the User or not and then revert if the present is already delivered to the User.
remove the check of balanceOf(msg.sender) > 0 in collectPresent() function.
/*//////////////////////////////////////////////////////////////
STATE VARIABLES
//////////////////////////////////////////////////////////////*/
+ mapping(address person => bool isPresentsDelivered) private s_presentsDelivered;
/*
* @notice Collect your present if you are nice or extra nice. You get extra presents if you are extra nice.
* - Nice: Collect an NFT
* - Extra Nice: Collect an NFT and a SantaToken
* This should not be callable until Christmas 2023 (give or take 24 hours), and addresses should not be able to collect more than once.
*/
function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
- if (balanceOf(msg.sender) > 0) {
- revert SantasList__AlreadyCollected();
- }
+ if (s_presentsDelivered[msg.sender]) {
+ revert SantasList__AlreadyCollected();
+ }
if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
+ s_presentsDelivered[msg.sender] = true;
_mintAndIncrement();
return;
} else if (
s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE
&& s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE
) {
+ s_presentsDelivered[msg.sender] = true;
_mintAndIncrement();
i_santaToken.mint(msg.sender);
return;
}
revert SantasList__NotNice();
}
