Description:
There is a lack of consistency in how pendingNFTs and related mappings are cleaned up if the function reverts after users have paid additional msg.value. For example, if the function reverts after transferring the NFT (before clearing mappings), the contract could end up with leftover mappings pointing to a completed purchase.
function resolveTrick(uint256 tokenId) public payable nonReentrant {
require(pendingNFTs[tokenId] == msg.sender, "Not authorized to complete purchase");
string memory treatName = tokenIdToTreatName[tokenId];
Treat memory treat = treatList[treatName];
uint256 requiredCost = treat.cost * 2;
uint256 amountPaid = pendingNFTsAmountPaid[tokenId];
uint256 totalPaid = amountPaid + msg.value;
require(totalPaid >= requiredCost, "Insufficient ETH sent to complete purchase");
_transfer(address(this), msg.sender, tokenId);
delete pendingNFTs[tokenId];
delete pendingNFTsAmountPaid[tokenId];
delete tokenIdToTreatName[tokenId];
if (totalPaid > requiredCost) {
uint256 refund = totalPaid - requiredCost;
(bool refundSuccess,) = msg.sender.call{value: refund}("");
require(refundSuccess, "Refund failed");
}
}
Impact:
if the function reverts after transferring the NFT (before clearing mappings), the contract could end up with leftover mappings pointing to a completed purchase. It will increae the gas cost for other users trying to call same function to resolve the purchase
Proof of Concept:
Recommended Mitigation:
A more resilient approach would be to clean up storage immediately before transferring the NFT and issuing the refund. This ensures that even if the refund fails, there is no storage corruption.
function resolveTrick(uint256 tokenId) public payable nonReentrant {
require(pendingNFTs[tokenId] == msg.sender, "Not authorized to complete purchase");
string memory treatName = tokenIdToTreatName[tokenId];
Treat memory treat = treatList[treatName];
uint256 requiredCost = treat.cost * 2; // Double price
uint256 amountPaid = pendingNFTsAmountPaid[tokenId];
uint256 totalPaid = amountPaid + msg.value;
require(totalPaid >= requiredCost, "Insufficient ETH sent to complete purchase");
// Clean up storage
+ delete pendingNFTs[tokenId];
+ delete pendingNFTsAmountPaid[tokenId];
+ delete tokenIdToTreatName[tokenId];
// Transfer the NFT to the buyer
_transfer(address(this), msg.sender, tokenId);
// Clean up storage
- delete pendingNFTs[tokenId];
- delete pendingNFTsAmountPaid[tokenId];
- delete tokenIdToTreatName[tokenId];
// Refund excess ETH if any
if (totalPaid > requiredCost) {
uint256 refund = totalPaid - requiredCost;
(bool refundSuccess,) = msg.sender.call{value: refund}("");
require(refundSuccess, "Refund failed");
}
}
