DatingDapp
First Flight #33
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Severity: low
Invalid

[H-2] Unchecked Ether Transfer in withdrawFees() Function

0xsnoweth

Summary

The withdrawFees() function in LikeRegistry.sol transfers ETH from the contract to the owner without verifying the recipient's validity. If owner() returns an unintended or compromised address (e.g., address(0), an external contract, or an address controlled by an attacker), the funds may be lost or misused. Implementing additional recipient validation is recommended to ensure the transfer behaves as expected.

Vulnerability Details

Code Snippet:

function withdrawFees() external onlyOwner {
require(totalFees > 0, "No fees to withdraw");
uint256 totalFeesToWithdraw = totalFees;
​
totalFees = 0;
- (bool success,) = payable(owner()).call{value: totalFeesToWithdraw}(""); // ❌ No recipient validation
require(success, "Transfer failed");
}
​

Why is this an issue?

  1. No validation on owner()

If owner() returns an unintended or uninitialized address (e.g., address(0)), ETH could be sent to a burn address and permanently lost.
If owner() is set to a contract that does not accept ETH, the transfer will always fail.

  1. No reentrancy protection
    If the owner() is a contract with malicious behavior, it could reenter the contract and drain additional funds before state changes are finalized.

PoC:

Consider a scenario where the owner() address is accidentally set to address(0).

  1. The contract accumulates fees:

totalFees = 10 ether;
​

  1. The owner is unintentionally set to address(0).

transferOwnership(address(0)); // Owner is now an invalid address
​

  1. Calling withdrawFees() sends 10 ETH to address(0), permanently locking the funds.

Impact

High Severity: This vulnerability can cause an irreversible loss of funds if the recipient is invalid.

Reentrancy Risk: If owner() is a malicious contract, it could execute reentrant attacks.

Failed Transfers: If owner() is a contract without a payable fallback function, ETH transfers will fail, locking funds in the contract.

Tools Used

Manual Review

Aderyn

Recommendations

Add recipient validation and reentrancy protection using nonReentrant from OpenZeppelin’s ReentrancyGuard:

function withdrawFees() external onlyOwner nonReentrant {
require(totalFees > 0, "No fees to withdraw");
address payable recipient = payable(owner());
require(recipient != address(0), "Invalid recipient");
​
uint256 totalFeesToWithdraw = totalFees;
totalFees = 0;
​
(bool success,) = recipient.call{value: totalFeesToWithdraw}("");
require(success, "Transfer failed");
}
​

Alternative Fix Using send() or transfer():

Instead of call{value: ...}(""), which can lead to reentrancy risks, use .send() or .transfer() for additional safety:

(bool success) = recipient.send(totalFeesToWithdraw);
require(success, "Transfer failed");
​

or

recipient.transfer(totalFeesToWithdraw); // Auto-reverts on failure
​

However, transfer() and send() limit gas to 2300, which might cause issues if owner() is a smart contract.

Additional Considerations:

Use a Multi-Signature Wallet for Withdrawals.

Instead of allowing a single owner to withdraw funds, use a multisig wallet (e.g., Gnosis Safe) for enhanced security.

Emit an Event for Transparency

Add an event to log withdrawals:

event FeesWithdrawn(address indexed recipient, uint256 amount);
​

Emit the event inside withdrawFees().

Updates

Appeal created

n0kto Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Admin is trusted

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelyhood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point.

Support

FAQs

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