Description
The `withdraw` function in the `FestivalPass` contract allows the contract owner to transfer all ETH
collected to a specified address. However, the implementation uses direct `transfer()`,
which is not gas-safe and can fail unexpectedly due to the fixed 2300 gas stipend limit imposed by transfer().
```javascript
function withdraw(address target) external onlyOwner {
payable(target).transfer(address(this).balance);
}
```
Direct ETH transfers using `.transfer()` are considered unsafe since EIP-1884,
which increased the gas cost of certain opcodes and may cause transfer() to fail on some smart contract wallets like multisigs.
Risk
Impact:
Risk: DoS (Denial of Service to withdraw funds), lost funds access
1. Contract owner could lose access to funds if transfer() fails due to recipient gas usage.
2. Funds could be locked forever if recipient is a contract not compatible with transfer().
Proof of Concept
Deploy `FestivalPass` and call `withdraw()` using a recipient address that has a fallback function consuming more than 2300 gas:
Add follwing in `FestivalPass.t.sol`
```javascript
contract FallbackRevert {
fallback() external payable {
require(gasleft() > 10000, "Consumes too much gas");
}
}
```
and test following test, it will be reverted due to more gas error.
```javascript
function testWithdrawRevertsOnGasHeavyFallback() public {
FallbackRevert br = new FallbackRevert();
vm.prank(user1);
festivalPass.buyPass{value: GENERAL_PRICE}(1);
vm.prank(owner);
vm.expectRevert();
festivalPass.withdraw(address(br));
}
```
- Call withdraw(address(receiver)) from owner.
- Transaction will revert due to .transfer() failing to deliver ETH.
Recommended Mitigation
Use a low-level call with manual success check, which avoids the 2300 gas limit problem:
```diff
function withdraw(address target) external onlyOwner {
+ require(target != address(0), "Invalid address");
- payable(target).transfer(address(this).balance);
+ uint256 balance = address(this).balance;
+ (bool success, ) = payable(target).call{value: balance}("");
+ require(success, "Withdraw failed");
}
```
Why this is better:
~ call forwards all available gas (unless specified otherwise).
~ More compatible with smart contract wallets.
~ Can safely handle larger logic in the recipient contract.