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

Race Condition in Profit Distribution Leads to Excess Claims

0xbrett8571

Summary

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

// @FOUND - Race condition in profit claiming
function claimProfit() external returns (uint256 profit) {
profit = saveProfit(msg.sender);
require(profit > 0, "No profit available");
savedProfit[msg.sender] = 0;
IERC20(currency).safeTransfer(msg.sender, profit);
emit Claim(msg.sender, profit);
}

The issue is that saveProfit() reads and updates global state (totalProfit and lastProfit) without proper synchronization. When multiple users claim simultaneously:

  1. User1 reads totalProfit = X

  2. User2 reads totalProfit = X

  3. User1 updates lastProfit[user1] = X

  4. User2 updates lastProfit[user2] = X

This can lead to double-counting of profits.

Vulnerability Details

The profit distribution system uses shared state variables (totalProfit and lastProfit) without proper synchronization mechanisms. When multiple users claim simultaneously, they can read the same totalProfit value before their respective lastProfit values are updated, leading to profit calculation inconsistencies.

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

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

// @FOUND - Profit calculation race condition
function claimProfit() external returns (uint256 profit) {
profit = saveProfit(msg.sender);
require(profit > 0, "No profit available");
savedProfit[msg.sender] = 0;
IERC20(currency).safeTransfer(msg.sender, profit);
emit Claim(msg.sender, profit);
}
​
// @FOUND - Shared state without synchronization
function saveProfit(address account) internal returns (uint256 profit) {
uint256 unsaved = getUnsaved(account);
lastProfit[account] = totalProfit; // Race condition point
profit = savedProfit[account] + unsaved;
savedProfit[account] = profit;
}
​
// @FOUND - Profit calculation uses potentially stale state
function getUnsaved(address account) internal view returns (uint256 profit) {
return ((totalProfit - lastProfit[account]) * shareOf(account)) / ACCURACY;
}

The vulnerability originates from the fundamental design of the profit distribution system in MembershipERC1155.sol. The core issue stems from three key architectural decisions:

uint256 public totalProfit;
mapping(address => uint256) internal lastProfit;

These global state variables are accessed and modified without atomic operations.

function saveProfit(address account) internal returns (uint256 profit) {
uint256 unsaved = getUnsaved(account);
lastProfit[account] = totalProfit; // Critical point
profit = savedProfit[account] + unsaved;
savedProfit[account] = profit;
}

The calculation relies on reading global state (totalProfit) and updating user-specific state (lastProfit) in separate operations.

function claimProfit() external returns (uint256 profit) {
profit = saveProfit(msg.sender);
savedProfit[msg.sender] = 0;
IERC20(currency).safeTransfer(msg.sender, profit);
}

The claim process spans multiple state changes without transaction-level atomicity.

This design creates a race condition where multiple users claiming simultaneously can read the same totalProfit value before individual lastProfit values are updated, leading to profit calculation inconsistencies. The vulnerability is inherent in the contract's architecture rather than a simple coding error.

Proof of Concept

Consider this attack Scenario Initial State

  • Total profits: 1000 tokens

  • Alice and Bob each hold 50% of shares

  • Expected claim: 500 tokens each

Attack Flow

// 1. Alice initiates claim
function claimProfit() {
uint256 profit = saveProfit(alice); // Reads totalProfit = 1000
// Calculates share = 500
}
​
// 2. Bob's claim executes before Alice's state update
function claimProfit() {
uint256 profit = saveProfit(bob); // Also reads totalProfit = 1000
// Also calculates share = 500
}

Result:

  • Alice receives 500 tokens

  • Bob receives 500 tokens

  • Total distributed: 1000 tokens (200% of intended amount)

Impact

Multiple users claiming simultaneously could receive more profits than actually available

Recommendations

// Add snapshot mechanism to ensure consistent profit calculations across simultaneous claims

function claimProfit() external returns (uint256 profit) {
+ uint256 currentTotalProfit = totalProfit;
- profit = saveProfit(msg.sender);
+ profit = saveProfit(msg.sender, currentTotalProfit);
require(profit > 0, "No profit available");
savedProfit[msg.sender] = 0;
IERC20(currency).safeTransfer(msg.sender, profit);
emit Claim(msg.sender, profit);
}
Updates

Lead Judging Commences

0xbrivan2 Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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

Give us feedback!