Beatland Festival

First Flight #44
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Submission Details
Impact: low
Likelihood: high
Invalid

Inefficient Iteration Over Unused Collection IDs in `FestivalPass::getUserMemorabiliaDetailed`

[L-1] Inefficient Iteration Over Unused Collection IDs in FestivalPass::getUserMemorabiliaDetailed.

Description: The FestivalPass::getUserMemorabiliaDetailed function loops from cId = 1 to cId < nextCollectionId even though the contract initializes FestivalPass::nextCollectionId = 100. This results in 99 unnecessary iterations over uninitialized FestivalPass::collectections[cId] entries. Since each iteration involves a nested loop and multiple balanceOf and FestivalPass::encodeTokenId calls, this adds significant overhead to what is expected to be a lightweight view function.

Impact: While this does not affect the correctness of the function or user balances, it:

  • Wastes gas in view simulations.

  • Slows down frontend responsiveness.

  • May lead to out-of-gas errors in off-chain tools or future on-chain calls if scaled further.

Proof of Concept: Add this into the FestivalPass.t.sol :

function test_GasUsedIngetUserMemorabiliaDetailed() public {
// Record gas before the function call
uint firstGas = gasleft();
// Call the function and store the returned arrays
(uint256[] memory tokenIds, uint256[] memory collectionIds, uint256[] memory itemIds) = festivalPass.getUserMemorabiliaDetailed(user2);
// Record gas after the function call
uint secondGas = gasleft();
// Log gas before, after, and total gas used
console.log("Gas first:", firstGas);
console.log("Gas after:", secondGas);
console.log("Gas consummed:", firstGas - secondGas);
}

This test measures how much gas is used when calling FestivalPass::getUserMemorabiliaDetailed(user2) by checking the gas before and after the call, then logging the difference. It's useful to see how expensive the function is.
Gas usage for a sample view call reached ~286,000 even without a valid collection.

Recommended Mitigation: Update the loop to start from the actual collection start ID:

- for (uint256 cId = 1; cId < nextCollectionId; cId++) {
+ for (uint256 cId = 100; cId < nextCollectionId; cId++) {

Alternatively, refactor the initial value of FestivalPass::nextCollectionId:

- uint256 public nextCollectionId = 100;
+ uint256 public nextCollectionId = 1;

Additionally, refactor FestivalPass::getUserMemorabiliaDetailed to eliminate the two-pass array allocation pattern and avoid redundant memory copying. A single-pass solution using inline assembly can significantly reduce memory operations and improve execution efficiency. This approach minimizes memory overhead, eliminates unnecessary condition checks, and scales better with user holdings especially in read-heavy environments like off-chain calls or dashboards.

function getUserMemorabiliaDetailed(address user)
external
view
returns (uint256[] memory tokenIds, uint256[] memory collectionIds, uint256[] memory itemIds)
{
uint256 start = 100;
uint256 max = nextCollectionId;
// Preallocate a large enough buffer in memory manually via assembly
uint256 maxSlots = 1000; // max collectibles a user can own (adjust safely)
assembly {
// Allocate memory for 3 arrays: tokenIds, collectionIds, itemIds
let ptr := mload(0x40) // Free memory pointer
// Reserve space for lengths + 3 arrays
// Format: [len][data ...] [len][data ...] [len][data ...]
// Each length takes 32 bytes; each data element takes 32 bytes
mstore(ptr, 0) // tokenIds length = 0
mstore(add(ptr, add(32, mul(maxSlots, 32))), 0) // collectionIds length = 0
mstore(add(ptr, add(64, mul(maxSlots, 64))), 0) // itemIds length = 0
}
uint256 found = 0;
for (uint256 cId = start; cId < max; ++cId) {
uint256 itemCount = collections[cId].currentItemId;
for (uint256 iId = 0; iId < itemCount; ++iId) {
uint256 tokenId = encodeTokenId(cId, iId);
if (balanceOf(user, tokenId) > 0) {
require(found < maxSlots, "Too many owned tokens");
assembly {
let base := mload(0x40)
// Store tokenId
mstore(add(base, add(32, mul(found, 32))), tokenId)
// Store collectionId
mstore(add(base, add(add(32, mul(maxSlots, 32)), add(32, mul(found, 32)))), cId)
// Store itemId
mstore(add(base, add(add(32, mul(maxSlots, 64)), add(32, mul(found, 32)))), iId)
// Update lengths
mstore(base, add(mload(base), 1)) // tokenIds length
mstore(add(base, add(32, mul(maxSlots, 32))), add(mload(add(base, add(32, mul(maxSlots, 32)))), 1)) // collectionIds length
mstore(add(base, add(64, mul(maxSlots, 64))), add(mload(add(base, add(64, mul(maxSlots, 64)))), 1)) // itemIds length
}
++found;
}
}
}
assembly {
// Return arrays using memory layout
let base := mload(0x40)
let size := add(96, mul(found, 96)) // estimate full size
// Update free memory pointer
mstore(0x40, add(base, size))
// Return data
tokenIds := base
collectionIds := add(base, add(32, mul(maxSlots, 32)))
itemIds := add(base, add(64, mul(maxSlots, 64)))
}
}

Note: Gas fees went from ~300,000 to ~40,000 after testing with this code.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 month ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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