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

Wrong `if condition` in `checkTwice` function results in Denial of Service (DOS)

0x0bserver

Summary

Wrong if condition in checkTwice function results in Denial of Service (DOS).

Vulnerability Details

function checkTwice(address person, Status status) external onlySanta {
@> if (s_theListCheckedOnce[person] != status) {
revert SantasList__SecondCheckDoesntMatchFirst();
}
s_theListCheckedTwice[person] = status;
emit CheckedTwice(person, status);
}

The if condition forces the s_theListCheckedOnce variable to be same as status variable selected by the Santa, i.e., checkTwice function always depends on the s_theListCheckedOnce variable value.

And due to the vulnerability already present in function checkList, anyone can call checkList function and change the s_theListCheckedOnce variable.

Hence, attacker can always change the status of s_theListCheckedOnce variable set by Santa before Santa can call checkTwice function due to attacker front running the Santa.

Impact

Due to this if condition and front running by the attacker, Santa can never change the s_theListCheckedTwice variable's value.

Hence, making the contract unusable for any user or santa.

To execute this test : forge test -vvvvv

address attacker = makeAddr("attacker");
function test() public {
vm.prank(santa);
santasList.checkList(user, SantasList.Status.EXTRA_NICE);
vm.prank(attacker);
if (
santasList.getNaughtyOrNiceOnce(user) ==
SantasList.Status.EXTRA_NICE
) {
santasList.checkList(user, SantasList.Status.NAUGHTY);
}
assertEq(
uint256(santasList.getNaughtyOrNiceOnce(user)),
uint256(SantasList.Status.NAUGHTY)
);
vm.prank(santa);
vm.expectRevert();
santasList.checkTwice(user, SantasList.Status.EXTRA_NICE);

Tools Used

Manual Review

Recommendations

To mitigate the vulnerability, if condition should be removed. There is no requirement of if condition in the logic of checkTwice function.

Here's how you can modify the function:

function checkTwice(address person, Status status) external onlySanta {
s_theListCheckedTwice[person] = status;
emit CheckedTwice(person, status);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge almost 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.