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

Anyone can check the list by calling SantasList::checkList and update the `s_theListCheckedOnce` mapping

shikhar229169

Summary

In the SantasList contract only Santa is to be allowed to call SantasList::checkList but anyone call the function as it lags necessary access control. The SantasList::checkList() function modifies the s_theListCheckedOnce mapping, thus anyone can update it as the function doesn't have access control check.

Vulnerability Details

This vulnerability exists in the SantasList::checkList function in the SantasList.sol file starting on line 121.
The function checkList lags the access control to only allow santa to call it as a result of which anyone can call the function

/*
* @notice Do a first pass on someone if they are naughty or nice.
* Only callable by santa
*
* @param person The person to check
* @param status The status of the person
*/
function checkList(address person, Status status) external {
// @audit no santa check
s_theListCheckedOnce[person] = status;
emit CheckedOnce(person, status);
}

Impact

The contract NatSpec mentions that only santa is allowed to call the checkList function but anyone can call it leading to unwanted changing of the user's status and make it difficult for the user to collect their present.

PoC

  • Include the below event in the test/unit/SantasListTest.t.sol file just before setUp function.

event CheckedOnce(address person, SantasList.Status status);

  • Include the below tests in the file test/unit/SantasListTest.t.sol

Run it by:

forge test --mt test_AnyoneCanCallCheckList

The below test involves notSanta as the attacker who checks the List as there is no access control.

function test_AnyoneCanCallCheckList() public {
address notSanta = makeAddr("notSanta");
address niceGuy = makeAddr("niceGuy");
​
vm.prank(notSanta);
vm.expectEmit(false, false, false, true, address(santasList));
emit CheckedOnce(niceGuy, SantasList.Status.NAUGHTY);
santasList.checkList(niceGuy, SantasList.Status.NAUGHTY);
​
SantasList.Status status = santasList.getNaughtyOrNiceOnce(niceGuy);
assert(status == SantasList.Status.NAUGHTY);
}

Run it by:

forge test --mt test_MissingAccessControlForCheckList_MakesItDifficultForOneToCollectPresent
function test_MissingAccessControlForCheckList_MakesItDifficultForOneToCollectPresent() public {
// the attacker who checks the list due to missing access control
address badElf = makeAddr("badElf");
​
vm.startPrank(santa);
​
// Santa Checks User Once
santasList.checkList(user, SantasList.Status.NICE);
​
// Santa Checks User Second Time
santasList.checkTwice(user, SantasList.Status.NICE);
​
vm.stopPrank();
​
// Now as the user is checked twice by santa they are fully eligible to collect their exciting present 🎁
​
// But Bad Elf changed the user's status of s_theListCheckedOnce to something else
vm.prank(badElf);
santasList.checkList(user, SantasList.Status.NAUGHTY);
​
// User tries to get the present, but it will not work as badElf messed up and it will revert
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME());
vm.prank(user);
vm.expectRevert(
SantasList.SantasList__NotNice.selector
);
santasList.collectPresent();
}

Tools Used

Manual Review, Foundry Tests

Recommendations

Add the required access control onlySanta check to the SantasList::checkList function.

+ function checkList(address person, Status status) external onlySanta {
s_theListCheckedOnce[person] = status;
emit CheckedOnce(person, status);
}
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.