Trick or Treat

First Flight #27
Beginner FriendlyFoundry
100 EXP
View results
Submission Details
Severity: high
Invalid

Cross-Function Reentrancy in the `trickOrTreat` function

Summary

The function trickOrTreathas nonReentrant modifier , however is possible hack this function by the Cross-Function Reentrancy. By using transferFromor safeTransferFromfunctions from the ERC721 interface.

Vulnerability Details

The trickOrTreat function does not adhere to the Checks-Effects-Interactions (CEI) pattern. While it does use the nonReentrant modifier, it remains vulnerable to cross-function reentrancy attacks. This vulnerability exists because a potential client can also be a smart contract. By exploiting the transferFrom or safeTransferFrom functions. It is possible to drain all NFT tokens owned by the smart contract.

The SpookySwap contract is an ERC721URIStorage and ERC721 type. It includes functions like transferFrom and safeTransferFrom, which can potentially be exploited to drain all NFT tokens from the contract.

To do that potential attacker has to send more ETH than is price for a specific treat. Because at the end of trickOrTreatfunction is if statementwhere is checking msg.value > requiredCostthat msg.value is greater than requiredCost. If it is true then refund msg.sender.call{value: refund}("");.

Please check bellow POC for hack:

TOKEN_ID_FOR_STOLEN- information about size of treatList you can take from size function or nextTokenIdparam.

POC

contract Attack {
SpookySwap public spookySwap;
constructor(SpookySwap _spookySwap) {
spookySwap = _spookySwap;
}
function attack() public payable {
// @audit it is example here you can use the item with the lowest price
spookySwap.trickOrTreat{value: 1 ether}("Choco");
}
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

Please use Checks-Effects-Interactions (CEI) pattern for trickOrTreat function.

function trickOrTreat(string memory _treatName) public payable nonReentrant {
Treat memory treat = treatList[_treatName];
require(treat.cost > 0, "Treat cost not set.");
uint256 costMultiplierNumerator = 1;
uint256 costMultiplierDenominator = 1;
// Generate a pseudo-random number between 1 and 1000
uint256 random =
uint256(keccak256(abi.encodePacked(block.timestamp, msg.sender, nextTokenId, block.prevrandao))) % 1000 + 1;
if (random == 1) {
// 1/1000 chance of half price (treat)
costMultiplierNumerator = 1;
costMultiplierDenominator = 2;
} else if (random == 2) {
// 1/1000 chance of double price (trick)
costMultiplierNumerator = 2;
costMultiplierDenominator = 1;
}
// Else, normal price (multiplier remains 1/1)
uint256 requiredCost = (treat.cost * costMultiplierNumerator) / costMultiplierDenominator;
if (costMultiplierNumerator == 2 && costMultiplierDenominator == 1) {
// Double price case (trick)
if (msg.value >= requiredCost) {
// User sent enough ETH
mintTreat(msg.sender, treat);
} else {
// User didn't send enough ETH
// Mint NFT to contract and store pending purchase
uint256 tokenId = nextTokenId;
nextTokenId += 1;
pendingNFTs[tokenId] = msg.sender;
pendingNFTsAmountPaid[tokenId] = msg.value;
tokenIdToTreatName[tokenId] = _treatName;
_mint(address(this), tokenId);
_setTokenURI(tokenId, treat.metadataURI);
// User needs to call fellForTrick() to finish the transaction
}
} else {
// Normal price or half price
require(msg.value >= requiredCost, "Insufficient ETH sent for treat");
mintTreat(msg.sender, treat);
}
// Refund excess ETH if any
if (msg.value > requiredCost) {
uint256 refund = msg.value - requiredCost;
(bool refundSuccess,) = msg.sender.call{value: refund}("");
require(refundSuccess, "Refund failed");
}
}
Updates

Appeal created

bube Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.