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

Unchecked transfer: `Snow.collectFee()` ignores return value by i_weth.transfer, potentially leading to silent failures.

mustby

Snow.collectFee() ignores return value by i_weth.transfer, potentially leading to silent failures.

Description

The Snow.collectFee function calls i_weth.transfer(s_collector, collection) without checking the boolean return value. If the WETH transfer fails (e.g., due to a reverting s_collector, insufficient balance, or non-standard token), the function proceeds to transfer ETH, assuming the WETH transfer succeeded. This could lead to silent failures, accounting errors, or funds being locked if s_collector is misconfigured.

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!!!");
}

Risk

Likelihood: MEDIUM

  • This could occur due to a reverting s_collector, insufficient balance, or non-standard token.

Impact: MEDIUM

  • The function will proceed to transfer ETH, assuming the WETH transfer succeeded. This could lead to silent failures, accounting errors, or funds being locked if s_collector is misconfigured.

Proof of Concept

  • Deploy Snow.sol with MockWETHFail (returns false on transfer).

  • Fund contract with WETH (jerry calls buySnow) and ETH (victory calls buySnow).

  • Call collectFee as collector.

  • Result: ETH is sent to collector, WETH remains in contract due to unchecked i_weth.transfer.

  • See testCollectFeeSilentWETHFailure in TestSnow.t.sol.

First, deploy this contract:

contract MockWETHFail is ERC20 {
constructor() ERC20("Wrapped Ether Fail", "WETHF") {}
​
function deposit() external payable {
_mint(msg.sender, msg.value);
}
​
function mint(address to, uint256 amount) external {
_mint(to, amount);
}
​
function transfer(address, uint256) public override returns (bool) {
return false;
}
​
function transferFrom(address from, address to, uint256 amount) public override returns (bool) {
_transfer(from, to, amount);
return true;
}
}

Then, add this test to TestSnow.t.sol and run forge test --match-test testCollectFeeSilentWETHFailure. Make sure to account for precision factors in the test setUp:

function testCollectFeeSilentWETHFailure() public {
// Step 1: Deploy Snow with a mock WETH that returns false on transfer
MockWETHFail wethFail = new MockWETHFail();
vm.prank(address(this)); // Test contract deploys the mock WETH
Snow snowWithFail = new Snow(address(wethFail), FEE, collector);
​
// Step 2: Fund the contract with WETH and ETH via buySnow
uint256 amount = 100;
uint256 cost = FEE * amount * 1e18; // s_buyFee is scaled by PRECISION
​
// Jerry buys with WETH
vm.prank(jerry);
wethFail.mint(jerry, cost);
vm.prank(jerry);
wethFail.approve(address(snowWithFail), cost);
vm.prank(jerry);
snowWithFail.buySnow(amount);
​
// Victory buys with ETH
vm.prank(victory);
snowWithFail.buySnow{value: cost}(amount);
​
// Verify initial balances
uint256 initialWETHBalance = wethFail.balanceOf(address(snowWithFail));
uint256 initialETHBalance = address(snowWithFail).balance;
uint256 collectorInitialETH = collector.balance;
assertEq(initialWETHBalance, cost, "Snow should have WETH");
assertEq(initialETHBalance, cost, "Snow should have ETH");
​
// Step 3: Call collectFee and observe ETH is sent, but WETH remains
vm.prank(collector);
snowWithFail.collectFee();
​
// Verify results
assertEq(wethFail.balanceOf(address(snowWithFail)), initialWETHBalance, "WETH should remain in contract");
assertEq(address(snowWithFail).balance, 0, "ETH should be transferred");
assertEq(collector.balance, collectorInitialETH + initialETHBalance, "Collector should receive ETH");
}

Recommended Mitigation

In order to fix this problem, I recommend using safeTransfer instead of the standard transfer.

I would also recommend emitting an event of FeeCollected() because we have that initialized as an event already and we're not currently using it in this function.

function collectFee() external onlyCollector {
uint256 collection = i_weth.balanceOf(address(this));
- i_weth.transfer(s_collector, collection);
+ i_weth.safeTransfer(s_collector, collection);
(bool collected,) = payable(s_collector).call{value: address(this).balance}("");
require(collected, "Fee collection failed!!!");
+ emit FeeCollected();
}
Updates

Lead Judging Commences

yeahchibyke Lead Judge 19 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.