Lack of ability to make an some external function calls makes the DAO stage unreachable.

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/NukeFund/NukeFund.sol#L46-L57

Vulnerability details

Impact

TL;DR: Switching to DAO phase in NukeFund is not possible

This vulnerability is on the boundary of scope because it makes it impossible to use the out-of-scope contract but the problem is in the contact inside the scope that is the owner of this external contract.

The TraitForgeNft contract is written in such a way that it can add and remove users from the airdrop. For this the NFT contract must have ownership over the airdrop contract.

Because of this an additional function is created in TraitForgeNft to start the airdrop while other functions that can only be called by the owner (setTraitToken, allowDaoFund) are absent in TraitForgeNft contract. Which makes these functions impossible to call.

And if the first function may not be so necessary (deployer can call it at deploy), but the second function must have its own handle in the TraitForgeNft contract since this function is used by NukeFund and can only be called after the start of the airdrop, that is, after the transfer of ownership to the TraitForgeNft contract

Proof of Concept

• With normal initialization owner of Airdrop is TraitForgeNft

contract DeployAllTest is Test {
  string constant NAME = "TRAIT";
  string constant SYMBOL = "TRAIT";
  uint8 constant DECIMALS = 18;
  uint256 constant TOTAL_SUPPLY = 1_000_000e18;

  address constant UNISWAP_ROUTER = address(0x7a250d5630B4cF539739dF2C5dAcb4c659F2488D);

  address immutable DEPLOYER = makeAddr("DEPLOYER");
  address immutable USER1 = makeAddr("USER1");
  address immutable USER2 = makeAddr("USER2");
  address immutable USER3 = makeAddr("USER3");
  address immutable DEV1 = makeAddr("DEV1");
  address immutable DEV2 = makeAddr("DEV2");
  address immutable DEV3 = makeAddr("DEV3");

  Trait token;
  TraitForgeNft nft;
  EntropyGenerator entropyGenerator;
  EntityTrading entityTrading;
  EntityForging entityForging;
  DevFund devFund;
  Airdrop airdrop;
  DAOFund daoFund;
  NukeFund nukeFund;

  Attacker attacker;

  uint256 trackId = 0;

  function setUp() public {
      vm.startPrank(DEPLOYER);
      // Deploy all contracts using Foundry's deployment method
      token = new Trait(NAME, SYMBOL, DECIMALS, TOTAL_SUPPLY); // @audit-qa Would be nice to burn some tokens
      nft = new TraitForgeNft();
      entropyGenerator = new EntropyGenerator(address(nft));
      entityTrading = new EntityTrading(address(nft));
      entityForging = new EntityForging(address(nft));
      devFund = new DevFund();
      airdrop = new Airdrop();
      daoFund = new DAOFund(address(token), UNISWAP_ROUTER);
      nukeFund = new NukeFund(address(nft), address(airdrop), payable(address(devFund)), payable(address(daoFund)));

      attacker = new Attacker(nft, airdrop);
      vm.stopPrank();
  }

  function _initVars() internal {
      vm.startPrank(DEPLOYER);
      nft.setEntityForgingContract(address(entityForging));
      nft.setEntropyGenerator(address(entropyGenerator));
      nft.setAirdropContract(address(airdrop));
      airdrop.setTraitToken(address(token));
      airdrop.transferOwnership(address(nft));
      nft.setNukeFundContract(payable(address(nukeFund)));

      entropyGenerator.transferOwnership(address(nft));

      entityTrading.setNukeFundAddress(payable(address(nukeFund)));
      entityForging.setNukeFundAddress(payable(address(nukeFund)));

      entropyGenerator.writeEntropyBatch1();
      entropyGenerator.writeEntropyBatch2();
      entropyGenerator.writeEntropyBatch3();
      vm.stopPrank();
  }

  function testMedium_deadFunc() public {
      _initVars();

      console2.log(airdrop.owner());
      assert(airdrop.owner() == address(nft));
  }
}

