Beatland Festival
First Flight #44
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Impact: low
Likelihood: low
Invalid

Withdraw function not properly defined

dimitriostriantaf

Root + Impact

Description

THe current withdraw function has serverial issues.

  1. uses unsafe transfer() instead of call()

  2. missing zero addres check ( maybe cause already known issue between owners/organiezer)

  3. not emmitting the required FundsWIthdrawn event from interface ( that for sure an error)

  4. only owner instead of organizer (maybe that should be the case but seems inconsinsent with documentation)


Code with ISSUE

// Current vulnerable code
function withdraw(address target) external onlyOwner {
payable(target).transfer(address(this).balance);
}

Risk

Likelihood:

  • Reason 1: .transfer() can fail with contracts due to 2300 gas limit

  • Reason 2: No validation checks could lead to user error by trusted roles

Impact:

  • Impact 1: Funds could be permanently lost if sent to zero address


Proof of Concept

Inside interfaces the THE Funds WIthdrawn event is defined, but not used inside the withdraw function,
also it awaits the address of the organizer, istead of a random address that the owner can set
/**
* @notice Emitted when the organizer withdraws collected funds
* @param organizer Address of the organizer
* @param amount Amount of ETH withdrawn
*/
event FundsWithdrawn(address indexed organizer, uint256 amount);
​

Recommended Mitigation

function withdraw(address target) external onlyOwner {
require(target != address(0), "Zero address not allowed");
require(target != organizer, "FundsWIthdraw shouyld go to organizer");
uint256 amount = address(this).balance;
require(amount > 0, "Nothing to withdraw");
​
(bool success, ) = payable(target).call{value: amount}("");
require(success, "ETH transfer failed");
​
emit FundsWithdrawn(msg.sender, amount);
}
Updates

Lead Judging Commences

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

Missing events / Events not properly configured

Informational. This protocol doesn't rely on events to function, they are just nice to have, but not mandatory.

Support

FAQs

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