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

Inconsistent Access Control Comments and Event Naming in `FestivalPass::withdraw`

ayb

[M-1] Inconsistent Access Control Comments and Event Naming in FestivalPass::withdraw function.

Description: The FestivalPass::withdraw function uses the onlyOwner modifier (correctly enforcing that only the contract owner can withdraw ETH). However, the inline comment (// Organizer withdraws ETH) and the emitted event (IFestivalPass::FundsWithdrawn(address organizer, uint256 amount)) imply that the organizer, not the owner, is the authorized actor.
This mismatch between the code and the documentation/comments may lead to confusion or misimplementation in the frontend or by other developers.

Impact:

  • Developers may incorrectly assume the organizer can withdraw ETH.

  • Event consumers (e.g. analytics, frontend) might mislabel the withdrawer as the organizer.

  • While no unauthorized access occurs, the inconsistent naming and documentation pose a medium risk to system clarity and maintainability.

Proof of Concept: Add this to your FestivalPass.t.sol:

function test_organizerWithdrawEth() public {
// To not query balance of address(0) or a weird corrupted pointer
vm.deal(owner, 0 ether);
uint256 startTime = block.timestamp + 30 minutes;
uint256 duration = 2 hours;
uint256 reward = 100e18;
vm.prank(organizer);
uint256 perfId = festivalPass.createPerformance(startTime, duration, reward);
assertEq(perfId, 0);
vm.prank(user1);
// BACKSTAGE_PASS = 0.25 ether
festivalPass.buyPass{value: BACKSTAGE_PRICE}(3);
​
vm.prank(organizer);
vm.expectRevert();
// reverts because the owner isn't the organizer
festivalPass.withdraw(organizer);
assertEq(address(organizer).balance, 0 ether);
​
vm.prank(owner);
festivalPass.withdraw(owner);
assertEq(address(owner).balance, 0.25 ether);
}
​
// This allows the test contract to receive ETH via .transfer() or .call{value:...}.
receive() external payable {}

The following test demonstrates that the FestivalPass::withdraw function allows the owner to withdraw ETH, even though the in-code comment implies the organizer should be authorized.

Recommended Mitigation: Choose one of the following, depending on the intended business logic:

If only the owner should withdraw, rename the event to FundsWithdrawn(address owner, uint256 amount) and correct the inline comment.

If the organizer is the true withdrawer, change the modifier to onlyOrganizer.

Most importantly, ensure the code, comments, and interface all reflect the same authorized role to avoid confusion.

Updates

Lead Judging Commences

inallhonesty Lead Judge 26 days ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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