howlbot-integration[bot] commented 2 months ago

Lines of code

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

Vulnerability details

Impact

forgeWithListed accepts arbitrary msg.value and if forgingFee is less than that it will just hold stuck it inside the contract with no way of withdrawing it.

Proof of Concept

forgeWithListed can be called with an arbitrary value, where only forgingFee is taken from it and the rest is left stuck inside the contract.

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

    ...
    uint256 devFee = forgingFee / taxCut;
    uint256 forgerShare = forgingFee - devFee;
    address payable forgerOwner = payable(nftContract.ownerOf(forgerTokenId));

Here is an example where due to the a network congestion the merger losses some of his ETH:

NFT is listed for 1 ETH
Merger sends a TX, but due to network congestion (aka. expensive gas fees) the TX takes a few hours to execute
Forger's NFT is not merged, so he lists it again for a lower price of 0.9 ETH
Our merger TX finally gets executed and he buys the NFT for 1 ETH.

In the example above due to bad luck our merger lost 0.1 ETH. This should never be the case and the function should either return the extra or revert if the msg.value doesn't match the asked price.

Tools Used

Manual review

Recommended Mitigation Steps

Revert if msg.value doesn't match price.

-   require(msg.value >= forgingFee, "Insufficient fee for forging");
+   require(msg.value == forgingFee, "Incorrect amount sent");

Assessed type

Error