Snowman Merkle Airdrop
First Flight #42
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Impact: medium
Likelihood: high
Invalid

Improperly Bundled Asset Transfers Lead to Frozen Funds

howiecht

Improperly bundling the WETH and native ETH withdrawals into a single transaction creates a Denial of Service vector

Description

  • The collectFee() function is designed to withdraw all accumulated WETH and native ETH fees from the contract and send them to the s_collector address. The developer intended for this to be an atomic operation.

  • The problem is that the WETH transfer and the ETH transfer are improperly bundled. The function first transfers WETH and then uses a low-level .call() to send ETH. If the s_collector is a contract that cannot receive ETH (e.g., it lacks a receive() function or its function reverts), the .call() will fail. This failure triggers the require() statement, which reverts the entire transaction. As a result, the initial successful WETH transfer is also rolled back, making it impossible to collect the WETH and effectively locking the funds.

// Root cause in the codebase with @> marks to highlight the relevant section
function collectFee() external onlyCollector {
uint256 collection = i_weth.balanceOf(address(this));
@> i_weth.transfer(s_collector, collection);
@> (bool collected,) = payable(s_collector).call{value: address(this).balance}("");
@> require(collected, "Fee collection failed!!!"); // A failed ETH transfer causes the entire transaction to revert.
}

Risk

Likelihood:

  • This vulnerability occurs when the s_collector address is set to a smart contract that does not implement a payable fallback() or receive() function.

  • This also occurs when the s_collector is a contract whose receive() function reverts for any reason, such as failing an internal requirement, running out of gas, or being intentionally malicious.

Impact:

  • Denial of Service (DoS): The collectFee() function becomes unusable. No fees of any kind can be withdrawn from the contract.

  • Permanent Loss of Funds: All WETH held by the contract becomes permanently frozen and irrecoverable, as the only function designed to withdraw it is bricked. The same applies to the native ETH.

Proof of Concept

The following test demonstrates the vulnerability. A MaliciousCollector contract is set as the fee collector. This contract is designed to reject any incoming native ETH. When collectFee() is called, the transaction reverts.

// A collector contract that maliciously rejects ETH payments
contract MaliciousCollector {
receive() external payable {
revert("I reject ETH!");
}
}
// Adding this test to test suite
function testCollectFeeFailed() public {
vm.startPrank(jerry);
weth.approve(address(snow), FEE);
snow.buySnow(1);
vm.stopPrank();
​
vm.prank(victory);
snow.buySnow{value: FEE}(1);
​
MaliciousCollector mc = new MaliciousCollector();
vm.prank(collector);
snow.changeCollector(address(mc)); // change collector to malicious collector
vm.prank(address(mc));
vm.expectRevert();
snow.collectFee(); // expect fails
​
assert(weth.balanceOf(address(mc)) == 0);
assert(address(mc).balance == 0);
assert(address(snow).balance == FEE);
assert(weth.balanceOf(address(snow)) == FEE);
}

Recommended Mitigation

Separate the collection logic into two distinct functions. This isolates the failure of one asset transfer from the other, preventing a DoS vector and ensuring that at least one type of asset can always be collected.

- function collectFee() external onlyCollector {
- uint256 collection = i_weth.balanceOf(address(this));
- i_weth.transfer(s_collector, collection);
- // collect both ieth and eth together, if one of tx fails, both are fail.
- (bool collected,) = payable(s_collector).call{value: address(this).balance}("");
- require(collected, "Fee collection failed!!!");
- }
​
+ function collectWeth() external onlyCollector {
+ uint256 amount = i_weth.balanceOf(address(this));
+ if (amount > 0) {
+ i_weth.transfer(s_collector, amount);
+ }
+ }
​
+ function collectEth() external onlyCollector {
+ uint256 amount = address(this).balance;
+ if (amount > 0) {
+ (bool success,) = payable(s_collector).call{value: amount}("");
+ require(success, "ETH collection failed");
+ }
+ }
Updates

Lead Judging Commences

yeahchibyke Lead Judge
5 months ago
yeahchibyke Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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