Summary
By adding a new Treat either by contract construction or calling SpookySwap:addTreat no checks for zero-prices are set. Consequently by calling SpookySwap:setTreatCost or SpookySwap:trickOrTreat, only those with prices greater than 0 will proceed. Moreover, updates of Treats to zero-cost aren't checked too.
Vulnerability Details
function addTreat(string memory _name, uint256 _rate, string memory _metadataURI) public onlyOwner {
@> treatList[_name] = Treat(_name, _rate, _metadataURI);
treatNames.push(_name);
emit TreatAdded(_name, _rate, _metadataURI);
}
function setTreatCost(string memory _treatName, uint256 _cost) public onlyOwner {
@> require(treatList[_treatName].cost > 0, "Treat must cost something.");
@> treatList[_treatName].cost = _cost;
}
function trickOrTreat(string memory _treatName) public payable nonReentrant {
Treat memory treat = treatList[_treatName];
@> require(treat.cost > 0, "Treat cost not set.");
...impl...
*/
}
Impact
stuck Treats in the Protocol, disabled of being updated nor trickedOrTreated, having them unusable which effectively waste protocol's resources. although it does not risk Protocol:funds.
Tools Used
Manual Review
Foundry testing tool
function testLackOfZeroCostVerification() public {
vm.expectEmit();
emit TreatAdded("candy", 0, "");
protocol.addTreat("candy", 0, "");
vm.expectRevert(bytes("Treat must cost something."));
protocol.setTreatCost("candy", 1 ether);
vm.expectRevert(bytes("Treat cost not set."));
protocol.trickOrTreat{ value: 0.1 ether }("candy");
}
Recommendations
function addTreat(string memory _name, uint256 _rate, string memory _metadataURI) public onlyOwner {
+ require(_rate > 0);
treatList[_name] = Treat(_name, _rate, _metadataURI);
treatNames.push(_name);
emit TreatAdded(_name, _rate, _metadataURI);
}
function setTreatCost(string memory _treatName, uint256 _cost) public onlyOwner {
+ require(_cost > 0);
require(treatList[_treatName].cost > 0, "Treat must cost something.");
treatList[_treatName].cost = _cost;
}