Some ETH may be locked in the contract

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

The msg.value passed in when the forgeWithListed function of the EntityForging contract is called can be greater than the forgingFee. However, when processing the incoming eth, the sum of devFee and forgerShare is equal to the forgingFee. If msg.value is greater than forgingFee, then the eth of msg.value-forgingFee will be left in the contract and cannot be withdrawn.

Proof of Concept

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

Modify some code in the official test case EntityForging.test.ts:

  describe('Forge With Listed', () => {
    it('should allow forging with a listed token', async () => {
      const forgerTokenId = FORGER_TOKEN_ID;
      const mergerTokenId = MERGER_TOKEN_ID;

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

      const forgerEntropy = await nft.getTokenEntropy(forgerTokenId);
      const mergerEntrypy = await nft.getTokenEntropy(mergerTokenId);
      /// The new token id will be forger token id + 1, cause it's the last item
      const expectedTokenId = FORGER_TOKEN_ID + 1;
      await expect(
        entityForging
          .connect(user1)
          .forgeWithListed(forgerTokenId, mergerTokenId, {
            value: ethers.parseEther('1.1'),
          })
      )
        .to.emit(entityForging, 'EntityForged')
        .withArgs(
          expectedTokenId,
          forgerTokenId,
          mergerTokenId,
          (forgerEntropy + mergerEntrypy) / 2n,
          FORGING_FEE
        )
        .to.emit(nft, 'NewEntityMinted')
        .withArgs(
          await user1.getAddress(),
          expectedTokenId,
          2,
          (forgerEntropy + mergerEntrypy) / 2n
        );

      const finalBalance = await ethers.provider.getBalance(owner.address);
      const contractBalance = await ethers.provider.getBalance(entityForging.getAddress());

      expect(finalBalance - initialBalance).to.be.eq((FORGING_FEE * 9n) / 10n);
      // Check forger nft delisted
      expect(contractBalance).to.equal(0, "Error: Contract still has balance remaining");

      const listingInfo = await entityForging.listings(forgerTokenId);

      expect(listingInfo.isListed).to.be.eq(false);
    });
  });

Test output:

    listForForging
      ✔ should not allow non-owners to list a token for forging
      ✔ should allow the owner to list a token for forging
    Forge With Listed
      1) should allow forging with a listed token
    Auto cancel listing after list for sale in EntityTrading
      ✔ Shoudl cancel list for forging after list for sale in Entity Trading


  3 passing (783ms)
  1 failing

  1) EntityForging
       Forge With Listed
         should allow forging with a listed token:

      AssertionError: Error: Contract still has balance remaining: expected 100000000000000000 to equal 0.
      + expected - actual

      -100000000000000000
      +0

      at Context.<anonymous> (test\EntityForging.test.ts:188:34)

Tools Used

vscode, hardhat

Recommended Mitigation Steps

It is recommended to modify the code at EntityForging.sol#L126 to:

require(msg.value == forgingFee, 'Insufficient fee for forging');

Assessed type

ETH-Transfer

[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)