Project
One World
NFTDeFi
15,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Profit Distribution Manipulation Through Transfer Race Condition

0xbrett8571

Summary

The vulnerability originates from a fundamental design flaw in the profit tracking mechanism of MembershipERC1155.sol. The core issue stems from the _update function's implementation where profit calculations occur before token balance updates.

Key points of origin:

function _update(
address from,
address to,
uint256[] memory ids,
uint256[] memory amounts
) internal virtual override(ERC1155Upgradeable) {
if (from != address(0)) saveProfit(from); // Calculates with stale balances
if (to != address(0)) saveProfit(to); // Calculates with stale balances
super._update(from, to, ids, amounts); // Updates balances after calculations
}

  • The profit calculation depends on shareOf() which uses current token balances:

function shareOf(address account) public view returns (uint256) {
return (balanceOf(account, 0) * 64) +
(balanceOf(account, 1) * 32) +
// ... weighted balance calculations
}

This creates a race condition where profit calculations use outdated balance states, leading to incorrect profit distributions. The design assumes profits can be safely calculated before balance updates, but this assumption breaks the accounting system's integrity during transfers.

Vulnerability Details

The profit distribution system in MembershipERC1155 contains a critical flaw in how profits are calculated and saved during token transfers. The _update function processes profit calculations before token balances are updated, allowing manipulation of profit distribution.

Technical Description The core issue lies in the sequence of operations during token transfers. Profits are calculated and saved using stale balances before the actual transfer occurs, leading to incorrect profit distribution and potential exploitation.

https://github.com/Cyfrin/2024-11-one-world/blob/1e872c7ab393c380010a507398d4b4caca1ae32b/contracts/dao/tokens/MembershipERC1155.sol#L203-L212

// @FOUND - Profit calculation uses outdated balances
function _update(
address from,
address to,
uint256[] memory ids,
uint256[] memory amounts
) internal virtual override(ERC1155Upgradeable) {
if (from != address(0)) saveProfit(from); // Uses old balance
if (to != address(0)) saveProfit(to); // Uses old balance
super._update(from, to, ids, amounts); // Updates balances after profit calculation
}

saveProfit() is called before the actual transfer occurs, which can lead to incorrect profit calculations since the token balances haven't been updated yet. https://github.com/Cyfrin/2024-11-one-world/blob/1e872c7ab393c380010a507398d4b4caca1ae32b/contracts/dao/tokens/MembershipERC1155.sol#L182-L187

// @FOUND - Profit calculation uses stale balances in saveProfit()
function saveProfit(address account) internal returns (uint256 profit) {
uint256 unsaved = getUnsaved(account);
lastProfit[account] = totalProfit;
profit = savedProfit[account] + unsaved;
savedProfit[account] = profit;
}

The profit tracking system calculates and updates profits before token balances change during transfers. This creates a race condition where profit calculations use outdated balances, leading to incorrect profit distribution.

Proof of Concept

Attack Scenario:

  1. Alice has 100 tokens with accumulated profits

  2. Bob initiates a transfer from Alice

  3. System calculates Alice's profits using her old balance (100 tokens)

  4. Transfer executes, updating balances

  5. Alice's profit calculation is incorrect as it used pre-transfer balance

// Initial state
Alice: 100 tokens, 10 ETH accumulated profits
totalProfit = 1000 ETH
​
// Transfer occurs
Bob transfers 50 tokens from Alice
saveProfit(Alice) // Calculates with 100 tokens instead of 50
_update executes transfer
// Result: Alice's profit calculation is inflated

Impact

Loss of user funds through incorrect calculations and breaking of the profit-sharing mechanism integrity

Recommendations

Two optional recommendations:

ensure profit calculations use the correct token balances by separating the profit saving from the transfer operation.

function _update(
address from,
address to,
uint256[] memory ids,
uint256[] memory amounts
) internal virtual override(ERC1155Upgradeable) {
// Save current profits first
uint256 fromProfit = from != address(0) ? getUnsaved(from) : 0;
uint256 toProfit = to != address(0) ? getUnsaved(to) : 0;
// Perform transfer
super._update(from, to, ids, amounts);
// Update profits after transfer with new balances
if(from != address(0)) {
savedProfit[from] += fromProfit;
lastProfit[from] = totalProfit;
}
if(to != address(0)) {
savedProfit[to] += toProfit;
lastProfit[to] = totalProfit;
}
}

Here is the other option.

function _update(
address from,
address to,
uint256[] memory ids,
uint256[] memory amounts
) internal virtual override(ERC1155Upgradeable) {
- if (from != address(0)) saveProfit(from);
- if (to != address(0)) saveProfit(to);
super._update(from, to, ids, amounts);
+ // Calculate profits after balance update
+ if (from != address(0)) {
+ uint256 fromProfit = getUnsaved(from);
+ savedProfit[from] += fromProfit;
+ lastProfit[from] = totalProfit;
+ }
+ if (to != address(0)) {
+ uint256 toProfit = getUnsaved(to);
+ savedProfit[to] += toProfit;
+ lastProfit[to] = totalProfit;
+ }
}
Updates

Lead Judging Commences

0xbrivan2 Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice
0xbrivan2 Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!