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

Business Logic: checkTwice function deos not revert if the address is not registered in the s_theListCheckedOnce

0x0noob

Summary

According to the information provided relating to this function on the firstflight page. The checkTwice is expected to revert in a situation where the address passed into function does not exist in the s_theListCheckedOnce mapping. That is, an address should only be checked twice if it has been checked once.

POC

The current state of the function as shown below only ensures that the status passed in the function is same as the status it was before. While the intented purpose of the function is to change the status only on the condition that it has been checked once.

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 test below also shows that the function does not fail when it has not been checked once:

function testCheckListTwice() public {
vm.startPrank(santa);
santasList.checkTwice(user, SantasList.Status.NICE);
vm.stopPrank();
assertEq(uint256(santasList.getNaughtyOrNiceTwice(user)), uint256(SantasList.Status.NICE));
}

Impact

The impact of this is that it doesn't follow the intended business logic which requires that all addresses are checked twice and also in a situation where a given address has been given a status it cannot be changed in the checkTwice function.

Tools Used

Manual review

Recommendations

Make a provision in the function to make sure that the function fails in a situation where an address has not been checked once and also remove the checks that restrict the status from been changed in the checkTwice function. Below is a recommended change that can be implemented

++ mapping(address person => bool checked) private s_checkedOrUnchecked; //(keeps track of addresses that has been checked once)
error SantasList__NotCheckedOnce(); //(new variable)
function checkList(address person, Status status) external {
s_theListCheckedOnce[person] = status;
++ s_checkedOrUnchecked[person] = true; //( added to update the addressed to true, i.e checked)
emit CheckedOnce(person, status);
}
​
function checkTwice(address person, Status status) external onlySanta {
++if (s_checkedOrUnchecked[person] == false) {
revert SantasList__NotCheckedOnce();
} // ( added to check that an address has been checked)
s_theListCheckedTwice[person] = status;
emit CheckedTwice(person, status);
} // the initial check was removed to make it possible to change the status of an address in checkTwice
​

Also, the test below shows that it reverts when a random address which has not been checked is entered in checkTwice:

function revertsIfStatusHasNotBeenChecked() public {
address randomUser = address(1234);
vm.startPrank(santa);
santasList.checkList(user, SantasList.Status.NICE);
vm.expectRevert();
santasList.checkTwice(randomUser, SantasList.Status.NICE);
vm.stopPrank();
}
Updates

Lead Judging Commences

inallhonesty Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.