howlbot-integration[bot] commented 2 months ago

Lines of code

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

Vulnerability details

Impact

The forgeWithListed function in the EntityForging contract allows an entity with the merger role to forge a listed token with its own token. The function requires the merger to pay the forging fee defined by the forger. The issue is that the function does not refund the merger for any excess payment:

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

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

    emit EntityForged(
      newTokenId,
      forgerTokenId,
      mergerTokenId,
      newEntropy,
      forgingFee
    );

    // @audit Excees payment is not returned back to the merger
    return newTokenId;
  }

According to the following check require(msg.value >= forgingFee, 'Insufficient fee for forging');, the merger can send ETH above the required fee, resulting in the excess payment being stuck in the contract and not returned to the merger.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider either restricting the payment to exactly the forgingFee or refunding the merger for any excess payment.

Assessed type

ETH-Transfer