Lack of refund mechanism for overpayment in EntityForging::forgeWithListed

[howlbot-integration]

Lines of code

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

Vulnerability details

Summary

EntityForging::forgeWithListed lets users to send more ETH to the contract than the fee for the forging they intend to make.

Impact

The contract does not account for or refund any excess Ether sent by Forgers beyond the required forging fee. This could result in users inadvertently losing funds if they send more than the required fee, as there is no mechanism to return the extra Ether.

Proof of Concept

In the current implementation of forgeWithListed, the function checks if msg.value is at least equal to forgingFee:

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

  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]++;

    . . .
  }

However, if msg.value exceeds forgingFee, the contract does not refund the excess amount. This could lead to users losing extra funds sent beyond the required fee.

Tools Used

Manual review

Recommended Mitigation Steps

Modify EntityForging::forgeWithListed to reject payments that exceed forgingFee, or send back the extra funds to the user.

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

This will ensure that users are protected from losing their funds due to overpayment, and will help to maintain trust in the contract.

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 #218

[c4-judge]

koolexcrypto marked the issue as duplicate of #218

[c4-judge]

koolexcrypto changed the severity to QA (Quality Assurance)