howlbot-integration[bot] commented 2 months ago

Lines of code

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

Vulnerability details

Impact

The EntityForging.sol contract allows merger users to pay forging fees using ETH and forge with other forger NFTs, to obtain a new NFT. The fee sent is split between the NukeFund and the forger NFT owner. However, there is no strict validation on how much a merger can pay, and if any excess ETH is provided (user sends extra to cover gas fees) it will be left stuck in the EntityForging.sol contract as there is no way to transfer it after.

Proof of Concept

The TraitForge protocol uses native token transfers to either buy or forge NFTs. Throughout the code, it can be seen that the user needs to either provide the exact amount or if he sends more, the excess will be returned. None of these approaches, however, were applied to the EntityForging::forgeWithListed(...) function, where any excess amounts are left stranded in the contract.

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

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 Airdrop and EntropyGenerator contracts to have the NFT contract as an owner, to remove any owner issues as shown 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); } } ```

Amount locked in EntityForging contract

```solidity function testExceesAmountIsStuckInForgerContract() 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); // We need to get a forger with forge potential in order to list assertTrue(nftContract.isForger(5)); assertGt(uint8((nftContract.getTokenEntropy(5) / 10) % 10), 0); vm.prank(naruto); entityForging.listForForging(5, 0.01 ether); vm.prank(sasuke); entityForging.forgeWithListed{value: 0.011 ether}(5, 4); // Excess fee is stuck in forger contract assertEq(address(entityForging).balance, 0.001 ether); } ``` ```bash Ran 1 test for test/unit/AuditTests.t.sol:AuditTests [PASS] testExceesAmountIsStuckInForgerContract() (gas: 1945373) Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 10.51ms (1.19ms CPU time) Ran 1 test suite in 156.95ms (10.51ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests) ```

Tools Used

Manual review + Foundry

Recommended Mitigation Steps

Add one of the two approaches to the EntityForging::forgeWithListed(...) function:

Require the exact forging fee to be paid -> https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/EntityTrading/EntityTrading.sol#L65

@@ -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, 'Invalid fee amount provided.');

     _resetForgingCountIfNeeded(forgerTokenId); // Reset for forger if needed
     _resetForgingCountIfNeeded(mergerTokenId); // Reset for merger if needed

Return any excess amounts to the sender -> https://github.com/code-423n4/2024-07-traitforge/blob/main/contracts/TraitForgeNft/TraitForgeNft.sol#L196-L200

@@ -158,6 +158,13 @@ contract EntityForging is IEntityForging, ReentrancyGuard, Ownable, Pausable {
     (bool success_forge, ) = forgerOwner.call{ value: forgerShare }('');
     require(success_forge, 'Failed to send to Forge Owner');

+    uint256 excessPayment = msg.value - devFee + forgerShare;
+
+    if (excessPayment > 0) {
+      (bool refundSuccess, ) = msg.sender.call{ value: excessPayment }('');
+      require(refundSuccess, 'Refund of excess payment failed.');
+    }
+

Assessed type

ETH-Transfer