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

function checkList() should have onlySanta modifier, exposing frontrunability

0x141414

Summary

The external function checkList() is missing the onlySanta modifier. This exposes a potential frontrunnable DoS opportunity for malicious actors.

Vulnerability Details

As per the natspec @notice comment, there should be a msg.sender check using the onlySanta modifier. This permits any address to change the Status of any other address in the s_theListCheckedOnce mapping.

/*
* @notice Do a first pass on someone if they are naughty or nice.
* Only callable by santa
*
* @param person The person to check
* @param status The status of the person
*/
function checkList(address person, Status status) external {
s_theListCheckedOnce[person] = status;
emit CheckedOnce(person, status);
}

With this public checkList() access, a malicious actor may frontrun the i_santa address's attempt to call checkTwice() by changing the Status of the person's address value in the s_theListCheckedOnce[person] mapping beforehand to a value different than that of the initial checkTwice() call attempt. This would prevent the if statement in checkTwice() to be true at runtime, resulting in a revert SantasList__SecondCheckDoesntMatchFirst().

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 calls may go like this.

  1. Santa calls contract with params checkList(user1, EXTRA_NICE) and txn is finalized.

  2. Santa attempts to call contract with params checkTwice(user1, EXTRA_NICE).

  3. Attacker frontruns txn2 with it's own contract call with params checkList(user1, NAUGHTY), slotting txn3 ahead of txn2.

  4. Attacker txn finalizes, setting s_theListCheckedOnce[user1] == NAUGHTY

  5. Santa's txn2 reverts since s_theListCheckedOnce[person] != EXTRA_NICE

PoC

Done in a forge test file, this demonstrates a non-santa's ability to frontrun a user.

function test_frontrunSantaDoS() public {
SantasList.Status userStatus = SantasList.Status.EXTRA_NICE;
​
// santa first check
vm.startPrank(santa);
santasList.checkList(user, userStatus);
vm.stopPrank();
​
// attacker frontrun santa's checkTwice txn
address attacker = makeAddr("attacker");
vm.startPrank(attacker);
santasList.checkList(user, SantasList.Status.NAUGHTY);
vm.stopPrank();
​
assert(
santasList.getNaughtyOrNiceOnce(user) == SantasList.Status.NAUGHTY
);
​
// santa checkTwice
vm.startPrank(santa);
vm.expectRevert(
SantasList.SantasList__SecondCheckDoesntMatchFirst.selector
);
santasList.checkTwice(user, userStatus);
}

Impact

Any (or all) users may be prevented from successfully being checked twice, and minting their tokens, potentially resulting in a denial of service.

Tools Used

Forge

Recommendations

Add the onlySanta modifier to the external function checkList().

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.