howlbot-integration[bot] commented 2 months ago

Lines of code

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

Vulnerability details

Impact

The EntityForging contract requires the user (merger's owner) to provide ETH (in msg.value) for forgeWithListed call to pay the forger listing fee. But if the user has provided more ETH than the forgingFee, then this excess ETH isn't refunded back to user.

Proof of Concept

Observe the forgeWithListed function:

  function forgeWithListed(
    uint256 forgerTokenId,
    uint256 mergerTokenId
  ) external payable whenNotPaused nonReentrant returns (uint256) {
    Listing memory _forgerListingInfo = listings[listedTokenIds[forgerTokenId]];

    // ... 

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

    // ... 

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

    // ... 

  }

In the above function, the condition in the require statement allows the user to pass msg.value >= forgingFee, but in the ETH transfer processing step, this function only sends an exact amount equal to devFee + forgerShare = forgingFee, the excess ETH is not refunded back to the user.

Tools Used

Manually Review

Recommended Mitigation Steps

Solution 1: Force the msg.value must be equal to the forgingFee Fix the conditional in the require statement as:

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

Solution 2: Refund the excess ETH back to the user At the end of the forgeWithListed function, refund the excess ETH back to the user by adding this block of code:

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

Assessed type

ETH-Transfer