Trick or Treat
First Flight #27
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: low
Valid

Gas-Limited ETH Transfer in withdrawFees

abhishekthakur

Summary

  • Root Cause: Usage of transfer() which has a hard gas limit of 2300 gas

  • Impact: Funds could be permanently locked if the owner is a smart contract (e.g., Gnosis Safe, other smart wallets)

Vulnerability Details

https://github.com/Cyfrin/2024-10-trick-or-treat/blob/main/src/TrickOrTreat.sol#L146-L150

function withdrawFees() public onlyOwner {
uint256 balance = address(this).balance;
/// @audit: use of transfer is probelamatic for smart wallets
payable(owner()).transfer(balance);
emit FeeWithdrawn(owner(), balance);
}

The withdrawFees() function uses the transfer() method to send ETH to the owner, However it is problematic because:

  1. transfer() has a hard gas limit of 2300 gas

  2. Smart contract wallets (like Gnosis Safe) typically require more than 2300 gas to process incoming ETH

  3. Modern smart wallets commonly used as multi-sigs would fail to receive funds

  4. The operation would revert, effectively locking the fees in the contract

POC

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;
import "forge-std/Test.sol";
import "../src/SpookySwap.sol";
​
contract GnosisSafeMock is Test {
// Simulate Gnosis Safe receive function that uses more than 2300 gas
receive() external payable {
// Perform some operations that consume gas
uint256 a = 1;
for(uint i = 0; i < 10; i++) {
a = a * 2;
}
}
}
​
contract SpookySwapTest is Test {
SpookySwap spookySwap;
GnosisSafeMock safeWallet;
function setUp() public {
SpookySwap.Treat[] memory treats = new SpookySwap.Treat[]();
treats[0] = SpookySwap.Treat("Candy", 1 ether, "uri");
spookySwap = new SpookySwap(treats);
safeWallet = new GnosisSafeMock();
}
function testWithdrawToSmartWallet() public {
// First, let's add some ETH to the contract
address(spookySwap).call{value: 1 ether}("");
// Change owner to our mock Gnosis Safe
spookySwap.changeOwner(address(safeWallet));
// This will fail because transfer() can't handle smart contract wallet
vm.expectRevert();
spookySwap.withdrawFees();
}
}

Impact

Funds will be locked in contract if owner is a smart wallet or contract that requires more than 2300 gas to receive ether

Tools Used

Manual Review

Recommendations

Replace transfer() with the recommended call() pattern:

function withdrawFees() public onlyOwner {
uint256 balance = address(this).balance;
(bool success, ) = payable(owner()).call{value: balance}("");
require(success, "Fee withdrawal failed");
emit FeeWithdrawn(owner(), balance);
}
Updates

Appeal created

bube Lead Judge 9 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Use of `transfer` instead of `call`

Support

FAQs

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