Missing Refund Logic in forgeWithListed May Result in Fund Loss

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

In the EntityForging.forgeWithListed() function, there is no proper refund logic to handle cases where the amount of ETH sent exceeds the required forging fee. This oversight leads to any excess ETH being stuck in the contract indefinitely, resulting in potential financial loss for users who accidentally overpay.

Proof of Concept

To demonstrate this issue, the following Hardhat test case shows how excess ETH sent to the contract during a forging operation remains trapped:

import { parseEther } from 'ethers';
import {
  Airdrop,
  DevFund,
  EntityForging,
  EntropyGenerator,
  EntityTrading,
  NukeFund,
  TraitForgeNft,
} from '../typechain-types';
import { HardhatEthersSigner } from '@nomicfoundation/hardhat-ethers/signers';
import generateMerkleTree from '../scripts/genMerkleTreeLib';

const { ethers } = require('hardhat');

describe('EntityForgingPoC', () => {
  let entityForging: EntityForging;
  let nft: TraitForgeNft;
  let owner: HardhatEthersSigner;
  let user1: HardhatEthersSigner;
  let entityTrading: EntityTrading;
  let nukeFund: NukeFund;
  let devFund: DevFund;
  let FORGER_TOKEN_ID: number;
  let MERGER_TOKEN_ID: number;

  const FORGING_FEE = ethers.parseEther('1.0'); // 1 ETH

  before(async () => {
    [owner, user1] = await ethers.getSigners();

    // Deploy TraitForgeNft contract
    const TraitForgeNft = await ethers.getContractFactory('TraitForgeNft');
    nft = (await TraitForgeNft.deploy()) as TraitForgeNft;

    // Deploy Airdrop contract
    const airdropFactory = await ethers.getContractFactory('Airdrop');
    const airdrop = (await airdropFactory.deploy()) as Airdrop;

    await nft.setAirdropContract(await airdrop.getAddress());
    await airdrop.transferOwnership(await nft.getAddress());

    // Deploy EntropyGenerator contract
    const EntropyGenerator = await ethers.getContractFactory(
      'EntropyGenerator'
    );
    const entropyGenerator = (await EntropyGenerator.deploy(
      await nft.getAddress()
    )) as EntropyGenerator;

    await entropyGenerator.writeEntropyBatch1();
    await nft.setEntropyGenerator(await entropyGenerator.getAddress());

    // Deploy EntityForging contract
    const EntityForging = await ethers.getContractFactory('EntityForging');
    entityForging = (await EntityForging.deploy(
      await nft.getAddress()
    )) as EntityForging;
    await nft.setEntityForgingContract(await entityForging.getAddress());

    devFund = await ethers.deployContract('DevFund');
    await devFund.waitForDeployment();

    const NukeFund = await ethers.getContractFactory('NukeFund');

    nukeFund = (await NukeFund.deploy(
      await nft.getAddress(),
      await airdrop.getAddress(),
      await devFund.getAddress(),
      owner.address
    )) as NukeFund;
    await nukeFund.waitForDeployment();

    await nft.setNukeFundContract(await nukeFund.getAddress());

    entityTrading = await ethers.deployContract('EntityTrading', [
      await nft.getAddress(),
    ]);

    // Set NukeFund address
    await entityTrading.setNukeFundAddress(await nukeFund.getAddress());

    const merkleInfo = generateMerkleTree([owner.address, user1.address]);

    await nft.setRootHash(merkleInfo.rootHash);

    // Mint some tokens for testing
    await nft.connect(owner).mintToken(merkleInfo.whitelist[0].proof, {
      value: ethers.parseEther('1'),
    });
    await nft.connect(user1).mintToken(merkleInfo.whitelist[1].proof, {
      value: ethers.parseEther('1'),
    });
    await nft.connect(user1).mintToken(merkleInfo.whitelist[1].proof, {
      value: ethers.parseEther('1'),
    });

    for (let i = 0; i < 10; i++) {
      await nft.connect(owner).mintToken(merkleInfo.whitelist[0].proof, {
        value: ethers.parseEther('1'),
      });
      const isForger = await nft.isForger(i + 4);
      if (isForger) {
        FORGER_TOKEN_ID = i + 4;
        break;
      }
    }

    MERGER_TOKEN_ID = 3;
  });

  it('poc excess spending not refunded', async () => {
    // list the token for forging
    await entityForging
      .connect(owner)
      .listForForging(FORGER_TOKEN_ID, FORGING_FEE);

    const forgerTokenId = FORGER_TOKEN_ID;
    const mergerTokenId = MERGER_TOKEN_ID;

    const initialBalance = await ethers.provider.getBalance(user1.address);

    await entityForging
      .connect(user1)
      .forgeWithListed(forgerTokenId, mergerTokenId, {
        // send more than the forging fee
        value: FORGING_FEE + parseEther('1'),
      });

    const balanceAfter = await ethers.provider.getBalance(user1.address);

    const stuckInContract = await ethers.provider.getBalance(
      await entityForging.getAddress()
    );

    console.log('forging fee:', FORGING_FEE.toString());
    console.log('user 1 spent:', (initialBalance - balanceAfter).toString()); // unrefunded amount + gas fee
    console.log('stuck in contract: ', stuckInContract.toString()); // stuck in contract forever
  });
});

