Trick or Treat

Vulnerability details

If we consider the situation where user is tricked then user has to pay double the price of the NFT. If the user didn't send enough ETH while calling the trickOrTreat function, the transaction will go into pending state and the user would have to call resolveTrick function to complete the pending tx.

In between calling trickOrTreat and resolveTrick the owner can easily increase/ decrease the price of the particular NFT by calling setTreatCost function. In which case the user would have to pay extra or they will still pay the same amount because decreasin the NFT price doesn't update the pendingNFT mapping.

Let's look at the following cases (all cases assume the situation where, user is tricked and has to pay double NFT price)

Let us consider:
starting NFT price is 10ETH, but as user is tricked they need to pay 20ETH to own.
User sends 10ETH (default price).

Case I

1. As eth sent by user <20 ETH, Tx will go in pending state and user will have to call resolveTrick to complete Tx

2. Owner calls setTreatCost after this, and updates the price of NFT to 20ETH for the particular token. Now there is a ambigous situation.
   Will the user pay 20x2-10 = 30ETH or 20-10 = 10ETH?
   The user will have to pay 30ETH now because :
   Treat is fetched by

   string memory treatName = tokenIdToTreatName[tokenId];

   Treat memory treat = treatList[treatName];

   so when owner calls setTreatCost it will update the treat cost

   struct Treat {

   string name;

   @> uint256 cost; // Cost in ETH (in wei) to get one treat

   string metadataURI; // URI for the NFT metadata

   }

   Hence, uint256 requiredCost = treat.cost * 2; will 2x the updated price by the owner.
   uint256 amountPaid = pendingNFTsAmountPaid[tokenId]; amountPaid will still remain same as it was recorded in when user called trickOrTreat
   User will pay, 20x2-10 = 30ETH

Case II

1. As eth sent by user <20 ETH, Tx will go in pending state and user will have to call resolveTrick to complete Tx

2. Owner calls setTreatCost after this, and updates the price of NFT to 5ETH for the particular token. Now there is a ambigous situation.
   Will the user pay 5*2 -10= 0ETH or 5-10 = -5ETH which isn't possible
   Logically if we see, the user should not pay anything but that would mean he/ she already owns the NFT. But that's not the case. Hence it would lead to unexpected situations.

Impact

1. If price increases: Users need to pay more

2. If price decreases: Users might get better deals, or lead to unexpected situations

3. Users can't predict the final price of NFT while initiating txs.

Tools used

Manual review

Reccomended mitigation

Make a mapping to store required cost

Update the mapping in trickOrTreat function to store the required cost, so no need to calculate it again while calling resolveTrick

Fetch required cost of pending NFT from the mapping in resolveTrick instead of recalculating