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

Anyone is able to call `checkList` function in SantasList contract and prevent any address from becoming `NICE` or `EXTRA_NICE` and collect present.

Summary

With the current design of the protocol, anyone is able to call checkList function in SantasList contract, while documentation says only Santa should be able to call it. This can be considered as an access control vulnerability, because not only santa is allowed to make the first check.

Vulnerability Details

An attacker could simply call the external checkList function, passing as parameter the address of someone else and the enum Status NAUGHTY(or NOT_CHECKED_TWICE, which should actually be UNKNOWN given documentation).
By doing that, Santa will not be able to execute checkTwice function correctly for NICE and EXTRA_NICE people. Indeed, if Santa first checked a user and assigned the status NICE or EXTRA_NICE, anyone is able to call checkList function again, and by doing so modify the status. This could result in Santa unable to execute the second check.
Moreover, any malicious actor could check the mempool and front run Santa just before calling checkTwice function to check users. This would result in a major denial of service issue.

Impact

The impact of this vulnerability is HIGH as it results in a broken mechanism of the check list system. Any user could be declared NAUGHTY for the first check at any time, preventing present collecting by users although Santa considered the user as NICE or EXTRA_NICE.

Santa could still call checkList function again to reassigned the status to NICE or EXTRA_NICE before calling checkTwice function, but any malicious actor could front run the call to checkTwice function. In this scenario, it would be impossible for Santa to actually double check a NICE or EXTRA_NICE user.

Proof of Concept

Just copy paste this test in SantasListTest contract :

function testDosAttack() external {
vm.startPrank(makeAddr("attacker"));
// any user can checList any address and assigned status to naughty
// an attacker could front run Santa before the second check
santasList.checkList(makeAddr("user"), SantasList.Status.NAUGHTY);
vm.stopPrank();
vm.startPrank(santa);
vm.expectRevert();
// Santa is unable to check twice the user
santasList.checkTwice(makeAddr("user"), SantasList.Status.NICE);
vm.stopPrank();
}

Tools Used

Foundry

Recommendations

I suggest to add the onlySanta modifier to checkList function. This will ensure the first check can only be done by Santa, and prevent DOS attack on the contract. With this modifier, specification will be respected :

"In this contract Only Santa to take the following actions:

  • checkList: A function that changes an address to a new Status of NICE, EXTRA_NICE, NAUGHTY, or UNKNOWN on the original s_theListCheckedOnce list."

The following code will resolve this access control issue, simply by adding onlySanta modifier:

function checkList(address person, Status status) external onlySanta {
s_theListCheckedOnce[person] = status;
emit CheckedOnce(person, status);
}

No malicious actor is now able to front run Santa before checkTwice function call.

The following tests shows that doing the first check for another user is impossible after adding onlySanta modifier:

function testDosResolved() external {
vm.startPrank(makeAddr("attacker"));
// checklist function call will revert if a user tries to execute the first check for another user
vm.expectRevert(SantasList.SantasList__NotSanta.selector);
santasList.checkList(makeAddr("user"), SantasList.Status.NAUGHTY);
vm.stopPrank();
}
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.