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

Overcharging players resulting in Financial Imbalance in EntityForging in the forgeWithListed function [ready]

Impact: In EntityForging::forgeWithListed, the protocol checks that msg.value is equal to or greater than the forging fee. This often results in users paying more than required, as there is no mechanism to refund the excess amount. players that are perticipating in this game will someday lose interest in the game if they are been charged more than the initial amount the protocol wascsupposed to charge them

Proof of Concept:

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

Tool used manual review

Recommended Mitigation: since the protocol used >= to validate how much a player can forge with is essential that the protocol also add an implementation that accounts and send back players exccess back to them.

Assessed type

Other