user funds will be locked in EntityForging contract if user calls EntityForging::forgeWithListed function with excess eth

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

If user calls EntityForging::forgeWithListed function with msg.value > forgingFee then remaining eth will be permanantly locked in EntityForging contract so user funds get lost

Proof of Concept

EntityForging function checks weather msg.value>=forgingFee and reverts if msg.value<forgingFee.
https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/EntityForging/EntityForging.sol#L125-L126
and sends devFee to nukeFundAddress, forgerShare to forgerOwner(forgingFee = forgerShare+devFee)
https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/EntityForging/EntityForging.sol#L146-L159
but remaining eth if any (msg.value-forgingFee) is not sending back to msg.sender.
and there is no other way to get back those funds from the contract as there is no withdraw function or upgradability pattern, so the user funds get locked in EntityForging contract forever.

Tools Used

manual review

Recommended Mitigation Steps

add this code snippet in EntityForging::forgeWithListed function by following Checks effects interactions(CEI) pattern,
uint256 excessPayment = msg.value - forgingFee;
if (excessPayment > 0) {
(bool refundSuccess, ) = payable(msg.sender).call{ value: excessPayment }('');
require(refundSuccess, 'Refund of excess payment failed.');
}
so by this remaining eth will be sent back 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 #687

[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)