howlbot-integration[bot] commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/main/contracts/DevFund/DevFund.sol#L69 https://github.com/code-423n4/2024-07-traitforge/blob/main/contracts/DevFund/DevFund.sol#L74

Vulnerability details

Summary

A developer may lose rewards if the receive function is triggered during the reward claim process from DevFund, resulting in their info.rewardDebt being higher than expected. This scenario can occur when a developer claims their rewards and then immediately mints an NFT with the funds, but the airdrop hasn’t yet started, they stand to lose those rewards. This happens because the system updates the user's rewardDebt after the callback.

Vulnerability Details

When claiming the dev's rewards are calculated as follows: uint256 pending = info.pendingRewards + (totalRewardDebt - info.rewardDebt) * info.weight;

Then we do a call to that dev to transfer his funds and only after that call do we update his reward debt to the total reward debt.

This can create a flow in which the developer loses a portion of his rewards. For example if the dev has a contract that receives his rewards and wants to use that money to then buy an NFT, as he believes in the protocol and wants to support it, he would have a receive function like this:

 receive() external payable {
      traitForgeNFtContract.mintWithBudget{ value: address(this).balance }("");
 }

However in this scenario the DevFund::receive function will be triggered (if the airdrop hasn't started) which increases totalRewardDebt and only after that the dev's info is updated to the totalRewardDebt, which means that the user will later receive less rewards because info.rewardDebt will include the new rewards from the received minting fees but the dev didn't claim them.

Proof of Concept

This is DevFund::claim():

  function claim() external whenNotPaused nonReentrant {
    DevInfo storage info = devInfo[msg.sender];

    uint256 pending = info.pendingRewards +
      (totalRewardDebt - info.rewardDebt) *
      info.weight;

    if (pending > 0) {
@->      uint256 claimedAmount = safeRewardTransfer(msg.sender, pending);
      info.pendingRewards = pending - claimedAmount;
      emit Claim(msg.sender, claimedAmount);
    }

@->    info.rewardDebt = totalRewardDebt;
  }

We can see that the info.rewardDebt happens after the call to the dev so now if the dev immediately calls the TraitForgeNft contract like that:

 receive() external payable {
      traitForgeNFtContract.mintWithBudget{ value: address(this).balance }("");
 }

This will mint the developer some NFTs and for each minted NFT the DevFund::receive() will be triggered. The minting in TraitForgeNft distributes the fees to the NukeFund contract and if the airdrop hasn't started yet, the rewards are then redirected to the DevFund contract which triggers the receive function and the totalRewardDebt is updated:

  receive() external payable {
    if (totalDevWeight > 0) {
      uint256 amountPerWeight = msg.value / totalDevWeight;
      uint256 remaining = msg.value - (amountPerWeight * totalDevWeight);
@->      totalRewardDebt += amountPerWeight;
      if (remaining > 0) {
        (bool success, ) = payable(owner()).call{ value: remaining }('');
        require(success, 'Failed to send Ether to owner');
      }
    } else {
      (bool success, ) = payable(owner()).call{ value: msg.value }('');
      require(success, 'Failed to send Ether to owner');
    }
    emit FundReceived(msg.sender, msg.value);
  }

Only after these steps we update the reward debt of the dev, which will be much higher so the developer loses the rewards from this minting fees.

!NOTE This scenario is more than likely to happen because the developers will want to support the protocol and participate in it.

Imapct

A developer loses a portion of his rewards.

Tools Used

Manual Review

Recommended Mitigation Steps

Follow the CEI pattern and transfer the dev's funds only after updating all the state variables in DevFund::claim().

Assessed type

Other

koolexcrypto commented 2 months ago

For more info and context, please check comments at this

https://github.com/code-423n4/2024-07-traitforge-validation/issues/1014

TForge1 commented 2 months ago

If I understand it correctly then it’s not an issue. I don’t see why myself or anybody else will do this. Especially using a sequencer based tx system like base. This is not an issue to me, not sure abt everybody else

koolexcrypto commented 2 months ago

@TForge1 Thank you for your feedback. In this scenario, the sequencer is irrelevant. This is on EVM level and applicable on all chains.

To summarize this for better assessment:

Scenario:
1. A developer claims rewards from DevFund.
2. The developer's contract immediately uses these funds to mint NFTs.
3. If the airdrop hasn't started, this minting triggers the DevFund's receive() function.
4. The totalRewardDebt is increased before the developer's info.rewardDebt is updated.
5. This results in the developer's info.rewardDebt being set higher than it should be, causing them to lose out on some rewards.
Impact: Developers may lose rewards.
Root cause: The issue occurs because the contract updates the user's rewardDebt after transferring funds to the developer.

Note from the warden: This scenario is considered likely because developers are expected to want to support and participate in the protocol actively.

For clearer picture of the technical flow, here is a diagram:

sequenceDiagram
    participant DeveloperContract
    participant DevFund
    participant NFTContract

    DeveloperContract->>DevFund: claim() rewards
    activate DevFund
    DevFund->>DeveloperContract: transfer rewards
    activate DeveloperContract
    Note over DeveloperContract: receive() function triggered
    DeveloperContract->>NFTContract: mintWithBudget()
    NFTContract->>DevFund: receive() (minting fees)
    DevFund->>DevFund: Update totalRewardDebt
    deactivate DeveloperContract
    DevFund->>DevFund: Update developer's info.rewardDebt
    deactivate DevFund
    Note over DevFund: Developer's info.rewardDebt now higher than expected

@LyuboslavVeliev Please confirm or correct me if I'm wrong

c4-judge commented 2 months ago

koolexcrypto marked the issue as primary issue

TForge1 commented 2 months ago

ok fair enough. I guess it's an issue then. not sure if ill do this but if I do then it can be an issue.

LyuboslavVeliev commented 2 months ago

@koolexcrypto Thank you for summarization. I just want to point out that #1061 describes a different scenario that is unlikely to happen. If you just look at his issue I don't think you would have validated it because it says that when the dev claims his rewards, then he distributes it back to the DevFund. Why would a dev do that? "As a result the dev loses rewards" - yeah he loses the rewards that he distributed back to the DevFund which means he doesn't want to receive rewards so there is no issue overall. Please correct me if I am wrong but I don't think it should be duped with this one since the likelihood of his issue is almost zero. Even though he found the root cause of the issue he doesn't demonstrate how it can be bad for the protocol.

samuraii77 commented 2 months ago

@koolexcrypto, so we are assuming that the protocol would have a receive() function to mint NFTs automatically? Why would that be a likely thing to happen? It was never mentioned that they would be minting NFTs and reenter into their own contract, that can't possibly be more than a QA. It classifies as a few different things - out of scope and user error, not only is it user error but it is actually owner error and owners are supposed to be much more knowledgeable than users.

LyuboslavVeliev commented 2 months ago

As it can be seen by the sponsor's reply they were not aware of this bug so it would be an owner error only if they knew the bug existed.
I don't see why the devs wouldn't do such thing to support their protocol by using the claimed rewards to mint NFTs. You can see that the sponsor said "not sure if ill do this but if I do then it can be an issue". This means the devs can use this method.
How is it out of scope when the root cause is in the DevFund - a contract that is in-scope.

koolexcrypto commented 2 months ago

ok fair enough. I guess it's an issue then. not sure if ill do this but if I do then it can be an issue.

There is a middle solution, implement a sweep function under restrictive condition, for instance, when there is no NFT to be nuked any longer. This way, rug-pull is not possible. You could also add to it a timeframe that has to pass first as a delay before sweeping even if all NFTs are nuked.

c4-judge commented 2 months ago

koolexcrypto marked the issue as satisfactory

koolexcrypto commented 2 months ago

No further changes on the dups. Looked at it already.

c4-judge commented 2 months ago

koolexcrypto marked the issue as selected for report

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

A dev will lose rewards if after claiming his rewards he mints an NFT #1060

Lines of code

Vulnerability details

Summary

Vulnerability Details

Proof of Concept

Imapct

Tools Used

Recommended Mitigation Steps

Assessed type