Duplicate NFT generation via repeated forging with the same parent

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/EntityForging/EntityForging.sol#L102-L144 https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/TraitForgeNft/TraitForgeNft.sol#L153-L179

Vulnerability details

Impact

It is possible to get the same NFT (same generation, same entropy) when the forging is done with the same parentId multiple times. Although the number of times this action will be feasible is limited with the forging potential of the token it can still ocurr at least once.

Scenario:

1. User A lists his/her NFT for forging
2. User B forges his/her NFT with the NFT from User A
3. The new NFT is created with the combined properties of the NFT1 and NFT2
4. User A lists his/her NFT for forging again
5. User B forges his/her NFT with the NFT from User A again
6. The NFT will be of the same generation and will have the same entropy as the one forged before

Proof of Concept

Add this test to the EntityForging.test.sol contract:

it('should not allow forging with the same parent multiple times', async () => {
      const forgerTokenId = FORGER_TOKEN_ID;
      const mergerTokenId = MERGER_TOKEN_ID;
      const fee = FORGING_FEE;

      await entityForging.connect(owner).listForForging(forgerTokenId, fee);

      const forgerEntropy = await nft.getTokenEntropy(forgerTokenId);
      const mergerEntrypy = await nft.getTokenEntropy(mergerTokenId);

      /// The new token id will be forger token id + 1, cause it's the last item
      const expectedTokenId = FORGER_TOKEN_ID + 1;
      await expect(
        entityForging
          .connect(user1)
          .forgeWithListed(forgerTokenId, mergerTokenId, {
            value: FORGING_FEE,
          })
      )
        .to.emit(entityForging, 'EntityForged')
        .withArgs(
          expectedTokenId,
          forgerTokenId,
          mergerTokenId,
          (forgerEntropy + mergerEntrypy) / 2n,
          FORGING_FEE
        )
        .to.emit(nft, 'NewEntityMinted')
        .withArgs(
          await user1.getAddress(),
          expectedTokenId,
          2,
          (forgerEntropy + mergerEntrypy) / 2n
        );

        // List it again
        await entityForging.connect(owner).listForForging(forgerTokenId, fee);
        const expectedId = FORGER_TOKEN_ID + 2;

        // Forge again with the same parent
        await expect(
          entityForging
            .connect(user1)
            .forgeWithListed(forgerTokenId, mergerTokenId, {
              value: FORGING_FEE,
            })
        )
          .to.emit(entityForging, 'EntityForged')
          .withArgs(
            expectedId,
            forgerTokenId,
            mergerTokenId,
            (forgerEntropy + mergerEntrypy) / 2n,
            FORGING_FEE
          )
          .to.emit(nft, 'NewEntityMinted')
          .withArgs(
            await user1.getAddress(),
            expectedId,
            2,
            (forgerEntropy + mergerEntrypy) / 2n
          );
    });

Tools Used

VS Code, Hardhat

Recommended Mitigation Steps

You could use a mapping(bytes32 => bool) forgedPairs to store token ids that have been forged before. An additional function to generate the unique key for id pairs would be necessary, for example:

keccak256(abi.encodePacked(id1 < id2 ? id1 : id2, id1 < id2 ? id2 : id1));

This type of hashing would ensure that you treat two pairs in reverse order as the same, since (entropy1 + entropy2) / 2 is the same as (entropy2 + entropy1) / 2.

You could than add require statement to check if the pair has already been used before. Don't forget to add the pairs to the mapping after they have been forged.

Assessed type

Error

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto marked the issue as selected for report

[c4-judge]

koolexcrypto changed the severity to 3 (High Risk)

[c4-judge]

koolexcrypto changed the severity to 2 (Med Risk)

[AtanasDimulski]

@koolexcrypto thanks for the swift judging. This is not a real issue, nowhere in the docs or in the code is it even slightly mentioned that minting with NFTs with the same parents is forbidden. There is no randomness at all in the project, as the entropies for all NFTs can be calculated before the user mints them, or forges. The entropy is used in different game mechanics but having the same entropy for NFTs is guaranteed for NFTs from different generations. I don't see how this is an issue, it doesn't lead to any lost funds, doesn't break any protocol invariants in the slightest, and there is no broken functionality. This is at best some design recommendation.

[samuraii77]

I agree, I also wanted to mention that, it's never mentioned that parents should not merge with each other more than once and it's in no way problematic.

As a matter of fact, it is completely expected for parents to merge more than once, those who have a brother or sister will probably agree with me.

[koolexcrypto]

Thank you both for your input.

Not every implementation in the code must be mentioned in the docs and the sponsor confirmed the issue. This confirm that it wasn't intended since we don't have in the docs or the code what states otherwise.

[AtanasDimulski]

Hey @koolexcrypto thanks for your feedback. However, I disagree with your decision. The only way this qualifies as a medium is if it was stated as an invariant of the protocol. The documentation was a google doc file, which the sponsor edited multiple times during the audit, and still didn't include this. The fact that this is undesired behavior was not disclosed in any public way before or during the audit. Given that this is an NFT game and developers can come up with whatever logic they want, they have to disclose all of their decisions before or during the audit. Lastly, I would like to ask since when whether the sponsors confirms something or not, decides whether an issue is valid or not. This issue doesn’t cover a single thing from the requirements for a medium issue, Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements, not even close. Someone could have reported that NFT with id 55 and NFT with id 77 shouldn't be forged if there is no full moon, and since the sponsor didn't explicitly say these NFTs can forge even without a full moon, should this also be considered a medium issue? Thanks for your time!

[Brivan-26]

Hi @koolexcrypto. This submission was more like a speculation of something that might be wrong. The documentation was very clear about how forging should happen and all its requirements (forging role, number of forging times per generation). The codebase was very clear on the forging process. If we ask the warden who submitted the report how he knows that this is an issue, he wouldn't have a clear response since he has no reference to go to that proves this is indeed an issue. We feel like this was intended behavior and the sponsor decided to change the forging behavior after reading this submission, so it was more like a feature suggestion.

[koolexcrypto]

Thank you for your input

From README

Core Mechanics Entities: NFTs with unique traits and parameters impacting the game.

Same generation and same entropy goes against this Mechanic. Therefore, I still see it an issue.