Project

Summary

Looking at the implementation in MembershipERC1155.sol, there's a potential issue in the profit claiming logic: https://github.com/Cyfrin/2024-11-one-world/blob/1e872c7ab393c380010a507398d4b4caca1ae32b/contracts/dao/tokens/MembershipERC1155.sol#L144-L150

The bug occurs because saveProfit() is called before checking if profit > 0. This means:

1. saveProfit() updates lastProfit[account] to current totalProfit

2. If profit = 0, the function reverts

3. But lastProfit was already updated, potentially causing loss of unclaimed profits

The state change happens before the validation check, which violates the expectation that profit claims should be atomic - either fully succeed or make no state changes.

Vulnerability Details

The claimProfit() function updates critical profit tracking state before validating the claim amount. This creates a state inconsistency when claims revert, as lastProfit[msg.sender] is already modified to the current totalProfit value.

Technical Description: In the profit claiming mechanism where lastProfit[msg.sender] is updated to current totalProfit before validating if there are claimable profits. If the claim reverts, the user loses track of previously accumulated profits.

The vulnerability originates in the MembershipERC1155 contract's profit tracking mechanism. Looking at the code, the issue stems from how the contract handles profit calculations in relation to token burning.

• The profit tracking relies on two key variables: https://github.com/Cyfrin/2024-11-one-world/blob/1e872c7ab393c380010a507398d4b4caca1ae32b/contracts/dao/tokens/MembershipERC1155.sol#L26-L28

• The profit calculation depends on the shareOf() function: https://github.com/Cyfrin/2024-11-one-world/blob/1e872c7ab393c380010a507398d4b4caca1ae32b/contracts/dao/tokens/MembershipERC1155.sol#L169-L177

When tokens are burned, the balanceOf values change, but the contract doesn't properly update the profit tracking state. This creates a disconnect between the user's actual profit entitlement and the contract's ability to track it, leading to the permanent loss of profit tracking capabilities.

The root cause is that the burn operation modifies token balances without corresponding adjustments to the profit tracking system, breaking the relationship between token ownership and profit claims.

Proof of concept

The following test demonstrates the profit claiming vulnerability.

Logs:

This shows how a user's profit tracking is completely lost after burning their tokens:

1. Initially, the user has accumulated profits (1000000000000000000000)

2. After burning tokens and claiming profits, their profit balance drops to 0

3. Even when new profits are added to the system, their balance remains at 0

This proves that burning tokens leads to permanent loss of profit tracking, which is a significant vulnerability in the profit distribution system.

Impact

1. User accumulated profits through token holdings

2. User attempts to claim with claimProfit()

3. saveProfit() updates lastProfit to current totalProfit

4. If profit = 0 or transfer fails, function reverts

5. User's lastProfit is now incorrectly set to current totalProfit

6. All previously accumulated unclaimed profits are effectively lost

7. Future profit calculations will start from the failed claim attempt point

This vulnerability allows profit tracking to become desynchronized from actual claimable amounts, leading to permanent loss of user profits. The issue is particularly severe for long-term token holders who accumulate significant profits over time.

The vulnerability breaks the core profit distribution mechanism that incentivizes users to hold membership tokens. Any failed claim permanently reduces a user's claimable profit amount.

Recommendations