Summary

https://github.com/Cyfrin/2024-10-trick-or-treat/blob/main/src/TrickOrTreat.sol

Vulnerability Details

the resolveTrick function breaks the Checks-Effects-Interactions (CEI) rule, which could lead to security issues, especially with the _transfer call to transfer the NFT to the user before clearing the mappings.

Impact

1. Reentrancy Attack
   By calling _transfer before clearing the mappings (pendingNFTs, pendingNFTsAmountPaid, and tokenIdToTreatName), an attacker could exploit the function using a malicious contract that performs reentrancy. In this case, the attacker could:

Re-enter the function before the mappings are deleted, which would allow them to call resolveTrick multiple times before the contract updates its state. This could result in the receipt of multiple NFTs or multiple refunds without paying the required cost.

Inconsistent Data

If the transaction of the NFT to the user fails, there is no way to verify if this transaction failed, as the _transfer function does not perform this verification, which would result in a failed transaction and the contract would clear the mappings. This leaves the smart contract with inconsistent data, as the state variables in the mappings are cleared without having delivered the NFT to the user.

Tools Used

Manual Review

Recommendations

check and update the state before any external interactions.

Use safeTransfer instead of _transfer