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

Anyone can change the address status `SantasList::checkList`

0xakira

Summary

The SantasList contract assumes that only Santa can change the status of addresses . The checkList() function changes the status of an address to a new one, but does not include access control, so anyone can call it, including an attacker.

Vulnerability Details

The vulnerability is in the SantasList::checkList function in the SantasList.sol contract starting at line 121.
The checkList() function should only be called by Santa, but it does not contain access control, so anyone can call it and change the status of the address.

* @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);
}

To limit who can change the status of an address, we need to check that the calling function, msg.sender, is the owner of the contract.

Impact

A possible use case for this function is for Santa to give addresses statuses and then check them. The essence of the contract is to give addresses with certain statuses special features. But given that the final status is only set after 2 checks, even if the attacker passes the first check, he will not pass the second check

Since anyone can set the status, including attackers, this opens up the possibility that depending on the context, these unauthorized and potentially malicious strings could be dangerous.

According to the following NatSpec comment: This function allows only Santa to set a new status, only Santa can set a status-this is the basic assumption, and the functionality that is not true is a high severity vulnerability, but since there is then a second check, the vulnerability is considered medium severity

Tools Used

forge

Recommendations

Working Test Case

The following test sets the status of "s_theListCheckedOnce" using the address of the user . When executed, this test will pass, demonstrating that the user can set the status :

function testUserCanChangeStatus() public {
vm.prank(user);
santasList.checkList(user, SantasList.Status.EXTRA_NICE);
console.log("user address:", user);
console.log("Santa address:", santa);
console.logString("User status successfully changed ");
}

Run the test:

forge test --mt testUserCanChangeStatus -vvvv

Which yields the following output:

Running 1 test for test/unit/SantasListTest.t.sol:SantasListTest
[PASS] testUserCanChangeStatus() (gas: 41526)
Logs:
user address: 0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D
Santa address: 0x70C9C64bFC5eD9611F397B04bc9DF67eb30e0FcF
User status successfully changed
​
Traces:
[41526] SantasListTest::testUserCanChangeStatus()
├─ [0] VM::prank(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D])
│ └─ ← ()
├─ [24111] SantasList::checkList(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], 1)
│ ├─ emit CheckedOnce(person: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], status: 1)
│ └─ ← ()
├─ [0] console::log(user address:, user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D]) [staticcall]
│ └─ ← ()
├─ [0] console::log(Santa address:, santa: [0x70C9C64bFC5eD9611F397B04bc9DF67eb30e0FcF]) [staticcall]
│ └─ ← ()
├─ [0] console::log(User status successfully changed ) [staticcall]
│ └─ ← ()
└─ ← ()
​
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.73ms
​
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended Mitigation

Include access control so that the checkList function can only be called by Santa. This can be achieved in two ways

  • Using an if statement, calling the function will result in a SantasList__NotSanta() custom error if the address calling the function is not the Santa:

function checkList(address person, Status status) external {
+ if (msg.sender != i_santa) {
+ revert SantasList__NotSanta();
+ }
s_theListCheckedOnce[person] = status;
emit CheckedOnce(person, status);
}

  • Using onlySanta modifier adds logic to check that the msg.sender is the owner of the contract before executing the function's logic:

+ function checkList(address person, Status status) external onlySanta{
s_theListCheckedOnce[person] = status;
emit CheckedOnce(person, status);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year 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.