c4-bot-2 commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/main/contracts/EntityForging/EntityForging.sol#L125-L126 https://github.com/code-423n4/2024-07-traitforge/blob/main/contracts/EntityForging/EntityForging.sol#L146-L148 https://github.com/code-423n4/2024-07-traitforge/blob/main/contracts/EntityForging/EntityForging.sol#L156-L159

Vulnerability details

Impact

Given the scenario below:

Forger fee for tokenId 99: 0.01 ether
Taxcut: 10 (10%)
Merging user merges tokenId 99 with tokenId 100 and pays/sends 0.1 ether

Outcome becomes:

uint256 forgingFee = _forgerListingInfo.fee; = 0.01 ether / 1e16
uint256 devFee = forgingFee / taxCut; = 0.01 / 10 = 1e15 / 0.001 ether
uint256 forgerShare = forgingFee - devFee; = 1e16 - 1e15 = 9e15 / 0.009 ether
(bool success, ) = nukeFundAddress.call{ value: devFee }(''); nukeFund contract receives 0.001 ether
(bool success_forge, ) = forgerOwner.call{ value: forgerShare }(''); Forger token owner receives 0.009 ether
0.09 ether becomes inaccessible in the EntityForging contract

Thus the merger ends up losing the excess ethers sent. Given that a similar scenario is handled below in the TraitForge contract, this issue requires mitigation as well during token forging.

TraitForge.sol

function mintToken(
    bytes32[] calldata proof
  )
    public
    payable
    whenNotPaused
    nonReentrant
    onlyWhitelisted(proof, keccak256(abi.encodePacked(msg.sender)))
  {
    uint256 mintPrice = calculateMintPrice();
    require(msg.value >= mintPrice, 'Insufficient ETH send for minting.');

    ...

    uint256 excessPayment = msg.value - mintPrice;
    if (excessPayment > 0) { // @note refunds excess during token mints
      (bool refundSuccess, ) = msg.sender.call{ value: excessPayment }('');
      require(refundSuccess, 'Refund of excess payment failed.');
    }
  }

Proof of Concept

EntityForging.sol

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

@>   uint256 forgingFee = _forgerListingInfo.fee;
@>   require(msg.value >= forgingFee, 'Insufficient fee for forging');
    ...

@>  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');
    ...

    return newTokenId;
  }

Tools Used

Manual review

Recommended Mitigation Steps

- require(msg.value >= forgingFee, 'Insufficient fee for forging');
+ require(msg.value == forgingFee, 'Insufficient fee for forging');

Assessed type

Payable