No way to return and withdraw excess amount in EntityForging::forgeWithListed, Locking of funds.

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

In EntityForging::forgeWithListed function a msg.sender can send money to forge with forgerId to generate a newTokenId to forge with any forgerId msg.sender have to pay some forgingFee amount. forgeWithListed also have a check that require(msg.value >= forgingFee, 'Insufficient fee for forging'); and except forgingFee the excess amount should return to msg.sender but missing this returning of msg.value-forgingFee to msg.sender cause locking of funds in EntityForging. No one can withdraw money from this contract anyway.

Proof of Concept

1. In forgeWithListed function msg.sender sent money.
2. checks for required forgingFee
3. And if msg.value-forgingFee > 0 then, there is no code to return excess money to msg.sender and not also any withdraw function to take money out of this contract.

See code below EntityForging::forgeWithListed :

code

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

    // according to docs devfee should only be 10% of the forgeFee Than why instead of takikng 10% we take taxCut as a state Variable and can be changed later??
    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
    );

    return newTokenId;
  }

Tools Used

Manual review

Recommended Mitigation Steps

Add the changes in the following code:

+   uint256 excessAmount = msg.value - forgingFee;
+   if(excessAmount > 0){
+      (bool success_return, ) = payable(msg.sender).call{ value: forgerShare }('');
+      require(success_return, 'Failed to send to msg.sender');
+   }

Assessed type

ETH-Transfer

[c4-judge]

koolexcrypto changed the severity to QA (Quality Assurance)

[c4-judge]

koolexcrypto marked the issue as grade-c

[c4-judge]

This previously downgraded issue has been upgraded by koolexcrypto

[c4-judge]

koolexcrypto marked the issue as duplicate of #687

[c4-judge]

koolexcrypto marked the issue as duplicate of #218

[c4-judge]

koolexcrypto changed the severity to QA (Quality Assurance)