fetchListings() will become unusable after a certain point

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/EntityForging/EntityForging.sol#L48-L53

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

It appears that fetchListings() is used by off-chain software to fetch all forging listings.

Reference in code: link

  function fetchListings() external view returns (Listing[] memory _listings) {
@>  _listings = new Listing[](listingCount + 1);
    for (uint256 i = 1; i <= listingCount; ++i) {
      _listings[i] = listings[i];
    }
  }

There are a couple of low impact issues and a serious one with this function. Lets explore them all.

listingCount is the number of listings that were ever created. It increment each time a user creates a listing and it does not become lower once the listing is fulfilled or canceled.

The first issue is that we add 1 to listingCount when we define the length of the _listings array and we start the indexing of the loop from 1 instead of 0. This way the 0 index in the returned array will always be the default Listing struct values.

The second issue is that once listings are canceled/fulfilled we do not track in any way which are the active listings and we simply reset the struct Listing to its default values. This would make fetchListings() to return many empty Listing structs in the array.

The third serious issue is that over time the listingCount will become very large because it is ever increasing and due to the fact that 1 NFT can be listed, canceled and filled indefinitely. This combined with high forging activity from users will make listingCount (the loop iterations) and the returned data to be astronomical. This is an issue because even tough it is going to be called off chain this function will start reverting due to out of gas error. This is because off chain calls “simulate” how much gas it is needed for the call to be successfully executed and if its more than the current block gas limit it will revert.

In our case listingCount can only increase which means that fetchListings() will become unusable after some amount of listings have accumulated.

You can run this POC inside EntityForging.test.ts by running npx hardhat test --grep "POC fetchListings offchain calls will always revert” and see that the call will indeed revert! Note: You can use lower than 10000 value in order for the test not to take too long. I used this value to be sure that the call will consume more than the block gas limit.

describe('listForForging', () => {
		it('should not allow non-owners to list a token for forging', async () => {...})
		
+		it('POC fetchListings offchain calls will always revert', async () => {
+      const tokenId = FORGER_TOKEN_ID;
+      const fee = FORGING_FEE;

+      for (let index = 0; index < 10000; index++) {
+        console.log("index", index);
+        await entityForging.connect(owner).listForForging(tokenId, fee);
+        await entityForging.connect(owner).cancelListingForForging(tokenId);
+      }

+      const tx = await entityForging.fetchListings();
+      console.log("tx", tx);
+    });
    
    it('should allow the owner to list a token for forging', async () => {...});
    ...
});

Tools Used

Manual Review

Recommended Mitigation Steps

If you want to retrieve all listings that were ever created consider adding uint256 from and uint256 to as arguments to fetchListings(). This way the fetching can be done in batches and we can avoid reaching the block gas limit.

Assessed type

DoS

[c4-judge]

koolexcrypto changed the severity to QA (Quality Assurance)