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 about 2 years 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.

Give us feedback!