howlbot-integration[bot] commented 2 months ago

Lines of code

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

Vulnerability details

Impact

When a user/Merger calls the forgeWithListed function and sends more Ether than the required forging fee, the excess amount is not returned to the user. This could result in a loss of funds for users who inadvertently send more Ether.

Proof of Concept

Required forging fee: 0.1 Ether User sends: 1 Ether Excess fee: 0.9 Ether (not refunded)

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

// ...

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

// Missing code to return excess fee

Tools Used

Manual review

Recommended Mitigation Steps

  // Refund excess Ether
    if (msg.value > forgingFee) {
        (bool refundSuccess, ) = msg.sender.call{ value: msg.value - forgingFee }('');
        require(refundSuccess, 'Failed to refund excess Ether');
    }

Assessed type

ETH-Transfer