Griefing attack on seller's airdrop benefits

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/TraitForgeNft/TraitForgeNft.sol#L47 https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/TraitForgeNft/TraitForgeNft.sol#L148 https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/TraitForgeNft/TraitForgeNft.sol#L294 https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/TraitForgeNft/TraitForgeNft.sol#L329

Vulnerability details

Impact

TraitForge NFTs can be transferred and sold, but the new owner can burn or most likely nuke the token to reduce the the initial owner's airdrop beneefits.

Proof of Concept

When a user mints or forges a new token, they're set as the token's initialOwner and given airdrop benefits equal to the entropy of their newly minted token.

  function _mintInternal(address to, uint256 mintPrice) internal {
//...
    initialOwners[newItemId] = to;

    if (!airdropContract.airdropStarted()) {
      airdropContract.addUserAmount(to, entropyValue);
    }

  function _mintNewEntity(
    address newOwner,
    uint256 entropy,
    uint256 gen
  ) private returns (uint256) {
//...
    initialOwners[newTokenId] = newOwner;
//...
    if (!airdropContract.airdropStarted()) {
      airdropContract.addUserAmount(newOwner, entropy);
    }

    emit NewEntityMinted(newOwner, newTokenId, gen, entropy);
    return newTokenId;
  }

When burning, the entropy is then deducted from the initialOwner's amount, but not removed when transfered or when the token is bought, which in another case could allow the owner to earn from minting airdrop and from token sale.

  function burn(uint256 tokenId) external whenNotPaused nonReentrant {
//...
    if (!airdropContract.airdropStarted()) {
      uint256 entropy = getTokenEntropy(tokenId);
      airdropContract.subUserAmount(initialOwners[tokenId], entropy);
    }
//...
  }

However, a transferred or sold token can still be nuked or burned which directly reduces the initial owner's user amount causing loss of funds to the initial owner. Also, by nuking, the nuker doesn't lose as much in the griefing attack compared to burning the token.

  function nuke(uint256 tokenId) public whenNotPaused nonReentrant {
//...
    nftContract.burn(tokenId); // Burn the token
//...
  }

4. Runnable POC

The gist link below holds a test case that shows that airdrop amounts are not migrated to new owner during transfers/sales, and that by burning/nuking the token, the seller's airdrop amount can be successfully reduced.

https://gist.github.com/ZanyBonzy/cea1e654826391ca5fb797184ce6bd27

The expected result should look like this:

Tools Used

Manual code review

Recommended Mitigation Steps

Recommend introducing a check in the burn function that skips reducing user amount if the caller is the NukeFund contract or not the initial owner.

Assessed type

Other

[TForge1]

great find. we want to keep only minters/forgers as airdrop receivers, but yes if they transfer or sell and that next person nukes, the initialOwner will lose entropy from their airdrop amount, this will be a real issue.

[c4-judge]

koolexcrypto changed the severity to 3 (High Risk)

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto marked the issue as selected for report

[KupiaSecAdmin]

I think this issue is not valid.

The documentation regarding the airdrop states that:

In the event that a player 'nukes' an entity, the entropy allocated by that entity is removed.

This implies that the incentive should be removed when an entity is nuked. Even if the new entity holder performs the nuke, the entropy allocated to that entity should be removed. Since the incentive assigned to that entity does not transfer during ownership changes, the original owner remains responsible for its burning.

Therefore, I believe this should be considered either a design choice or potentially a user error.

[AtanasDimulski]

Hey @koolexcrypto thanks for the swift judging! The Airdrop contract is not within the scope of this contest. The trait forge contract doesn't inherit it, it just interacts with it in certain conditions - if the airdrop is started. Since all the logic that may cause some potential losses for the initial owner is in the Airdop.sol contract, this issue is clearly out of scope and thus invalid. Please have another look. Thank you for your time!

[AtanasDimulski]

There are no points without the Airdop contract, so what exactly should be fixed if the contract where the point distribution logic and their potential value is determined, is out of scope? All the interactions with that contract should be considered OOS. This is not a USDC or some other ERC20 contract, the value of these points comes from the Airdrop contract. Feel free to check the SC verdict on this type of situations.

[koolexcrypto]

Thank you everyone for your input.

Given that:

• The bug is in scope since mapping(uint256 => address) public initialOwners; is used to track minters and forgers. without tracking, no one will get airdrops even if the airdrop contract is ready and implemented.
• loss of funds to initial owners.

I believe the issue stands as is.