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

Missing access control for the function `SantasList::checkList(address,Status)` allows anyone to change the status of an address inside `SantasList::s_theListCheckedOnce`

gabr1sr

Summary

Anyone can execute the function SantasList::checkList(address,Status) to change SantasList::Status of a given address inside the SantasList::s_theListCheckedOnce mapping to any enum SantasList::Status value.

Vulnerability Details

The function SantasList::checkList(address,Status) should be only called by Santa, but since it misses an access control check anyone can execute it.

Proof of Concept

Apply the following diff:

modified test/unit/SantasListTest.t.sol
@@ -152,4 +152,27 @@ contract SantasListTest is Test {
cmds[1] = string.concat("youve-been-pwned");
cheatCodes.ffi(cmds);
}
+
+ function testAnyoneCanCheckOnce(address randomCallerAddress, address randomTargetAddress) public {
+ // Initial `randomTargetAddress` status
+ SantasList.Status initialStatus = santasList.getNaughtyOrNiceOnce(randomTargetAddress);
+
+ // Impersonates the `randomCallerAddress`
+ vm.startPrank(randomCallerAddress);
+
+ // The new `randomTargetAddress` status (EXTRA_NICE) different than the initial one
+ SantasList.Status status = SantasList.Status.EXTRA_NICE;
+
+ // Assert if the new status is different than the initial one
+ assert(status != initialStatus);
+
+ // Check the `randomTargetAddress` status once
+ santasList.checkList(randomTargetAddress, status);
+
+ // Assert the new `randomTargetAddress` status
+ assert(santasList.getNaughtyOrNiceOnce(randomTargetAddress) == status);
+
+ // Stops impersonating the `randomCallerAddress`
+ vm.stopPrank();
+ }
}

And run the testAnyoneCanCheckOnce test:

forge test --match-test testAnyoneCanCheckOnce

Impact

Anyone can execute the function SantasList::checkList(address,Status) and change the status of a given address to NICE, EXTRA_NICE, NAUGHTY, or NOT_CHECKED_TWICE, inside the mapping SantasList::s_theListCheckedOnce.

Tools Used

  • Manual Review

  • GNU Emacs (solidity-mode + magit)

  • Foundry tests

Recommendations

Add modifier onlySanta to the SantasList::checkList(address,Status) function:

modified src/SantasList.sol
@@ -118,7 +118,7 @@ contract SantasList is ERC721, TokenUri {
* @param person The person to check
* @param status The status of the person
*/
- 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 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.