Project

Summary

The claimProfit function in MembershipERC1155 contains a critical reentrancy vulnerability that could lead to profit double-spending, allowing malicious actors to drain the contract's funds by claiming the same profits multiple times. This vulnerability directly affects the profit distribution mechanism of the DAO membership system and could result in significant financial losses for other token holders. Given that the contract handles profit sharing and real currency transfers, a successful exploit would have severe economic implications for the protocol.

The vulnerability stems from the improper ordering of state updates and external calls in the claimProfit function. The function calculates the profit amount, then performs an external call through IERC20.safeTransfer() before updating the user's savedProfit state to zero. This violates the Checks-Effects-Interactions pattern. If the ERC20 token contract is malicious or includes callback functionality, it could reenter the claimProfit function before the savedProfit state is updated, allowing multiple withdrawals of the same profit amount.

Vulnerable code:

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

Scenario

An attacker could exploit this vulnerability through the following sequence:
The attacker first accumulates legitimate profits through token holdings. When calling claimProfit, their malicious token contract's transfer function includes a callback that reenters claimProfit. Since savedProfit[msg.sender] hasn't been set to zero yet, the second call to saveProfit returns the same profit amount. This creates a loop where the attacker can claim the same profits multiple times before the state is updated. The attacker continues this process until they've drained the available profits from the contract, effectively stealing funds that should have been distributed to other token holders.

Recommended Fix

To remediate this vulnerability, implement the following changes:

This fix applies the Checks-Effects-Interactions pattern and adds a reentrancy guard. The state is updated before performing any external calls, and the nonReentrant modifier prevents reentrant calls to the function. These changes ensure that the profit claiming mechanism remains secure even when interacting with malicious token contracts.