ETH will get stuck/locked in the EntityForging contract

[c4-bot-10]

Lines of code

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

Vulnerability details

Vulnerability Details

In the EntityForging contract, the forgeWithListed is payable since the caller has to send the native token to pay for the forging fee. However, there is only a check that the msg.value is greater or equal than the required fee. In case a user sends more msg.value than needed, there is no refund mechanism or even a way to get the native token out of the contract. This is inconsistent with the rest of the protocol's payable functions, which either have a refund mechanism or only check that the msg.value is equal to the required amount.

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

        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;
    }

Impact

Users' excess ETH will remain locked and lost in the EntityForging contract without a way for anyone to get it out.

Proof of Concept

I'm providing my full foundry test file:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;

import "forge-std/Test.sol";
import {console2} from "forge-std/Test.sol";
import {EntropyGenerator} from "../contracts/EntropyGenerator/EntropyGenerator.sol";
import {DevFund} from "../contracts/DevFund/DevFund.sol";
import {Airdrop} from "../contracts/Airdrop/Airdrop.sol";
import {TraitForgeNft} from "../contracts/TraitForgeNft/TraitForgeNft.sol";
import {EntityForging} from "../contracts/EntityForging/EntityForging.sol";
import {IEntityForging} from "../contracts/EntityForging/IEntityForging.sol";
import {NukeFund} from "../contracts/NukeFund/NukeFund.sol";
import {EntityTrading} from "../contracts/EntityTrading/EntityTrading.sol";
//import {IEntityTrading} from "../contracts/EntityTrading/IEntityTrading.sol";
import {TraitForgeNftMock} from "../contracts/TraitForgeNft/TraitForgeNftMock.sol";

contract ContractBTest is Test {
    address public owner = makeAddr("owner");
    address player1 = makeAddr("player1");
    address player2 = makeAddr("player2");

    EntityForging entityForging;
    TraitForgeNft traitForgeNft;
    EntropyGenerator entropyGenerator;
    Airdrop airdrop;
    DevFund devFund;
    NukeFund nukeFund;
    EntityTrading entityTrading;

    function setUp() public {
        vm.startPrank(owner);
        // Deploy TraitForgeNft contract
        traitForgeNft = new TraitForgeNft();

        // Deploy Airdrop contract
        airdrop = new Airdrop();
        traitForgeNft.setAirdropContract(address(airdrop));
        airdrop.transferOwnership(address(traitForgeNft));

        // Deploy entropyGenerator contract
        entropyGenerator = new EntropyGenerator(address(traitForgeNft));

        traitForgeNft.setEntropyGenerator(address(entropyGenerator));

        // Deploy EntityForging contract
        entityForging = new EntityForging(address(traitForgeNft));
        traitForgeNft.setEntityForgingContract(address(entityForging));

        devFund = new DevFund();
        nukeFund = new NukeFund(address(traitForgeNft), address(airdrop), payable(address(devFund)), payable(owner));
        traitForgeNft.setNukeFundContract(payable(address(nukeFund)));

        entityTrading = new EntityTrading(address(traitForgeNft));
        entityTrading.setNukeFundAddress(payable(address(nukeFund)));

        nukeFund.setAgeMultplier(1);
        vm.stopPrank();

        // to skip whitelist
        vm.warp(block.timestamp + 3 days);
    }

    function testStuckEth() public {
        bytes32[] memory proof;

        // this is a block I found that the first 2 entropies are: 1. forger role, 2. merger role
        vm.roll(6);
        entropyGenerator.writeEntropyBatch1();

        vm.deal(player1, 10 ether);
        vm.deal(player2, 10 ether);
        assertEq(player2.balance, 10 ether);

        vm.startPrank(player1);
        traitForgeNft.mintToken{value: 1 ether}(proof);

        entityForging.listForForging(1, 1 ether);
        vm.stopPrank();

        vm.startPrank(player2);
        traitForgeNft.mintToken{value: 1 ether}(proof);
        entityForging.forgeWithListed{value: 5 ether}(1, 2);
        vm.stopPrank();

        assert(player2.balance < 5 ether);
        assertEq(address(entityForging).balance, 4 ether);
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

1. Add a refund mechanism like the one in TraitForgeNft::mintToken function.
2. Simply allow only for the exact amount of msg.value

    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");
+       require(msg.value == forgingFee, "Insufficient fee for forging");
        .
        .
        .
    }

Assessed type

Other

[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 #218

[c4-judge]

koolexcrypto changed the severity to QA (Quality Assurance)