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

Owner Can Accidentally Add a Treat with the Same Name, Overwriting Existing Data

codedesignz

Description: The SpookySwap::treatList mapping uses the treat name as a key to store relevant Treat structs. Currently, there is no safeguard to prevent the owner from adding a new treat with a name that already exists in the mapping. As a result, adding a new treat with an existing name will overwrite the previous treat data with the new values, in addition to creating a new treat.

Impact: The original treat entry is unintentionally updated with the new treat’s values, leading to unintended data overwriting and loss of the original treat’s details.

Proof of Concept:

  1. The owner adds a treat named PumpkinBurst with a cost of 1 ether and metadata URI PumpkinBurstValley.com.

  2. The owner then accidentally adds another treat with the same name, PumpkinBurst.

  3. For the second treat, a cost of 3 ether and metadata URI PumpkinCity.com are used.

  4. This overwrites the original treat's data, resulting in both treats reflecting the latest cost and metadata URI values.

Proof of Code

Create a directory for the test file at the root level of the repository as test/unit. Create a new file named TrickOrTreatTest.t.sol inside this directory and paste the following code into this file.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.24;
​
import {Test, console2} from "forge-std/Test.sol";
import {SpookySwap} from "../../src/TrickOrTreat.sol";
import {Strings} from "@openzeppelin/contracts/utils/Strings.sol";
​
contract TrickOrTreatTest is Test {
using Strings for uint256;
​
SpookySwap spookySwap;
SpookySwap.Treat treat;
SpookySwap.Treat[] treatsList;
address owner = makeAddr("owner");
address nonowner = makeAddr("nonowner");
​
function setUp() public {
treat.name = "GoinLiveTreat";
treat.cost = 1 ether;
treat.metadataURI = "test.metadata.com";
treatsList.push(treat);
vm.prank(owner);
spookySwap = new SpookySwap(treatsList);
console2.log("SpookySwap address: ", address(spookySwap));
}
​
function testOwnerCanAddMultipleTreatsWithSameName() public {
vm.startPrank(owner);
// Owner adds a treat
spookySwap.addTreat("PumpkinBurst", 1 ether, "PumpkinBurstValley.com");
​
// Owner adds another treat with the same name but different cost and metadataURI
spookySwap.addTreat("PumpkinBurst", 3 ether, "PumpkinCity.com");
​
// Check the updated Treats
string[] memory totalTreats = spookySwap.getTreats();
​
// Since the setup of this test adds one treat while deployment of the contract, the new treats occupy second and third location
(, uint256 firstTreatCost, string memory firstTreatMetadataURI) = spookySwap.treatList(totalTreats[1]);
(, uint256 secondTreatCost, string memory secondTreatMetadataURI) = spookySwap.treatList(totalTreats[2]);
​
assertEq(firstTreatCost, secondTreatCost);
assertEq(firstTreatCost, 3 ether);
assertEq(firstTreatMetadataURI, secondTreatMetadataURI);
assertEq(firstTreatMetadataURI, "PumpkinCity.com");
}
}
​

Recommended Mitigation:
Introduce a new mapping treatExists in the SpookySwap contract to track names already used for treats. This mapping will prevent adding a treat with an existing name.

mapping(string => bool) private treatExists;`

Then, modify the SpookySwap::addTreat function to check treatExists to verify if a treat with the specified name already exists, and update treatExists when a new treat with a unique name is added.

function addTreat(string memory _name, uint256 _rate, string memory _metadataURI) public onlyOwner {
+ if (treatExists[_name]) {
+ revert("Treat with this name already exists");
+ }
treatList[_name] = Treat(_name, _rate, _metadataURI);
treatNames.push(_name);
+ treatExists[_name] = true; // Mark this name as added
emit TreatAdded(_name, _rate, _metadataURI);
}
Updates

Appeal created

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

[invalid] Duplicate treats

The function `addTreat` is called by the owner. The owner is trusted. There will be no duplicates.

Support

FAQs

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