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

Gas Inefficiency in `getUserMemorabiliaDetailed()` Due to Unoptimized Looping and Storage Access

fffuuuming

Root + Impact

Description

  • The current implementation of getUserMemorabiliaDetailed() is highly gas inefficient.

  • Although the global state variable nextCollectionId starts from 100, the outer loop begins iteration from cId = 1, resulting in unnecessary loop executions over unused or non-existent collection IDs.

  • Additionally, both the outer and inner loops repeatedly access state variables (nextCollectionId and collections[cId].currentItemId), which are stored in contract storage. These epeated SLOAD operations in each iteration result in additional gas cost.

Risk

Likelihood:

  • This inefficiency incurs gas overhead every time getUserMemorabiliaDetailed() is invoked

Impact:

  • Based on the Known Issues, we know that the project pays attention to gas optimization, and this implementation is obviously inefficient

  • While this is not a direct security vulnerability, it increases gas costs for users interacting with the function on-chain

  • It may discourage usage if the function is part of a core user experience, such as displaying collected memorabilia

Proof of Concept

Add the following test, then run the command: forge test -vv --match-test test_GetUserMemorabiliaDetailed_GasCost

function test_GetUserMemorabiliaDetailed_GasCost() public {
// Create 2 collections
vm.startPrank(organizer);
festivalPass.createMemorabiliaCollection("Col1", "ipfs://1", 50e18, 5, true);
festivalPass.createMemorabiliaCollection("Col2", "ipfs://2", 75e18, 5, true);
vm.stopPrank();
​
// Analyze gas cost
uint256 gasStart = gasleft();
festivalPass.getUserMemorabiliaDetailed(user1);
uint256 gasUsed = gasStart - gasleft();
console.log("Gas used for getUserMemorabiliaDetailed: %s", gasUsed);
}

PoC Results:

forge test -vv --match-test test_GetUserMemorabiliaDetailed_GasCost
[⠊] Compiling...
[⠘] Compiling 1 files with Solc 0.8.25
[⠃] Solc 0.8.25 finished in 751.12ms
Ran 1 test for test/FestivalPass.t.sol:FestivalPassTest
[PASS] test_GetUserMemorabiliaDetailed_GasCost() (gas: 585043)
Logs:
Gas used for getUserMemorabiliaDetailed: 281061
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.34ms (1.29ms CPU time)
​
Ran 1 test suite in 227.47ms (7.34ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended Mitigation

  • Let cId start from 100, result gas cost:

    forge test -vv --match-test test_GetUserMemorabiliaDetailed_GasCost
    [⠊] Compiling...
    [⠘] Compiling 4 files with Solc 0.8.25
    [⠃] Solc 0.8.25 finished in 765.95ms
    Ran 1 test for test/FestivalPass.t.sol:FestivalPassTest
    [PASS] test_GetUserMemorabiliaDetailed_GasCost() (gas: 312397)
    Logs:
    Gas used for getUserMemorabiliaDetailed: 8415
    ​
    Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.66ms (1.23ms CPU time)
    ​
    Ran 1 test suite in 228.28ms (7.66ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

  • Furthermore, create a local copy of nextCollectionId and collections[cId].currentItemId, resulting gas cost:

    forge test -vv --match-test test_GetUserMemorabiliaDetailed_GasCost
    [⠊] Compiling...
    [⠘] Compiling 4 files with Solc 0.8.25
    [⠃] Solc 0.8.25 finished in 745.99ms
    Ran 1 test for test/FestivalPass.t.sol:FestivalPassTest
    [PASS] test_GetUserMemorabiliaDetailed_GasCost() (gas: 311957)
    Logs:
    Gas used for getUserMemorabiliaDetailed: 7975
    ​
    Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.07ms (1.02ms CPU time)
    ​
    Ran 1 test suite in 226.89ms (6.07ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Final version:

function getUserMemorabiliaDetailed(address user) external view returns (
uint256[] memory tokenIds,
uint256[] memory collectionIds,
uint256[] memory itemIds
) {
uint256 count = 0;
+ uint256 localNextCollectionId = nextCollectionId;
+ for (uint256 cId = 100; cId < localNextCollectionId; cId++) {
- for (uint256 cId = 1; cId < nextCollectionId; cId++) {
+ uint256 currentItemId = collections[cId].currentItemId;
+ for (uint256 iId = 1; iId < currentItemId; iId++) {
- for (uint256 iId = 1; iId < collections[cId].currentItemId; iId++) {
uint256 tokenId = encodeTokenId(cId, iId);
if (balanceOf(user, tokenId) > 0) {
count++;
}
}
}
​
tokenIds = new uint256[](count);
collectionIds = new uint256[](count);
itemIds = new uint256[](count);
​
uint256 index = 0;
+ for (uint256 cId = 100; cId < localNextCollectionId; cId++) {
- for (uint256 cId = 1; cId < nextCollectionId; cId++) {
+ uint256 currentItemId = collections[cId].currentItemId;
+ for (uint256 iId = 1; iId < currentItemId; iId++) {
- for (uint256 iId = 1; iId < collections[cId].currentItemId; iId++) {
uint256 tokenId = encodeTokenId(cId, iId);
if (balanceOf(user, tokenId) > 0) {
tokenIds[index] = tokenId;
collectionIds[index] = cId;
itemIds[index] = iId;
index++;
}
}
}
​
return (tokenIds, collectionIds, itemIds);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 4 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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