Test Output

In the test, user1 calls forgeWithListed() with msg.value = FORGING_FEE + 1 ETH, sending 1 ETH more than required. As a result, the user ends up spending FORGING_FEE + gas fee + 1 ETH, with the excess 1 ETH remaining stuck in the contract:

EntityForgingPoC
Here is Root Hash: 0x070e8db97b197cc0e4a1790c5e6c3667bab32d733db7f815fbe84f5824c7168d
forging fee: 1000000000000000000
user 1 spent: 2000519148050517142
stuck in contract:  1000000000000000000
    ✔ poc excess spending not refunded

Tools Used

• Manual Review
• Hardhat Test

Recommended Mitigation Steps

To address this issue, consider the following mitigation strategies:

1. Strict Fee Matching:
   Modify the require statement in the forgeWithListed() function to ensure that msg.value is exactly equal to 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, 'Incorrect forging fee provided');
   
       _resetForgingCountIfNeeded(forgerTokenId); // Reset for forger if needed
       _resetForgingCountIfNeeded(mergerTokenId); // Reset for merger if needed

2. Refund Excess Amount:
   Implement a refund mechanism that automatically returns any excess ETH sent to msg.sender. This can be done by subtracting the forging fee from msg.value and transferring the difference back to the user:

diff --git a/contracts/EntityForging/EntityForging.sol b/contracts/EntityForging/EntityForging.sol
index c9ce23b..580419c 100644
--- a/contracts/EntityForging/EntityForging.sol
+++ b/contracts/EntityForging/EntityForging.sol
@@ -125,6 +125,13 @@ contract EntityForging is IEntityForging, ReentrancyGuard, Ownable, Pausable {
     uint256 forgingFee = _forgerListingInfo.fee;
     require(msg.value >= forgingFee, 'Insufficient fee for forging');

+    // Refund any excess ETH
+    uint256 excessAmount = msg.value - forgingFee;
+    if (excessAmount > 0) {
+        (bool success, ) = payable(msg.sender).call{value: excessAmount}("");
+        require(success, "Refund failed");
+    }
+
     _resetForgingCountIfNeeded(forgerTokenId); // Reset for forger if needed
     _resetForgingCountIfNeeded(mergerTokenId); // Reset for merger if needed

By implementing one of these changes, you can ensure that users are not overcharged and that excess funds do not remain locked in the contract.

Assessed type

Payable

[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 marked the issue as duplicate of #218

[c4-judge]

koolexcrypto changed the severity to QA (Quality Assurance)