Wrong calculation of `taxCut` can lead to incorrect distribution of funds

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/EntityForging/EntityForging.sol#L146 https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/EntityTrading/EntityTrading.sol#L72 https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/NukeFund/NukeFund.sol#L41

Vulnerability details

Impact

taxCut is a non-constant variable, being set to 10% on deploy. It has a setTaxCut() function which allows the taxCut value to be changed at any time by the contract owner. The taxCut amount is used on three places through the protocol: 1) In NukeFund::receive() upon receiving ETH the taxCut is calculated and send to the Dev/DAO Fund. 2) In EntityTrading::buyNFT() upon buying a NFT the nukeFundContribution variable represents the taxCut end value that is being transfered to the nukeFund address. 3) In EntityForging::forgeWithListed() upon forging a NFT the devFee variable represents the taxCut end value that is being transfered to the nukeFund address.

The taxCut calculation may work with the current value set on deploying of the contracts which is 10% but if changed in the future by the contract owner it will return wrong values making the protocol/seller receiving less or more fees depending of the taxCut value being set.

NukeFund.sol::receive()

  receive() external payable {
  @>>>  uint256 devShare = msg.value / taxCut; // Calculate developer's share (10%)
  @>>>  uint256 remainingFund = msg.value - devShare; // Calculate remaining funds to add to the fund

    fund += remainingFund; // Update the fund balance

    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);
    }

    emit FundReceived(msg.sender, msg.value); // Log the received funds
    emit FundBalanceUpdated(fund); // Update the fund balance
  }

EntityTrading.sol::buyNFT()

  function buyNFT(uint256 tokenId) external payable whenNotPaused nonReentrant {
    ......
    ......
    ......

    //transfer eth to seller (distribute to nukefund)
@>>>    uint256 nukeFundContribution = msg.value / taxCut;
@>>>    uint256 sellerProceeds = msg.value - nukeFundContribution;
    transferToNukeFund(nukeFundContribution); // transfer contribution to nukeFund

    // transfer NFT from contract to buyer
    (bool success, ) = payable(listing.seller).call{ value: sellerProceeds }(
      ''
    );

   .....
   .....
  }

EntityForging.sol::forgeWithListed()

  function forgeWithListed(
    uint256 forgerTokenId,
    uint256 mergerTokenId
  ) external payable whenNotPaused nonReentrant returns (uint256) {

    .....
    .....
    .....

@>>>    uint256 devFee = forgingFee / taxCut;
@>>>    uint256 forgerShare = forgingFee - devFee;
    address payable forgerOwner = payable(nftContract.ownerOf(forgerTokenId));

    .....
    .....

    (bool success, ) = nukeFundAddress.call{ value: devFee }('');
    require(success, 'Failed to send to NukeFund');
    (bool success_forge, ) = forgerOwner.call{ value: forgerShare }('');
    require(success_forge, 'Failed to send to Forge Owner');

    .....
    .....
  }

Proof of Concept

Lets see the following example:

1) Alice wants to buy 1 NFT which price is 1 ETH. 2) The current tax 10% is calculated like this: a) uint256 nukeFundContribution = msg.value / taxCut; --> 1 ETH / 10 =0.1ETH Everything looks good, but lets say that the tax is being changed to e.g 20%

b) uint256 nukeFundContribution = msg.value / taxCut --> 1 ETH / 20 =0.05ETH But if we use the formula for calculating percentages ,(x*p)/100 where p = taxCut(20%) the calculation will look like this: (1 ETH * 20) / 100 =0.2ETH

We have a difference of 0.15 ETH in favor of the nukeFund address and the seller of the NFT will receive less ETH than intended

Tools Used

Manual Review

Recommended Mitigation Steps

Change the calculation of the taxCut with this formula: (msg.value * taxCut)/100

Assessed type

Math

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto changed the severity to 2 (Med Risk)

[c4-judge]

koolexcrypto changed the severity to 3 (High Risk)