Pieces Protocol
First Flight #32
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Severity: low
Valid

Contract permanently locks ETH fees from token sales with no withdrawal mechanism

kweks

Description: The TokenDivider contract collects ETH fees (1% of transaction value) during token sale operations but lacks any mechanism to withdraw these accumulated fees. This creates a critical vulnerability where ETH becomes permanently locked within the contract. The contract implements fee collection during token sales but fails to provide the contract owner or any other authorized party with the ability to retrieve these fees, effectively burning all collected fees permanently.

Impact: The impact of this vulnerability is severe for several reasons. First, all transaction fees collected by the contract are permanently inaccessible, resulting in a continuous and irreversible loss of funds. This directly affects the protocol's revenue model, as fees meant to compensate the protocol operators become permanently locked. As more transactions occur, the amount of locked ETH will continue to grow, potentially reaching significant values over time. Additionally, this could affect user trust in the protocol, as the permanent locking of fees might be seen as poor contract design or management.

Proof of Concept: The following test demonstrates how ETH becomes permanently locked in the contract:

function testEthCanBecomeLockedInContract() public {
// Initial setup
uint256 tokenPrice = 1 ether;
uint256 fees = tokenPrice / 100; // 1% fee
uint256 totalAmount = tokenPrice + fees; // Price plus fees
uint256 initialContractBalance = address(tokenDivider).balance;
// PART 1: Show ETH entering the contract through normal operation
// ---------------------------------------------------------------------
// Setup a sale of ERC20 tokens
vm.startPrank(USER);
erc721Mock.approve(address(tokenDivider), TOKEN_ID);
tokenDivider.divideNft(address(erc721Mock), TOKEN_ID, AMOUNT);
ERC20Mock erc20Mock = ERC20Mock(tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address);
erc20Mock.approve(address(tokenDivider), AMOUNT);
// Create sell order for 1 ETH
tokenDivider.sellErc20(address(erc721Mock), tokenPrice, AMOUNT);
vm.stopPrank();
// Send ETH to buy tokens, including fees
vm.deal(USER2, totalAmount);
vm.prank(USER2);
tokenDivider.buyOrder{value: totalAmount}(0, USER);
// Verify ETH entered the contract
assertTrue(
address(tokenDivider).balance > initialContractBalance,
"Contract should have received ETH from fees"
);
// PART 2: Demonstrate ETH cannot be withdrawn
// ---------------------------------------------------------------------
// Store the locked balance (should be the fees amount)
uint256 lockedBalance = address(tokenDivider).balance;
console.log("Locked fees in contract:", lockedBalance);
// Try to withdraw as owner
vm.startPrank(tokenDivider.owner());
// Attempt various withdrawal methods
// Try a direct transfer
(bool successTransfer,) = payable(address(tokenDivider)).call{gas: 2300}("");
assertFalse(successTransfer, "Owner should not be able to force withdraw ETH");
// Try a withdraw function call
(bool successWithdraw,) = address(tokenDivider).call(
abi.encodeWithSignature("withdraw()")
);
assertFalse(successWithdraw, "No withdraw function should exist");
vm.stopPrank();
// Verify ETH remains locked
assertEq(
address(tokenDivider).balance,
lockedBalance,
"ETH should remain locked in contract after withdrawal attempts"
);
// Document the vulnerability
console.log("\nSecurity Vulnerability Analysis:");
console.log("--------------------------------");
console.log("1. Contract accumulates fees from token sales");
console.log("2. These fees (", lockedBalance, " wei) are now permanently locked because:");
console.log(" - No withdraw function exists");
console.log(" - No owner recovery mechanism exists");
console.log(" - No emergency withdrawal system exists");
console.log("3. This means all collected fees are permanently inaccessible");
}

Recommended Mitigation: To address this vulnerability, implement a secure withdrawal mechanism for collected fees. Here are the recommended changes:

  • Add a withdrawal function accessible only by the contract owner:

function withdrawFees() external onlyOwner {
uint256 balance = address(this).balance;
require(balance > 0, "No fees to withdraw");
(bool success,) = owner().call{value: balance}("");
require(success, "Fee transfer failed");
emit FeesWithdrawn(owner(), balance);
}

  • Add an event to track fee withdrawals:

event FeesWithdrawn(address indexed owner, uint256 amount);

Consider implementing additional safety features:

  • Add a time-lock mechanism for large withdrawals

  • Implement a maximum withdrawal amount per transaction

  • Add an emergency pause mechanism for the withdrawal function

  • Consider splitting fee withdrawals among multiple authorized recipients

Add comprehensive withdrawal tracking:

mapping(uint256 => WithdrawalRecord) public withdrawalHistory;
struct WithdrawalRecord {
uint256 amount;
uint256 timestamp;
address recipient;
}

The implementation of these mitigations would ensure that collected fees can be properly managed and withdrawn by authorized parties while maintaining security and transparency of the fee collection system.

Updates

Lead Judging Commences

fishy Lead Judge 8 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Token misshandling

The extra eth sent by the user in the buy order will be locked in the contract forever

Support

FAQs

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