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

Reentrancy Vulnerability in claimProfit Allows Malicious Contracts to Drain Excess Profit

cheatc0d33

Summary

The claimProfit function in MembershipERC1155 contract is vulnerable to reentrancy attacks through ERC1155 token transfers that could occur during profit claiming. This vulnerability allows an attacker to drain more profit share than they are entitled to by manipulating the contract's state during the claim process. The severity is critical as it directly impacts the contract's fund distribution mechanism and could result in loss of funds for other legitimate token holders.

The issue stems from the contract's handling of profit claims and token transfers. When a user claims their profit share, the contract updates the profit state before executing the external token transfer. During this transfer, if tokens are moved, the ERC1155 standard mandates a callback to the recipient contract through onERC1155Received. A malicious contract could exploit this callback to reenter the claimProfit function before the initial claim is completed, finding the contract in an inconsistent state where profit calculations could be manipulated.

Vulnerable Code Snippet

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

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);
}
​
​
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);
}

Scenario

Consider the following attack scenario:

  1. Attacker deploys a malicious contract that holds membership tokens and implements onERC1155Received

  2. The attacker's contract initiates a claimProfit transaction

  3. During the IERC20 transfer, the attacker's contract transfers some tokens, triggering _update

  4. The _update function calls saveProfit, recalculating profits with the new token distribution

  5. Through the onERC1155Received callback, the attacker reenters claimProfit

  6. Due to the state inconsistency, the attacker can claim additional profits before the initial claim completes

  7. This cycle can potentially continue until the contract's funds are drained

Recommended Fix

contract MembershipERC1155 is ERC1155Upgradeable, AccessControlUpgradeable, IMembershipERC1155, ReentrancyGuardUpgradeable {
function claimProfit() external nonReentrant returns (uint256 profit) {
// Cache current values
uint256 currentProfit = saveProfit(msg.sender);
require(currentProfit > 0, "No profit available");
// Update state
savedProfit[msg.sender] = 0;
lastProfit[msg.sender] = totalProfit;
// Perform external call last
IERC20(currency).safeTransfer(msg.sender, currentProfit);
emit Claim(msg.sender, currentProfit);
return currentProfit;
}
}

Consider implementing a pull-payment pattern where users first lock their claims and then withdraw in a separate transaction to further enhance security.

Updates

Lead Judging Commences

0xbrivan2 Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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