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

LOCKED ETHER in MembershipERC1155

0bingo76

Summary

Ether can be permanently locked within the MembershipERC1155.sol contract. While the contract includes functionality to forward Ether through callExternalContract, it lacks a mechanism to withdraw any Ether that might accumulate in the contract through other means (such as selfdestruct calls from other contracts).

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

Vulnerability Details

The vulnerability stems from two main issues:

  1. The contract can receive Ether through:

    • The callExternalContract function which is marked as payable

    • Force-sending methods (like selfdestruct) from other contracts

  2. The contract lacks:

    • A withdrawal mechanism for accumulated Ether

    • Explicit receive() or fallback() functions to handle incoming Ether

    • Any way to recover Ether that becomes trapped

Proof of Concept

  1. An attacker can force-send Ether to the contract using selfdestruct

  2. The contract has no mechanism to withdraw this Ether

  3. The Ether becomes permanently locked in the contract

// Example of how Ether can be force-sent
contract Attacker {
function attack(address target) public payable {
selfdestruct(payable(target));
}
}

Impact

The vulnerability can result in:

  1. Permanent loss of any Ether sent to the contract

  2. No recovery mechanism for accidentally transferred funds

  3. Potential accumulation of locked Ether over time

The impact is considered High because:

  • It results in permanent loss of funds

  • There is no workaround once Ether is locked

  • The issue cannot be fixed without contract redeployment

Tools Used

  1. SolidityScan - Static Analysis Tool

  2. Manual Code Review

  3. Solidity Documentation Reference

Recommendations

1. Add Withdrawal Functionality

/**
* @notice Allows withdrawal of Ether from the contract
* @param to Recipient address
* @param amount Amount to withdraw in wei
*/
function withdrawEther(address payable to, uint256 amount)
external
onlyRole(DEFAULT_ADMIN_ROLE)
{
require(to != address(0), "Invalid recipient");
require(amount <= address(this).balance, "Insufficient balance");
(bool success, ) = to.call{value: amount}("");
require(success, "Transfer failed");
emit EtherWithdrawn(to, amount);
}
​
event EtherWithdrawn(address indexed to, uint256 amount);

2. Implement Receive Function (Optional)

If the contract needs to receive Ether:

/**
* @notice Allows the contract to receive Ether
*/
receive() external payable {
emit EtherReceived(msg.sender, msg.value);
}
​
event EtherReceived(address indexed from, uint256 amount);

3. Add Balance Checking Function

/**
* @notice Returns the contract's Ether balance
*/
function getContractBalance() external view returns (uint256) {
return address(this).balance;
}

4. Alternative Approach

If the contract should not handle Ether at all:

  1. Remove the payable modifier from callExternalContract

  2. Add checks to prevent Ether reception

  3. Document clearly that the contract should not receive Ether

While the contract's primary purpose may not involve handling Ether directly, the ability to receive Ether without withdrawal capabilities creates an unacceptable risk of permanent fund loss.

The recommended solutions provide multiple options for addressing this vulnerability, with the choice depending on the intended behavior of the contract.

Updates

Lead Judging Commences

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!