howlbot-integration[bot] commented 2 months ago

Lines of code

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

Vulnerability details

Impact

Excess Funds are always lost, so users lost fund.

Proof of Concept

In forgeWithListed() users pay forgingFee and merge NFT. But in here remaining funds excluding forgingFee will not be returned.

  function forgeWithListed(
    uint256 forgerTokenId,
    uint256 mergerTokenId
  ) external payable whenNotPaused nonReentrant returns (uint256) {
    ...

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

    ...
    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");

    ...
  }

As you can see above, only the specific forgingFee should be sent to the NukeFund contract and the forgerOwner, regardless of the amount of ETH (msg.value) being transmitted. If the forgingFee is changed for any reason before the transaction is executed, there might be leftover ETH in the contract As a result users suffer from loss of fund.

Tools Used

Mannual Review

Recommended Mitigation Steps

forgeWithListed()is modified as follow.

  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");
+   uint256 excessPayment = msg.value - forgingFee;
+   if (excessPayment > 0) {
+     (bool refundSuccess, ) = msg.sender.call{ value: excessPayment }("");
+     require(refundSuccess, "Refund of excess payment failed.");
+   }

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

...
    return newTokenId;
  }

Assessed type

ETH-Transfer