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

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

In forgeWithListed function msg.sender sent money.
checks for required forgingFee
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

```javascript 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