c4-bot-4 commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/549e6891a6fcac4eed095b305f5cce8ca166ce51/contracts/EntityForging/EntityForging.sol#L102-L175

Vulnerability details

Impact

A forgerOwner can DOS another user's forgeWithListed function call when he does not want such user to use his listed forger token for forging even though such user should be allowed to forge with any forger token that is already listed.

Proof of Concept

When calling the following forgeWithListed function, (bool success_forge, ) = forgerOwner.call{ value: forgerShare }('') is executed, which calls the forgerOwner's receive function if the forgerOwner is a contract. In such receive function, the forgerOwner can conditionally revert it if he does not want the caller of the forgeWithListed function to use his listed forger token for forging, which causes such caller's forgeWithListed function call to be DOS'ed.

https://github.com/code-423n4/2024-07-traitforge/blob/549e6891a6fcac4eed095b305f5cce8ca166ce51/contracts/EntityForging/EntityForging.sol#L102-L175

  function forgeWithListed(
    uint256 forgerTokenId,
    uint256 mergerTokenId
  ) external payable whenNotPaused nonReentrant returns (uint256) {
    Listing memory _forgerListingInfo = listings[listedTokenIds[forgerTokenId]];
    require(
      _forgerListingInfo.isListed,
      "Forger's entity not listed for forging"
    );
    require(
      nftContract.ownerOf(mergerTokenId) == msg.sender,
      'Caller must own the merger token'
    );
    require(
      nftContract.ownerOf(forgerTokenId) != msg.sender,
      'Caller should be different from forger token owner'
    );
    require(
      nftContract.getTokenGeneration(mergerTokenId) ==
        nftContract.getTokenGeneration(forgerTokenId),
      'Invalid token generation'
    );

    uint256 forgingFee = _forgerListingInfo.fee;
    require(msg.value >= forgingFee, 'Insufficient fee for forging');

    _resetForgingCountIfNeeded(forgerTokenId); // Reset for forger if needed
    _resetForgingCountIfNeeded(mergerTokenId); // Reset for merger if needed

    // Check forger's breed count increment but do not check forge potential here
    // as it is already checked in listForForging for the forger
    forgingCounts[forgerTokenId]++;

    // Check and update for merger token's forge potential
    uint256 mergerEntropy = nftContract.getTokenEntropy(mergerTokenId);
    require(mergerEntropy % 3 != 0, 'Not merger');
    uint8 mergerForgePotential = uint8((mergerEntropy / 10) % 10); // Extract the 5th digit from the entropy
    forgingCounts[mergerTokenId]++;
    require(
      mergerForgePotential > 0 &&
        forgingCounts[mergerTokenId] <= mergerForgePotential,
      'forgePotential insufficient'
    );

    uint256 devFee = forgingFee / taxCut;
    uint256 forgerShare = forgingFee - devFee;
    address payable forgerOwner = payable(nftContract.ownerOf(forgerTokenId));

    uint256 newTokenId = nftContract.forge(
      msg.sender,
      forgerTokenId,
      mergerTokenId,
      ''
    );
    (bool success, ) = nukeFundAddress.call{ value: devFee }('');
    require(success, 'Failed to send to NukeFund');
@>  (bool success_forge, ) = forgerOwner.call{ value: forgerShare }('');
    require(success_forge, 'Failed to send to Forge Owner');

    // Cancel listed forger nft
    _cancelListingForForging(forgerTokenId);

    uint256 newEntropy = nftContract.getTokenEntropy(newTokenId);
    ...
    return newTokenId;
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Instead of reverting the forgeWithListed function call, such function can be updated to send forgerShare in WETH to the forgerOwner if executing forgerOwner.call{ value: forgerShare }('') reverts.

Assessed type

DoS

c4-judge commented 1 month ago

koolexcrypto marked the issue as unsatisfactory: Invalid

rbserver commented 1 month ago

Hi @koolexcrypto, thanks for your work!

The issue of this report is similar to #769, which all describe the issue of DOSing a merger's forgeWithListed function call. Would this report, #769, and other issues similar to #769 be reconsidered as a valid issue?

koolexcrypto commented 1 month ago

This is mentioned in the Bot report.

[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

rbserver commented 1 month ago

Hi @koolexcrypto, thanks for taking another look at this report!

I would like to add another comment regarding this report.

This report was similar to the validation repo's #769, which is now brought to the findings repo as #1053. This report, #1053, and the duplicates of #1053 describe that a malicious forgerOwner can DOS the merger's forgeWithListed function call at the forgerOwner's will, such as by conditionally reverting its receive function. This report is not about the forgerOwner consuming all transaction gas.

Can this report be a duplicate of #1053?

samuraii77 commented 1 month ago

I hope this finding along with the other mentioned finding won't be validated, they are at most QA. It's the same as the user frontrunning and cancelling his order, same impact and likelihood but that is obviously not valid as well, it's just common sense and not of any significance.