Summary

The claimProfit function in the MembershipERC1155 contract is vulnerable to a reentrancy attack, which could allow malicious users to drain the contract's funds by recursively calling claimProfit before their profit state is updated.

Finding Description

The issue arises because the claimProfit function transfers tokens to the user before updating the savedProfit[msg.sender] variable. This creates a window where a malicious user could exploit the transfer process by re-entering the claimProfit function, calling it recursively, and draining additional tokens from the contract.

Security Guarantees Broken:

• Reentrancy protection: The function does not guard against reentrancy, which is a common vulnerability in Solidity smart contracts that can allow an attacker to perform unintended actions.

• State consistency: By updating the state (i.e., resetting savedProfit[msg.sender]) after transferring tokens, the function violates the typical order of operations, which may allow unexpected behavior.

How it breaks:

A malicious actor could call claimProfit, triggering the token transfer to their address. Before the state update is completed (before the savedProfit[msg.sender] is set to 0), they could recursively call claimProfit, claiming additional tokens without the state reflecting the correct balance. This could result in the contract transferring more funds than intended.

Malicious input flow:

1. Malicious actor calls claimProfit.

2. Tokens are transferred to the malicious actor’s address.

3. Before the savedProfit[msg.sender] state variable is updated, the actor reenters the claimProfit function.

4. The attacker claims more tokens than they are entitled to by exploiting the reentrancy gap.

5. This recursive attack can continue, leading to fund draining.

Vulnerability Details

• Vulnerability Type: Reentrancy attack.

• Location in code: The issue is in the claimProfit function, specifically with the order of state updates and token transfers.

• Token Transfer: The contract calls IERC20(currency).safeTransfer(msg.sender, profit) before updating the state (i.e., savedProfit[msg.sender] = 0).

Impact

This vulnerability can lead to a loss of funds from the contract. Malicious users can exploit the reentrancy gap to repeatedly claim profits, draining the contract's available balance. Depending on how much profit has accumulated and the number of reentrancy calls, this can result in serious financial damage.

Proof of Concept

Here’s how a malicious actor could exploit the vulnerability:

1. Deploy a malicious contract that calls the claimProfit function in the target contract.

2. In the fallback function of the malicious contract, call claimProfit recursively.

3. This will allow the malicious actor to drain the contract’s balance by continuously claiming profit before the state is updated.

Example of a malicious contract:

Recommendations

To fix the issue and prevent reentrancy attacks, the claimProfit function should be modified to update the state first and then transfer the tokens to the user. This eliminates the reentrancy window that allows recursive calls before the state is updated.

Fixed Code Snippet:

Additional safeguard:

Consider adding a reentrancy guard (e.g., using the ReentrancyGuard from OpenZeppelin) to prevent multiple nested calls to the claimProfit function.

File location: MembershipERC1155.sol