Snowman Merkle Airdrop

AI First Flight #10
Beginner FriendlyFoundrySolidityNFT
EXP
View results
Submission Details
Impact: low
Likelihood: low
Invalid

`Snow::collectFee` uses an unchecked `i_weth.transfer`, ignoring the return value despite importing SafeERC20

Snow::collectFee uses an unchecked i_weth.transfer, ignoring the return value despite importing SafeERC20

Description

  • Snow imports and using SafeERC20 for IERC20, indicating safe transfer semantics are intended.

  • collectFee calls the raw i_weth.transfer(...) and ignores its boolean return. A token implementation that returns false instead of reverting on failure would silently fail to move the fees.

function collectFee() external onlyCollector {
uint256 collection = i_weth.balanceOf(address(this));
i_weth.transfer(s_collector, collection); // return value ignored
...
}

Risk

Likelihood: Low

  • Low for canonical WETH, but the contract is written generically against an IERC20 and the safe pattern is already available, so the unchecked call is an avoidable latent bug.

Impact: Low

  • A failed-but-non-reverting transfer would leave fees stuck in the contract while the function reports success.

Proof of Concept

The ERC20 standard permits an implementation to signal failure by returning false instead of reverting. collectFee discards that return value, so against such a token the call appears to succeed while the fees never move.

This PoC deploys Snow with a standards-compliant WETH whose transfer returns false (no revert, no movement), accumulates fees via the WETH buy path, then calls collectFee and shows it does not revert and the balance is unchanged:

// A WETH-like token whose transfer() silently returns false instead of reverting.
contract FalseTransferWETH is ERC20 {
constructor() ERC20("FalseWETH", "fWETH") {}
function mint(address to, uint256 amt) external { _mint(to, amt); }
function transfer(address, uint256) public pure override returns (bool) {
return false; // silent failure: no revert, no balance change
}
}
function test_L2_collectFeeSilentlyFailsOnFalseTransfer() public {
FalseTransferWETH weth = new FalseTransferWETH();
Snow snow = new Snow(address(weth), FEE, collector); // FEE = 5
uint256 price = snow.s_buyFee() * 1;
// Accumulate WETH fees via the WETH buy path (msg.value == 0 != price).
weth.mint(user, price);
vm.prank(user);
weth.approve(address(snow), price);
vm.prank(user);
snow.buySnow(1);
assertEq(weth.balanceOf(address(snow)), price);
// transfer() returns false, but the return value is unchecked -> no revert.
vm.prank(collector);
snow.collectFee();
// Fees never left the contract, yet collectFee reported success.
assertEq(weth.balanceOf(address(snow)), price);
assertEq(weth.balanceOf(collector), 0);
}

Run: forge test --mt test_L2_collectFeeSilentlyFailsOnFalseTransfer -vv

Result:

[PASS] test_L2_collectFeeSilentlyFailsOnFalseTransfer() (gas: 2477408)

Recommended Mitigation

The contract already declares using SafeERC20 for IERC20, so the safe wrapper is one keyword away. safeTransfer checks the boolean return (and tolerates non-standard tokens that return no data), reverting on failure instead of silently continuing.

- i_weth.transfer(s_collector, collection);
+ i_weth.safeTransfer(s_collector, collection);

Additional hardening for the same function:

  • The native-ETH sweep uses a low-level call whose success is checked (require(collected, ...)) — good. Consider replacing the bare-string require with a custom error for consistency with the rest of the contract and lower gas, e.g. if (!collected) revert S__FeeCollectionFailed();.

  • collectFee reads i_weth.balanceOf(address(this)) then transfers it; with the SafeERC20 fix this becomes a single safe, all-or-nothing transfer, removing the silent-loss path entirely. No further accounting changes are required.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 8 hours ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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

Give us feedback!