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 player are consistently overcharged due to the lack of a refund mechanism in EntityForging::forgeWithListed this below require condition is expecting the fee to either be equal or greater than the expected fee without accounting for the excess to be transfered back to the player require(msg.value >= forgingFee, "Insufficient fee for forging"); transfering back to the player will bring trust and total accountability

Proof of Concept The vulnerable line is been pointed to, also you will notice the protocol failure to account for exccess to be sent back.

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

Recommended Mitigation: Either the check is changed to only equal to or implement a check that will give total account of the excess funds such validation was also used by the protocol to account for excess in TraitForge::mintToken`

Assessed type

Other