• There is no way to call Airdrop.allowDaoFund()
• Third stage in NukeFund dev share distribution relly completely on airdropContract.daoFundAllowed()

        if (!airdropContract.airdropStarted()) {
            (bool success,) = devAddress.call{value: devShare}("");
            require(success, "ETH send failed");
            emit DevShareDistributed(devShare);
        } else if (!airdropContract.daoFundAllowed()) {
            (bool success,) = payable(owner()).call{value: devShare}("");
            require(success, "ETH send failed");
        } else {
            (bool success,) = daoAddress.call{value: devShare}("");
            require(success, "ETH send failed");
            emit DevShareDistributed(devShare);
        }

Tools Used

Manual Review

Recommended Mitigation Steps

Add delegate handle to TraitForgeNft:

diff --git a/contracts/TraitForgeNft/TraitForgeNft.sol b/contracts/TraitForgeNft/TraitForgeNft.sol
index b933d74..14b2f99 100644
--- a/contracts/TraitForgeNft/TraitForgeNft.sol
+++ b/contracts/TraitForgeNft/TraitForgeNft.sol
@@ -86,6 +86,11 @@ contract TraitForgeNft is ITraitForgeNft, ERC721Enumerable, ReentrancyGuard, Own
         airdropContract.startAirdrop(amount); //@external-logic
     }

+    function allowDaoFund() external whenNotPaused onlyOwner {
+        airdropContract.allowDaoFund();
+    }
+
     function setStartPrice(uint256 _startPrice) external onlyOwner {
         startPrice = _startPrice;
     }

Assessed type

Access Control

[c4-judge]

koolexcrypto marked the issue as unsatisfactory: Invalid

[PFAhard]

I think this issue was marked as invalid because validators mistakenly flagged it as a duplicate of https://github.com/code-423n4/2024-07-traitforge-findings/issues/214.

But this is a completely different issue, 214 addresses the normal behavior by reporting that "the TraitForgeNft cannot call subUserAmount and addUserAmount because they are ownable", but this is incorrect since the ownership is transferred to TraitForgeNft.

But since the ownership is transferred to TraitForgeNft, allowDaoFund cannot be called (not implemented). Which results in the failure to switch to the DAO stage of the project.

And that's what this issue is about

[c4-judge]

koolexcrypto marked the issue as not a duplicate

[c4-judge]

koolexcrypto removed the grade

[samuraii77]

@koolexcrypto, hello, I saw you marked this issue as not a duplicate, I just want to mention that the Airdrop contract is OOS and any issues related to improper access control set on it should not be valid at all.

[PFAhard]

@samuraii77 The issue described here is not related to improper access to the Airdrop, but to the inability to enter the DAO phase in NukeFund.

[samuraii77]

Which is a function on the Airdrop contract and can't be called due to improper access control as if it had a different access control, let's say onlyAuthorizedCaller, it would be callable by someone else. It is an assumption you are making that the NukeFund contract should be the one being able to call that function but that is not necessarily the case, the root cause of the issue is in the Airdrop contract which is OOS

[PFAhard]

@samuraii77 The main reason is TraitForgeNft, because it is the owner of Aidrop and do not have handle. It is the same as saying that unable to call pause, unpause is out of scope, because Ownable is out of scope (and a completely different project from OZ to be precise)

[samuraii77]

It was never mentioned that this function should be callable. The only reason you know of its existence is because you checked an OOS contract. Your argument about Pausable is incorrect as the contract has been inherited, thus they have shown intent they want the contract to be pausable. However, Airdrop contract is not inherited, it is a completely OOS and external contract, you don't actually have the knowledge whether someone should be callable as it's OOS and the access control is in the OOS contract which means the error is there.

Sorry for messaging in this issue to the judge if my input was unwanted, I just wanted to leave my opinion and will abstain from commenting further, will leave it up to the judge to decide.

[LyuboslavVeliev]

Checking OOS contracts doesn't mean that the issue is OOS. The rule is if the root cause of the issue is in a contract that is in-scope, then the issue is valid. Here the issue is in a contract that is in-scope and thus i belive this is a valid issue.

[c4-judge]

koolexcrypto marked the issue as primary issue

[koolexcrypto]

Thank you everyone for your input.

I believe the issue is valid. Owner of Airdrop is TraitForgeNft and the bug prevents from entering the DAO stage. The root cause and the fix is in TraitForgeNft, therefore it is in scope.

[c4-judge]

koolexcrypto marked the issue as selected for report