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

Because previously collected addresses are not tracked, allegedly nice users can infinite collect presents

rocknet

Summary

Because addresses which have previously collected presents are not independently tracked, and only their current balance tracked, NICE and EXTRA_NICE users can infinitely collect presents, which is very NAUGHTY.

Vulnerability Details

collectPresent only checks for msg.sender's current NFT balance, which is a very naïve check for whether the user has already collected. A user could easily transfer the NFT to an alt of their own, or transfer it to someone NAUGHTY, making all the work Santa did in checking the list twice moot.

Impact

In the worst case, a single NICE or EXTRA_NICE user could give a Santa NFT to the entire world, multiple times over. An EXTRA_NICE user could further be a SantaToken whale, given they'd get 1e18 SantaToken with each call. If SantaToken was traded on exchange, or people provided liquidity, a lot of value could be destroyed. The reputational damage to Santa would be immense.

Tools Used

forge test. I wrote a passing test that proves that an EXTRA_NICE user can easily mint 101 NFTs, and have 101e18 SantaToken. The same would hold true for NICE users, just that they wouldn't have the additional SantaToken balance.

modifier beExtraNice() {
vm.startPrank(santa);
santasList.checkList(user, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(user, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
_;
}
​
function testUserCanGetInfinitePresents() public beExtraNice {
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
vm.startPrank(user);
address[] memory userAlts = new address[](100);
​
for (uint256 i = 0; i < 100; i++) {
userAlts[i] = makeAddr(string.concat("user", Strings.toString(i)));
santasList.collectPresent();
santasList.transferFrom(user, userAlts[i], i);
}
santasList.collectPresent();
vm.stopPrank();
​
assertEq(santaToken.balanceOf(user), 101e18);
​
assertEq(santasList.balanceOf(userAlts[0]), 1);
assertEq(santasList.balanceOf(userAlts[99]), 1);
assertEq(santasList.balanceOf(user), 1);
}

Recommendations

Easiest thing to do would be to maintain a mapping of addresses that collected presents already, and check whether the address was a key in the mapping prior to allowing collecting. Recommend mapping vs array for gas efficiency so you don't spin through a large array looking for the address.

Updates

Lead Judging Commences

inallhonesty Lead Judge almost 2 years 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.