Summary
The current design of the protocol assumes that if a user gets an unlucky roll, they will pay double for that NFT.
However, there is no incentive for the user to actually pay the remaining amount by calling on resolveTrick, with the exception that they might have REALLY bad luck if they call on trickOrTreat again.
Vulnerability Details
Consider the scenario in which a user getting an unlucky roll. They are met with the decision to either pay the remainder using resolveTrick, essentially paying an over-collateralized amount (double the cost of the NFT) of funds that they may not have, or instead try their luck again by calling on trickOrTreat. Calling on trickOrTreat again would mean either paying the normal amount or hopefully getting a lucky roll to pay half.
Either way the user has lost their money, but the issue here is that they may not be able to send double the funds again to resolve the trick and could instead continue to mint other NFTs. Furthermore, by not requiring the user to complete an NFT mint, the user could exploit sending in half the cost when minting, expecting the call on trickOrTreat to either revert, get lucky at half the price, or get unlucky and only lose half.
Impact
The impact of this vulnerability is a MEDIUM as it could result in a loss of potential funds from users trying to mint an NFT. Furthermore, while vulnerability is rare to come across, but could pose an issue due to its unintented side-effects.
POC
First, please create a test file SpookySwapTest.t.sol under ~/test/ with the following setup:
pragma solidity 0.8.24;
import { Test, console } from "forge-std/Test.sol";
import { SpookySwap } from "../src/TrickOrTreat.sol";
contract SpookySwapTest is Test {
SpookySwap public spookySwap;
uint256 userStartBalance = 3 ether;
address user = makeAddr("user");
address user = makeAddr("owner");
function setUp() public {
deal(user, userStartBalance);
SpookySwap.Treat[] memory treats = new SpookySwap.Treat[]();
treats[0] = SpookySwap.Treat("WereGummy", 1 ether, "");
vm.startPrank(owner);
spookySwap = new SpookySwap(treats);
vm.stopPrank();
}
function helper_GetTreat(string memory _treatName) public view returns (SpookySwap.Treat memory) {
(string memory treatName, uint256 treatCost, string memory treatMeta) = spookySwap.treatList(_treatName);
return SpookySwap.Treat(treatName, treatCost, treatMeta);
}
}
In this following test, I created a function trickOrTreat_Trick that would guarantee getting an unlucky roll inside SpookySwap. The only difference from this function to trickOrTreat is the hardcoded costMultiplierNumerator and costMultiplierDenominator variables:
function trickOrTreat_Trick(string memory _treatName) public payable nonReentrant {
Treat memory treat = treatList[_treatName];
require(treat.cost > 0, "Treat cost not set.");
@> uint256 costMultiplierNumerator = 2;
@> uint256 costMultiplierDenominator = 1;
uint256 requiredCost = (treat.cost * costMultiplierNumerator) / costMultiplierDenominator;
if (costMultiplierNumerator == 2 && costMultiplierDenominator == 1) {
if (msg.value >= requiredCost) {
mintTreat(msg.sender, treat);
} else {
uint256 tokenId = nextTokenId;
_mint(address(this), tokenId);
_setTokenURI(tokenId, treat.metadataURI);
nextTokenId += 1;
pendingNFTs[tokenId] = msg.sender;
pendingNFTsAmountPaid[tokenId] = msg.value;
tokenIdToTreatName[tokenId] = _treatName;
emit Swapped(msg.sender, _treatName, tokenId);
}
} else {
require(msg.value >= requiredCost, "Insufficient ETH sent for treat");
mintTreat(msg.sender, treat);
}
if (msg.value > requiredCost) {
uint256 refund = msg.value - requiredCost;
(bool refundSuccess,) = msg.sender.call{value: refund}("");
require(refundSuccess, "Refund failed");
}
}
Then add the following test:
function test_UserCanSkipPayingDouble() public {
string[] memory treats = spookySwap.getTreats();
string memory _treat = treats[0];
SpookySwap.Treat memory curTreat = helperGetTreat(_treat);
vm.startPrank(user);
spookySwap.trickOrTreat_Trick{value: 1 ether}(curTreat.name);
vm.stopPrank();
uint256 curTokenId = spookySwap.nextTokenId()-1;
console.log("curTokenId: ", curTokenId);
address curBuyer = spookySwap.pendingNFTs(curTokenId);
assertEq(curBuyer, user);
string memory pendingNFT = spookySwap.tokenIdToTreatName(curTokenId);
assertEq(pendingNFT, "WereGummy");
uint256 pendingCost = spookySwap.pendingNFTsAmountPaid(curTokenId);
assertEq(pendingCost, 1 ether);
vm.startPrank(user);
spookySwap.trickOrTreat{value: 0.5 ether}(curTreat.name);
vm.stopPrank();
}
Tools Used
Recommendations
To remedy this issue, a mapping could be added, which saves how much the user has left to pay:
mapping(address => bool) public userPaymentPending;
And then modify the trickOrTreat function as follows:
function trickOrTreat(string memory _treatName) public payable nonReentrant {
Treat memory treat = treatList[_treatName];
require(treat.cost > 0, "Treat cost not set.");
+ require(userPaymentPending[msg.sender] == false, "Must pay for pending NFT before minting again");
