Excess ether received in EntityForging::forgeWithListed gets locked in the contract

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

If a user forges a new entity using the function EntityForging::forgeWithListed sending more ether than the specified forging fee, the excess amount gets locked in the contract and there is no way of recovering the funds.

Proof of Concept

In the following foundry test a forge is done sending 0.04 more ether than needed, and the funds end up locked in the contract's balance.

pragma solidity ^0.8.13;

import {VmSafe} from "forge-std/Vm.sol";
import {Test, console} from "forge-std/Test.sol";
import "../contracts/EntityForging/EntityForging.sol";
import "../contracts/TraitForgeNft/TraitForgeNft.sol";
import "../contracts/EntropyGenerator/EntropyGenerator.sol";
import "../contracts/Airdrop/Airdrop.sol";

contract EntityForgingTest is Test {
    function test_excess_ether() public {
        // set any block to be sure the test is reproducible
        // in other environments
        vm.roll(1);

        // initialize nft contract
        TraitForgeNft nft = new TraitForgeNft();

        // initialize entity forging
        EntityForging entityForging = new EntityForging(address(nft));
        nft.setEntityForgingContract(address(entityForging));

        // initialize entropy generator
        EntropyGenerator generator = new EntropyGenerator(address(this));
        generator.writeEntropyBatch1();
        generator.writeEntropyBatch2();
        generator.writeEntropyBatch3();
        generator.initializeAlphaIndices();
        generator.setAllowedCaller(address(nft));
        nft.setEntropyGenerator(address(generator));

        // initialize airdrop
        Airdrop airdrop = new Airdrop();
        airdrop.transferOwnership(address(nft));
        nft.setAirdropContract(address(airdrop));

        // go 2 days to the future to avoid whitelist
        vm.warp(2*24*60*60);

        bytes32[] memory emptyProof;

        // two accounts are used in the test: forger and minter

        // initialize accounts and mint nfts
        VmSafe.Wallet memory forger = vm.createWallet("forger");
        vm.deal(forger.addr, 100 ether);
        vm.startPrank(forger.addr);
        nft.mintToken{value: nft.calculateMintPrice()}(emptyProof);
        nft.mintToken{value: nft.calculateMintPrice()}(emptyProof);
        nft.mintToken{value: nft.calculateMintPrice()}(emptyProof);
        nft.mintToken{value: nft.calculateMintPrice()}(emptyProof);
        nft.mintToken{value: nft.calculateMintPrice()}(emptyProof); // forger
        
        // list for forging
        entityForging.listForForging(5, 0.01 ether);
        vm.stopPrank();

        VmSafe.Wallet memory merger = vm.createWallet("merger");
        vm.deal(merger.addr, 100 ether);
        vm.startPrank(merger.addr);
        nft.mintToken{value: nft.calculateMintPrice()}(emptyProof); // merger


        // forge a new nft
        // sends 0.04 ether more than needed
        entityForging.forgeWithListed{value: 0.05 ether}(5, 6);
        vm.stopPrank();

        // 0.04 ether are locked in the contract
        assertEq(address(entityForging).balance, 0.04 ether);
    }
}

Tools Used

Foundry

Recommended Mitigation Steps

Check that msg.value equals the forging fee.

diff --git a/contracts/EntityForging/EntityForging.sol b/contracts/EntityForging/EntityForging.sol
index c9ce23b..07c3d24 100644
--- a/contracts/EntityForging/EntityForging.sol
+++ b/contracts/EntityForging/EntityForging.sol
@@ -123,7 +123,7 @@ contract EntityForging is IEntityForging, ReentrancyGuard, Ownable, Pausable {
     );
 
     uint256 forgingFee = _forgerListingInfo.fee;
-    require(msg.value >= forgingFee, 'Insufficient fee for forging');
+    require(msg.value == forgingFee, 'Invalid fee for forging');
 
     _resetForgingCountIfNeeded(forgerTokenId); // Reset for forger if needed
     _resetForgingCountIfNeeded(mergerTokenId); // Reset for merger if needed

Assessed type

Payable

[koolexcrypto]

Findings that require the user to be careless or enter the wrong information into a contract call are QA or Invalid.

https://docs.code4rena.com/awarding/judging-criteria/severity-categorization

Setting this as QA based on above criteria.

[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 not a duplicate

[c4-judge]

koolexcrypto marked the issue as selected for report

[c4-judge]

koolexcrypto marked the issue as not selected for report

[c4-judge]

koolexcrypto changed the severity to QA (Quality Assurance)

[c4-judge]

koolexcrypto marked the issue as grade-c