Beginner FriendlyFoundry
100 EXP
View results
Submission Details
Severity: high
Valid

Access Control Vulnerability in Initializers SantasList::constructor SantasList::checkList, allowing anyone (not only Santa) to call it

Summary

In this code we have two initializers, constructor function and the checkList function. These functions are called during the contract creation and the contract deployment, respectively.

The checkList function, according to the Readme, should only allow Santa to call it. But that's not the case here.
Since the constructor function is an as well, we should protect it too.

Vulnerability Details

If we don't use a modifier in the constructor of our contract, it means that the constructor can be called by any address, not just the one specified as the Santa. This could lead to several potential issues.

The function checkList allows anyone to change the status of an address in the s_theListCheckedOnce list. If an attacker were to exploit this function, they could potentially manipulate the status of any address in the list, which could have significant implications, especially if the status determines whether an address is eligible to receive NFTs.

Impact

constructor:
If the constructor is called by an address other than the Santa, the contract could be initialized with incorrect values. This could lead to the contract behaving in unexpected ways, as the state of the contract would not match the expected state.

If the constructor can be called by any address, it could lead to the contract being misused. For example, an attacker could deploy the contract and set themselves as the Santa, which could allow them to manipulate the contract's state and behavior to their advantage.

checkList:
The impact of this vulnerability can be quite severe, as it could lead to unauthorized distribution of NFTs and potentially cause financial loss for the contract owner or the NFT holders. This is particularly true if the NFTs have a significant market value or if they are part of a larger system that relies on the integrity of the contract.

Tools Used

Manual Review and AI.

Recommendations

Therefore, it is crucial to implement appropriate access control measures to restrict the constructor and checkList function to only be callable by Santa as mentioned in the Readme. This can be achieved by using the onlySanta modifier in the function definition.

To protect these initializers, you could add the onlySanta modifier to them. This would ensure that only the Santa can call these functions. Here is how we could do it:

  1. For constructor:

- constructor() ERC721("Merry Christmas 2023", "SANTA") {
+ constructor() ERC721("Merry Christmas 2023", "SANTA") onlySanta {
i_santa = msg.sender;
i_santaToken = new SantaToken(address(this));
}
  1. For checkList:

- 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
almost 2 years ago
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.