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 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!