Trick or Treat
First Flight #27
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: low
Invalid

H-01: When owner changes treat price, invariant breaks for users who need to pay remainder of associated NFT

somasine

Summary

In the event that a user has been "tricked" and has to pay double the cost for a spooky NFT, the owner could update the price of the treat BEFORE the user pays the remainder from resolveTrick.
This forces the user to pay for a different price, potentially more than the original "double amount" that they were going to initially pay to get an NFT.

Vulnerability Details

User who have been "tricked" have the amount they paid added to a mapping that tracks how much they've paid, whereas the NFT that they need to pay for is minted to the SpookySwap address. To get the NFT, the user has to pay double the current cost of the NFT in order to finish the minting process and recieve their spooky NFT.
The vulnerability lies in the resolveTrick function:

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;

If the owner calls on setTreatCost to change the price of a spooky NFT, the users who are in the process of minting that same NFT could be put at a potential disadvantage if the cost were to rise. And if the owner where to lower the price, they would be putting themselves at a disadvantage!

Impact

While the chance of becoming an unlucky user who has to pay double is rare, at scale this becomes a serious issue as it results in a loss of funds for the users.
This also breaks the protocol's invariant, that a treat may cost either the normal price, or half, or double, which the protocol outlines in the documentation.
If the price of a treat changes, those users who originally had to pay double for that NFT, now have to pay double of the updated price instead of the original.

For example, lets say a spooky NFT called "WereGummy" is priced at 1 eth. A user makes a call on trickOrTreat, sending 1 eth, but gets unlucky!
Then the owner decides to update the price of the "WereGummy" to 2 eth.
Then the user calls on resolveTrick with 2 eth (since thats double of the original price), but the transaction reverts!
Now the user has to send 4 eth (and will get 2 eth back), essentially losing 1 whole eth due to the updated price!

POC

First, please create a test file SpookySwapTest.t.sol under ~/test/ with the following setup:

// SPDX-License-Identifier: MIT
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);
}
}

Then add the following test:

function test_OwnerCanBreakTreatCostInvariant() public {
string[] memory treats = spookySwap.getTreats();
string memory _treat = treats[0];
SpookySwap.Treat memory curTreat = helperGetTreat(_treat);
vm.startPrank(user);
spookySwap.trickOrTreatTrick{value: curTreat.cost}(curTreat.name);
uint256 curTokenId = spookySwap.nextTokenId()-1;
vm.stopPrank();
​
uint256 pendingPaid = spookySwap.pendingNFTsAmountPaid(curTokenId);
assertEq(pendingPaid, 1 ether);
uint256 curBalance = address(spookySwap).balance;
assertEq(curBalance, 1 ether);
vm.startPrank(owner);
spookySwap.setTreatCost(curTreat.name, curTreat.cost * 2);
vm.stopPrank();
​
vm.startPrank(user);
vm.expectRevert(); // Oh No! :(
spookySwap.resolveTrick{value: curTreat.cost*2}(curTokenId);
SpookySwap.Treat memory newTreat = helperGetTreat(_treat);
spookySwap.resolveTrick{value: newTreat.cost*2}(curTokenId);
vm.stopPrank();
}

Tools Used

  • Manual Review

Recommendations

To remedy this issue, a mapping could be added, which saves how much the user has left to pay:

mapping(uint256 => uint256) public pendingRemainderToPay; //tokenId => amount user has left to pay

And then modify the resolveTrick function as follows:

// Function for users to complete their purchase if they didn't pay enough during a trick
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 remainderToPay = pendingRemainderToPay[tokenId];
​
- uint256 requiredCost = treat.cost * 2; // Double price
+ uint256 requiredCost = remainderToPay * 2; // Double price
uint256 amountPaid = pendingNFTsAmountPaid[tokenId];
uint256 totalPaid = amountPaid + msg.value;
​
require(totalPaid >= requiredCost, "Insufficient ETH sent to complete purchase");
​
// 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");
}
}
Updates

Appeal created

bube Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

[invalid] Change cost between the call of trickOrTreat and resolveTrick

Only the owner has the rights to change the cost of the treat. Therefore it is assumed that the owner will not change the cost of the pending NFTs. The owner role is trusted.

Support

FAQs

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