Beginner FriendlyFoundry
100 EXP
View results
Submission Details
Severity: high
Valid

User is able to call `collectPresent()` more than once which allows User to `steal the present`

Summary

  • User is able to call collectPresent() more than once which allows User to steal the present by transferring his present to another address and then calling collectPresent() again to get another present.

Vulnerability Details

  • Below is the test code snippet of the collectPresent() function which shows that User can call collectPresent() more than once.

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

  • User can steal the present by transferring his present to another address and then calling collectPresent() again to get another present.

Tools Used

  • Manual Review

  • foundry

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

Lead Judging Commences

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

Weak Already Collected Check

Relying on balanceOf > 0 in collectPresent() allows the msg.sender to send their present to another address and then collect again.

Support

FAQs

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