Submission Details
Severity:
```SantaList::collectPresent()``` is unredeemable if the user has received a bought Present
Summary
If a user classified as "NICE" or "EXTRA_NICE" receives a SantaList::buyPresent() NFT prior to redeeming their gift, they are precluded from redeeming the present by invoking SantaList::collectPresent().
Vulnerability Details
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
function testCollectPresentNiceIsUncollectable() public {
//The user2 is EXTRA_NICE and collects the present and one SantaToken
vm.startPrank(santa);
santasList.checkList(user2, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(user2, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
vm.startPrank(user2);
santasList.collectPresent();
assertEq(santasList.balanceOf(user2), 1);
assertEq(santaToken.balanceOf(user2), 1e18);
vm.stopPrank();
//The user is NICE
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);
//User receive an NFT present from user2
vm.startPrank(user2);
santasList.safeTransferFrom(user2, user, 0);
assertEq(santasList.balanceOf(user), 1);
vm.stopPrank();
//User tries to collect the present but he/she can't because he has already received a present from another address (user2)
vm.startPrank(user);
vm.expectRevert();
santasList.collectPresent();
vm.stopPrank();
}
Tools Used
Manual review
Recommendations
Considering to store the status of the Present collection (collected/uncollected) using a mapping.
+ mapping(address => bool) private s_presentCollected;
function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
- if (balanceOf(msg.sender) > 0)
+ if (s_presentCollected[msg.sender]) {
revert SantasList__AlreadyCollected();
}
if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
+ s_presentCollected[msg.sender] = true;
_mintAndIncrement();
return;
} else if (
s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE
&& s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE
) {
+ s_presentCollected[msg.sender] = true;
_mintAndIncrement();
i_santaToken.mint(msg.sender);
return;
}
revert SantasList__NotNice();
}
Updates
Lead Judging Commences
inallhonesty about 2 years ago
Submission Judgement Published 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.
Rewards breakdown
nSLOC
116This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.