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

`setTreatCost` function in SpookySwap contract contains a logical error

cryptolappi

Summary

The setTreatCost function in SpookySwap contract contains a logical error in its validation check where it validates the old cost value instead of the new one, allowing setting zero or negative costs for treats.

Vulnerability Details

The current implementation:

function setTreatCost(string memory _treatName, uint256 _cost) public onlyOwner {
require(treatList[_treatName].cost > 0, "Treat must cost something.");
treatList[_treatName].cost = _cost; // New cost is set without validation
}

The issue lies in the validation logic:

  1. require(treatList[_treatName].cost > 0) checks the CURRENT cost

  2. After passing this check, ANY new cost value is accepted

  3. No validation is performed on the _cost parameter

Example exploit:

// Initial state
spookySwap.addTreat("Candy", 1 ether, "uri"); // Cost is 1 ETH
​
// Attack
spookySwap.setTreatCost("Candy", 0); // Will succeed because old cost was > 0
// or even worse
spookySwap.setTreatCost("Candy", type(uint256).max); // Could also succeed

Proof of Concept:

contract SpookySwapTest {
function testIncorrectCostValidation() public {
// Setup
Treat[] memory treats = new Treat[]();
treats[0] = Treat("Candy", 1 ether, "uri");
SpookySwap spooky = new SpookySwap(treats);
// This will pass because current cost is 1 ether
spooky.setTreatCost("Candy", 0);
// Now we can mint treats for free
assertEq(spooky.treatList("Candy").cost, 0);
}
}

Impact

  • HIGH severity

  • Allows setting zero costs for treats

  • Can break the economic model of the platform

  • Possible front-running opportunities

  • Potential loss of revenue

Real-world attack scenarios:

  1. Owner attempts to update price from 1 ETH to 2 ETH

  2. Attacker front-runs and sets price to 0

  3. Multiple users can mint for free before owner notices

  4. Economic damage to the platform

Tools Used

  • Manual review

  • Solidity debugger

  • Unit tests

  • Echidna for property testing

Recommendations

  1. Correct the validation logic to check the new cost:

function setTreatCost(string memory _treatName, uint256 _cost) public onlyOwner {
require(treatList[_treatName].cost > 0, "Treat does not exist");
require(_cost > 0, "New cost must be greater than 0");
treatList[_treatName].cost = _cost;
emit TreatCostUpdated(_treatName, _cost);
}

  1. Add additional safeguards:

function setTreatCost(string memory _treatName, uint256 _cost) public onlyOwner {
Treat storage treat = treatList[_treatName];
require(treat.cost > 0, "Treat does not exist");
require(_cost > 0, "New cost must be greater than 0");
// Prevent extreme price changes
uint256 maxChange = treat.cost * 2; // Max 100% increase
uint256 minChange = treat.cost / 2; // Max 50% decrease
require(_cost <= maxChange, "Price increase too high");
require(_cost >= minChange, "Price decrease too high");
treat.cost = _cost;
emit TreatCostUpdated(_treatName, _cost);
}

  1. Implement a time-delay for price changes:

struct TreatPrice {
uint256 currentCost;
uint256 pendingCost;
uint256 updateTimestamp;
}
​
mapping(string => TreatPrice) public treatPrices;
uint256 public constant PRICE_UPDATE_DELAY = 1 days;
​
function scheduleTreatCostUpdate(string memory _treatName, uint256 _newCost) public onlyOwner {
require(_newCost > 0, "New cost must be greater than 0");
require(treatList[_treatName].cost > 0, "Treat does not exist");
treatPrices[_treatName] = TreatPrice({
currentCost: treatList[_treatName].cost,
pendingCost: _newCost,
updateTimestamp: block.timestamp + PRICE_UPDATE_DELAY
});
emit TreatCostUpdateScheduled(_treatName, _newCost, block.timestamp + PRICE_UPDATE_DELAY);
}
​
function executeTreatCostUpdate(string memory _treatName) public {
TreatPrice storage priceInfo = treatPrices[_treatName];
require(block.timestamp >= priceInfo.updateTimestamp, "Update not ready");
require(priceInfo.pendingCost > 0, "No update scheduled");
treatList[_treatName].cost = priceInfo.pendingCost;
delete treatPrices[_treatName];
emit TreatCostUpdated(_treatName, priceInfo.pendingCost);
}

The proposed changes provide:

  • Correct validation of new cost values

  • Protection against extreme price changes

  • Time-delay for price updates

  • Clear event emission for tracking

  • Ability to cancel pending price changes

Additional considerations:

  • Add price change limits based on time periods

  • Implement emergency price change mechanism

  • Add multi-signature requirement for price changes

  • Consider using price oracles for automated adjustments

This fix transforms what could have been a critical economic vulnerability into a secure price management system.

Updates

Appeal created

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

[invalid] Zero treat cost

The cost of the treat is set only by the owner (in the constructor, in addTreat and in setTreatCost). That means the cost of the treat will always be greater than zero.

Support

FAQs

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