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

If any treat's price is set to zero during deployment, the owner cannot reset it later on, and the `trickOrTreat` function for that treat becomes inaccessible to users.

soupy

Summary

Missing price check for treat at constructor could result the treat price couldn't be reset later on by owner and function trickOrTreat is not callable by user if the treat price was set as zero during contract deployment

https://github.com/Cyfrin/2024-10-trick-or-treat/blob/9cb3955058cad9dd28a24eb5162a96d759bfa842/src/TrickOrTreat.sol#L29-L35

Vulnerability Details

The constructor of the contract takes in an array of Treat and loop through each item in the array with addTreat function. However, there is no check to ensure that the treats[i].cost is > 0

constructor(Treat[] memory treats) ERC721("SpookyTreats", "SPKY") {
nextTokenId = 1;
​
for (uint256 i = 0; i < treats.length; i++) {
<@@>! addTreat(treats[i].name, treats[i].cost, treats[i].metadataURI);
}
}

In function setTreatCost where owner can reset the price of the treat, there's a condition check of require(treatList[_treatName].cost > 0, "Treat must cost something.");

function setTreatCost(string memory _treatName, uint256 _cost) public onlyOwner {
<@@> ! require(treatList[_treatName].cost > 0, "Treat must cost something.");
treatList[_treatName].cost = _cost;
}

Similarly, in function trickOrTreat which the user calls to participate and buy the NFT, there's also a check require(treat.cost > 0, "Treat cost not set.")

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;
​
...
}

If any of the treat item was deployed with cost 0 at constructor stage, the owner can't reset the price anymore for that treat via setTreatCost and users can't buy that particular treat viatrickOrTreat due to the condition check of cost > 0 causing that particular treat inaccessible by both owner and user.

Proof of Concept:

Step 1: Add the following test file test\TrickOrTreatTest.t.sol :

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;
​
import {Test} from "lib/forge-std/src/Test.sol";
import {SpookySwap} from "../src/TrickOrTreat.sol";
​
contract SpookySwapTest is Test {
SpookySwap public spookySwap;
address public owner;
address public user;
​
function setUp() public {
owner = makeAddr("owner");
user = makeAddr("user");
SpookySwap.Treat[] memory treats = new SpookySwap.Treat[]();
treats[0] = SpookySwap.Treat("Vesper Nightshade", 0.02 ether, "https://metadata/vesper");
treats[1] = SpookySwap.Treat("Luna Gloom", 0, "https://metadata/luna");
treats[2] = SpookySwap.Treat("Ophelia Dread", 0.08 ether, "https://metadata/ophelia");
vm.prank(owner);
spookySwap = new SpookySwap(treats);
}
​
function test_audit_treatWithZeroCostIssueAtDeployment() public {
string memory treatWithZeroInitialCost = "Luna Gloom";
​
// owner can reset the cost price
vm.prank(owner);
vm.expectRevert("Treat must cost something.");
spookySwap.setTreatCost(treatWithZeroInitialCost, 0.06 ether);
​
// user can't buy the NFT
vm.prank(user);
vm.expectRevert("Treat cost not set.");
spookySwap.trickOrTreat(treatWithZeroInitialCost);
}
}

Step 2: Run the test forge test --match-test test_audit_treatWithZeroCostIssueAtDeployment

The test passed with the expected revert indicating that owner can't reset the price of the treat and user can't buy that treat.

Ran 1 test for test/TrickOrTreatTest.t.sol:SpookySwapTest
[PASS] test_audit_treatWithZeroCostIssueAtDeployment() (gas: 33898)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 731.13µs (71.00µs CPU time)
​

Impact

Owner can't reset the price of the treat and users can't buy the treat when the treat is set with zero cost during contract deployment

Tools Used

Manual review with test

Recommendations

Implement the cost price condition check for the treat at constructor stage :

constructor(Treat[] memory treats) ERC721("SpookyTreats", "SPKY") {
nextTokenId = 1;
​
for (uint256 i = 0; i < treats.length; i++) {
+ require(treats[i].cost > 0, "Treat cost must be > 0");
addTreat(treats[i].name, treats[i].cost, treats[i].metadataURI);
}
}

Rerun of the same test forge test --match-test test_audit_treatWithZeroCostIssueAtDeployment will fail this round indicating that the implementation recommended has effectively blocked treat item that has its cost set to zero.

Ran 1 test for test/TrickOrTreatTest.t.sol:SpookySwapTest
[FAIL: setup failed: revert: Treat cost must be > 0] setUp() (gas: 0)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 779.79µs (0.00ns CPU time)
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.