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

Lack of treat cost check in `setTreatCost` function to ensure the new cost entered to be more than zero

soupy

Summary

In setTreatCost function, owner can reset the cost price of a treat. There's a condition check on the cost in treatList[_treatName] within the setTreatCost function itself, however there's no similar check on the new cost entered as input parameter. This could potentially cause the final treat cost be zero if it is somehow accidentally entered by mistake and there's no way to reset back again the treat to the correct cost price after the first mistaken action.

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

Vulnerability Details

Function setTreatCost implement a cost check on treatList[_treatName], however there's no similar condition check on the new cost input parameter _cost

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

This could potentially have the final treatList[_treatName].cost be set to zero if it is entered unintentionally by mistake. Once this is set, there's no way for the owner to reset back the correct cost price as it will be reverted by require(treatList[_treatName].cost > 0, "Treat must cost something."); and further causes the trickOrTreat function to revert when user makes the call to that function on the affected treatName.

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_lackOfSecondLevelCostCheckAtSetTreatCost() public {
vm.prank(owner);
// first cost reset action by mistake
spookySwap.setTreatCost("Vesper Nightshade", 0);
​
(, uint256 newCost,) = spookySwap.treatList("Vesper Nightshade");
assertEq(newCost, 0);
​
vm.prank(owner);
// second cost reset action in attempt to correct back the mistake done earlier
vm.expectRevert("Treat must cost something.");
spookySwap.setTreatCost("Vesper Nightshade", 0.08 ether);
​
vm.prank(user);
// user who wants to get "Vesper Nightshade" NFT is now unable to buy it via `trickOrTreat`
vm.expectRevert("Treat cost not set.");
spookySwap.trickOrTreat("Vesper Nightshade");
}
​
}

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

The test passed with the expected reverts and assertion indicating that owner can't reset the price of the treat once mistake of setting the cost to zero is made and user thenafter can't buy the affected NFT.

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

Impact

Owner can't reset the price of the treat once mistake of setting the cost to zero is made and user thenafter can't buy the affected NFT.

Tools Used

Manual review with test

Recommendations

Implement similar cost condition check on the input parameter as described below:

function setTreatCost(string memory _treatName, uint256 _cost) public onlyOwner {
+ require(_cost > 0, "Cost must be greater than zero.");
require(treatList[_treatName].cost > 0, "Treat must cost something.");
treatList[_treatName].cost = _cost;
}

Rerun of the same test forge test --match-test test_audit_lackOfSecondLevelCostCheckAtSetTreatCost will fail this round indicating that the implementation recommended has effectively blocked the wrong doing action of setting the cost of treat to zero.

Ran 1 test for test/TrickOrTreatTest.t.sol:SpookySwapTest
[FAIL: revert: Cost must be greater than zero.] test_audit_lackOfSecondLevelCostCheckAtSetTreatCost() (gas: 13299)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 689.25µs (39.63µs CPU time)
Updates

Appeal created

bube Lead Judge 7 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.