- High: 3
- Medium: 1
- Low: 1
Anyone can call buyPresent even though they do not have santaTokens. There is mismatch between the protocol document and the implementation.
In the documentation, it points out that the buyPresent function is only callable by anyone with SantaToken. However, there is no balance check in the function. Consider there are two users, A and B. A has santaToken and approve to the santasList contract, he/she is going to buy present for friends. B sees the chance and trigger buyPresent right before A executes the function. B can simply pass the address of A in the presentReceiver parameter and the token of A will be burnt and B will receive the NFT.
Unintended behavior for buyPresent function might lead to the loss of token for token owner.
Manual Review
- Check whether
msg.senderinbuyPresnetfunction has enough balance. - burn the token of
msg.sender, not thepresentReceiver. - allocating NFT for
presentReceiver, notmsg.sender.
When the SantasList::s_theListCheckedOnce and SantasList::s_theListCheckedTwice are not initialized, the default value of the enum will be NICE. It will bypass the validation rules in SantasList::collectPresent and earn the NFT.
Since the default value of enum type will be its first value in the enum structure. In this case, all the default mapping value will be NICE. User bypasses the validation rules in SantasList::collectPresent since s_theListCheckedOnce[msg.sender] == Status.NICE and s_theListCheckedTwice[msg.sender] == Status.NICE. Users are able to mint and receive the NFT even though they should be marked naughty later.
Unintended amount of NFTs will be allocated.
Manual Review
Add a new type in the first element in Status, such as UNINITIALIZED.
Arbitrary user can update the s_theListCheckedOnce array due to the missing access control. In the natspec of the protocol, it points out that there should be onlySanta modifier for SantasList::checkList function.
In the SantasList::checkList function where the s_theListCheckedOnce array is updated, there is no access control and anyone can update the value. An user can bypass the first validation easily.
The first validation is useless since anyone can update its value.
Manual Review
Add onlySanta modifier in the SantasList::checkList function
The protocol documentation outline the price to buy the present is 2e18 but the buyPresent funciton only charge 1e18.
In the protocol documentation, it states that:
The cost of santa tokens for naughty people to buy presents is 2e18.
However, in the buyPresent function it only burn 1e18 for each operation, that is, users pay less than they are required.
Protocol lose funds due to the incorrect calculation of the cost buying the gift.
Manual review
Update the unit of burning tokens to 2e18.
There is important update after solidity version 0.8.20, the push0 opcode is introduced which might not work in other EVM compatible chains.
EIP-3855 introduces a breaking change in solidity version 0.8.20. And the push0 opcode is utilized which might not be updated on layer 2 chains such as Arbitrum or others.
It will lead to unintended behavior and the failure of deployment.
Manual Review
Use solidity 0.8.19 or earlier version instead.