Lines of code

https://github.com/code-423n4/2024-06-panoptic/blob/153f0d82440b7e63075d55b0659706531431145f/contracts/base/FactoryNFT.sol#L139-L147

Vulnerability details

Impact

FactoryNFT contract (@contracts\base\FactoryNFT.sol:139-147)

svgOut = svgOut
    .replace(
        "<!-- TEXT -->",
        metadata[bytes32("descriptions")][lastCharVal + 16 * (rarity / 8)]
            .decompressedDataStr()
    )
    .replace("<!-- ART -->", metadata[bytes32("art")][lastCharVal].decompressedDataStr())
    .replace("<!-- FILTER -->", metadata[bytes32("filters")][rarity].decompressedDataStr());

The problem is with the index calculation lastCharVal + 16 * (rarity / 8) when accessing the metadata[bytes32("descriptions")] array.

If the value of lastCharVal is large enough, it could cause an integer overflow when added to 16 * (rarity / 8). This could lead to even out-of-bounds access to the metadata[bytes32("descriptions")] array.

Proof of Concept

If the value of lastCharVal is large enough, the index calculation lastCharVal + 16 * (rarity / 8) can result in an integer overflow. This means that the calculated index could wrap around and become a smaller value than intended. As a result, the contract might access an element outside the valid range of the metadata[bytes32("descriptions")] array.

For example, let's say the metadata[bytes32("descriptions")] array has a length of 100. If lastCharVal is 2^256 - 20 (a very large value) and rarity is 24, the calculated index would be.

index = (2^256 - 20) + 16 * (24 / 8)
      = (2^256 - 20) + 48
      = 28 (due to integer overflow)

In this case, the contract would access metadata[bytes32("descriptions")][28], which is within the array bounds but not the intended element.

To illustrate the concept further

// Assume metadata[bytes32("descriptions")] has a length of 100
string[] memory descriptions = new string[](100);

// Set some example data
descriptions[0] = "Description 1";
descriptions[1] = "Description 2";
...
descriptions[99] = "Description 100";

// Assume lastCharVal is a very large value, e.g., 2^256 - 20
uint256 lastCharVal = type(uint256).max - 19;

// Assume rarity is 24
uint256 rarity = 24;

// Calculate the index
uint256 index = lastCharVal + 16 * (rarity / 8);

// Access the description using the calculated index
string memory description = descriptions[index];

In this example, index will be calculated as 28 due to integer overflow, and description will hold the value "Description 29" (assuming it exists), which is not the intended description for the given lastCharVal and rarity.

Tools Used

Vs Code FactoryNFT contract (@contracts\base\FactoryNFT.sol:58-112)

Recommended Mitigation Steps

Ensure that the index calculation is performed safely and within the bounds of the array. You can use the SafeMath library or perform explicit bounds checking to prevent integer overflow and ensure the index stays within the valid range of the array.

svgOut = svgOut
    .replace(
        "<!-- TEXT -->",
        metadata[bytes32("descriptions")][Math.min(lastCharVal + 16 * (rarity / 8), metadata[bytes32("descriptions")].length - 1)]
            .decompressedDataStr()
    )
    .replace("<!-- ART -->", metadata[bytes32("art")][lastCharVal].decompressedDataStr())
    .replace("<!-- FILTER -->", metadata[bytes32("filters")][rarity].decompressedDataStr());

Assessed type

Math

code-423n4 / 2024-06-panoptic-validation

Integer Overflow in Metadata Access Logic ([bytes32("descriptions")]) #13