howlbot-integration[bot] commented 2 months ago

Lines of code

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

Vulnerability details

Impact

Forging fees remaining in the contract may not be refunded to the caller and may be permanently locked in the contract.

Proof of Concept

The EntityForging#forgeWithListed() is a function where a Merger is forged with a Forger.

  function forgeWithListed(
    uint256 forgerTokenId,
    uint256 mergerTokenId
  ) external payable whenNotPaused nonReentrant returns (uint256) {
    SNIP...
--> uint256 forgingFee = _forgerListingInfo.fee;
    require(msg.value >= forgingFee, 'Insufficient fee for forging');

    SNIP...

--> 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');

    SNIP...
  }

As you can see, in the provided code snippet, only the corresponding forgingFee is expected to be sent to the NukeFund contract and forgerOwner regardless of the transmitted ETH (msg.value). If forgingFee is reset before the transaction is executed for various reasons, there may be residual ETH left in the contract.

Example:

Let's say Alice sets fee to 0.1 ether using the listForForging() function.
Bob calls the forgeWithListed() function with msg.value = 0.1 ether to forge Alice's NFT.
Let's say that the cancelListingForForging() function is called for various reasons (such as selling an NFT or incorrectly setting parameters) before the Bob's transaction is executed, and the listForForging() function is called again to reset the fee to 0.08 ether.
When Bob's transaction is executed, 0.02 ether will remain in the contract.
However, since the remaining funds are not refunded to the caller and a mechanism to recover ether is not implemented within the contract, the remaining funds are permanently locked within the contract.

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to implement a mechanism to refund the remaining funds to the caller.

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

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

    SNIP...

    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');

+++ uint256 excessPayment = msg.value - forgingFee;
+++ if (excessPayment > 0) {
+++   (bool refundSuccess, ) = msg.sender.call{ value: excessPayment }('');
+++   require(refundSuccess, 'Refund of excess payment failed.');
+++ }
    SNIP...
  }

Assessed type

Payable