Mystery Box
First Flight #25
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Invalid

Potential Funds Lockup in `withdrawFunds()` Function

cryptolappi

Summary

The withdrawFunds function in the MysteryBox contract has the potential to permanently lock funds if the owner address becomes a contract that cannot receive ETH or if the owner loses access to their account.

Vulnerability Details

The vulnerable code is in the withdrawFunds function:

function withdrawFunds() public {
@> require(msg.sender == owner, "Only owner can withdraw");
@> (bool success,) = payable(owner).call{value: address(this).balance}("");
require(success, "Transfer failed");
}

The issues with this implementation are:

  1. It assumes the owner is always able to receive ETH.

  2. It sends the entire balance of the contract in one transaction.

  3. There's no way to partially withdraw funds or change the withdrawal address.

Impact

The impact of this vulnerability is medium:

  1. Fund Lockup: If the owner address becomes a contract that cannot receive ETH (e.g., a contract without a receive() or fallback() function), all funds in the MysteryBox contract could become permanently locked.

  2. Single Point of Failure: If the owner loses access to their account (e.g., lost private key), there's no way to recover or withdraw the funds.

  3. Inflexibility: The inability to partially withdraw funds or change the withdrawal address limits the contract's adaptability to different scenarios.

Tools Used

Manual code review.

Recommendations

To address this vulnerability, consider implementing the following improvement:

  1. Add a safety check to ensure the withdrawal address can receive ETH:

function withdrawFunds(uint256 _amount) public {
require(msg.sender == owner, "Only owner can withdraw");
require(_amount <= address(this).balance, "Insufficient funds");
// Check if the withdrawal address is a contract
uint256 size;
address addr = withdrawalAddress;
assembly { size := extcodesize(addr) }
// If it's a contract, ensure it has a receive() or fallback() function
if(size > 0) {
(bool canReceive,) = withdrawalAddress.call{value: 0}("");
require(canReceive, "Withdrawal address cannot receive ETH");
}
(bool success,) = payable(withdrawalAddress).call{value: _amount}("");
require(success, "Transfer failed");
}
Updates

Appeal created

inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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