howlbot-integration[bot] commented 2 months ago

Lines of code

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

Vulnerability details

Impact

The forgeWithListed function enables users to breed an owned merger token with a listed forger token by paying a forgingFee. However, in the current implementation, any excess ETH sent by the user beyond the forgingFee gets locked within the EntityForging contract and cannot be recovered. To prevent this issue, any surplus ETH should be returned to the msg.sender.

  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;
    // @> Any value above forgingFee is lost forever
    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;
  }

Proof of Concept

Add the following test to EntityForging.test.ts:

In this test, user1 sends twice the requested forgingFee. An amount equal to the forgingFee is split between the forger token owner and the dev fund (as intended), while the remaining amount is locked in the EntityForging contract.

   it('does NOT return leftover ETH', async () => {
      const forgerTokenId = FORGER_TOKEN_ID;
      const mergerTokenId = MERGER_TOKEN_ID;

      // List token for forging
      await entityForging
        .connect(owner)
        .listForForging(forgerTokenId, FORGING_FEE);

      // Get entityForging contract and user1 balances before the forging
      const user1InitialBalance = (await ethers.provider.getBalance(
        user1.address
      )) as bigint;
      const forgingContractInitialBalance = await ethers.provider.getBalance(
        await entityForging.getAddress()
      );
      console.log({
        user1InitialBalance: ethers.formatUnits(user1InitialBalance),
        forgingContractInitialBalance: ethers.formatUnits(
          forgingContractInitialBalance
        ),
      });

      // Forge tokens sending twice the forgingFee
      const tx = await entityForging
        .connect(user1)
        .forgeWithListed(forgerTokenId, mergerTokenId, {
          value: 2n * FORGING_FEE, // Sending 2 times the FORGING_FEE
        });
      const receipt = await tx.wait(1);

      const user1FinalBalance = await ethers.provider.getBalance(user1.address);
      const forgingContractFinalBalance = await ethers.provider.getBalance(
        await entityForging.getAddress()
      );
      console.log({
        user1FinalBalance: ethers.formatUnits(user1FinalBalance),
        forgingContractFinalBalance: ethers.formatUnits(
          forgingContractFinalBalance
        ),
      });

      // We expect the user1 balance to be down twice the FORGING_FEE (plus the tx fee),
      // and the forgingContract balance to have increased by FORGING_FEE, meaning funds
      // got locked into the contract.
      expect(
        forgingContractFinalBalance - forgingContractInitialBalance
      ).to.be.eq(FORGING_FEE);
      expect(user1FinalBalance).to.be.eq(
        user1InitialBalance -
          2n * FORGING_FEE -
          receipt!.gasUsed * receipt!.gasPrice
      );
    });

Tools Used

Manual review

Recommended Mitigation Steps

There are at least 2 possibilities:

The user is requested to send the exact amount of forging fee

- require(msg.value >= forgingFee, 'Insufficient fee for forging');
+ require(msg.value == forgingFee, 'Wrong fee for forging');

The function allows sending more than the required forging fee but refunds the user the leftover ETH.

 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 remainingFee = msg.value - forgingFee;
    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');

+    if (remainingFee > 0) {
+      (bool success_remaining, ) = payable(msg.sender).call{ value: remainingFee }('');
+      require(success_remaining, 'Failed to send remaining to msg.sender');
+    }

    // Cancel listed forger nft
    _cancelListingForForging(forgerTokenId);

    uint256 newEntropy = nftContract.getTokenEntropy(newTokenId);

    emit EntityForged(
      newTokenId,
      forgerTokenId,
      mergerTokenId,
      newEntropy,
      forgingFee
    );

    return newTokenId;
  }

Assessed type

Other