Description
collectFee sweeps the contract's accrued WETH and ETH to the fee collector. The contract already imports SafeERC20 and uses safeTransferFrom in buySnow.
Two defects in the same six lines. The WETH leg uses a raw transfer and discards the boolean, so a token that signals failure by returning false rather than reverting makes the fees silently stay put while the call reports success. The ETH leg then pushes the entire ETH balance behind a require, so a collector that cannot accept ETH reverts the whole function — taking the WETH leg down with it.
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:
The unchecked return matters whenever the configured WETH-like token returns false instead of reverting — the behaviour SafeERC20 exists to normalise, and the reason the codebase's own linter flags erc20-unchecked-transfer at this line.
The ETH push reverts whenever s_collector is a contract without a payable receive/fallback, or one whose receive hook consumes more than the forwarded gas — a multisig or splitter contract is a natural choice for a fee collector.
ETH genuinely accrues here: the exact-payment branch of buySnow leaves msg.value in the contract, so the ETH balance is non-zero in normal operation and the push is reached.
Impact:
Protocol revenue is lost or permanently stranded. In the false-return case collectFee emits success while transferring nothing, so the loss is silent and the operator has no signal. In the non-receiving-collector case every call reverts and neither the WETH nor the ETH can ever be withdrawn.
There is no owner-level rescue path: Ownable grants no powers anywhere in the contract (see separate finding), so the only remedy is changeCollector, which is itself onlyCollector and therefore unavailable to a collector that cannot transact.
Proof of Concept
Paste the harness and test function(s) into test/PoC.t.sol, placing test functions inside contract PoC. Any additional helper contracts go outside contract PoC. Run forge test --match-path test/PoC.t.sol -vv.
pragma solidity ^0.8.24;
import {Test, console2} from "forge-std/Test.sol";
import {Snow} from "../src/Snow.sol";
import {Snowman} from "../src/Snowman.sol";
import {SnowmanAirdrop} from "../src/SnowmanAirdrop.sol";
import {MockWETH} from "../src/mock/MockWETH.sol";
contract PoC is Test {
Snow snow;
Snowman nft;
MockWETH weth;
address collector = makeAddr("collector");
address attacker = makeAddr("attacker");
address victim = makeAddr("victim");
function setUp() public {
weth = new MockWETH();
snow = new Snow(address(weth), 1, collector);
nft = new Snowman("ipfs://snowman");
}
function _leaf(address who, uint256 amount) internal pure returns (bytes32) {
return keccak256(bytes.concat(keccak256(abi.encode(who, amount))));
}
}
contract FalseWETH {
mapping(address => uint256) public balanceOf;
mapping(address => mapping(address => uint256)) public allowance;
function mint(address to, uint256 amt) external { balanceOf[to] += amt; }
function approve(address sp, uint256 amt) external returns (bool) { allowance[msg.sender][sp] = amt; return true; }
function transfer(address, uint256) external pure returns (bool) { return false; }
function transferFrom(address f, address t, uint256 amt) external returns (bool) {
allowance[f][msg.sender] -= amt; balanceOf[f] -= amt; balanceOf[t] += amt; return true;
}
}
contract RejectingCollector {
function collect(Snow s) external { s.collectFee(); }
}
function test_CollectFeeIgnoresFailedTransferReturnValue() public {
FalseWETH bad = new FalseWETH();
Snow s = new Snow(address(bad), 1, collector);
bad.mint(address(s), 5 ether);
vm.prank(collector);
s.collectFee();
assertEq(bad.balanceOf(address(s)), 5 ether, "fees still stuck in the contract");
assertEq(bad.balanceOf(collector), 0, "collector received nothing, yet the call succeeded");
}
function test_NonReceivingCollectorBricksAllFeeCollection() public {
RejectingCollector rc = new RejectingCollector();
Snow s = new Snow(address(weth), 1, address(rc));
address buyer = makeAddr("buyer2");
vm.deal(buyer, 10 ether);
vm.prank(buyer);
s.buySnow{value: 1 ether}(1);
weth.mint(address(s), 5 ether);
vm.expectRevert("Fee collection failed!!!");
rc.collect(s);
assertEq(weth.balanceOf(address(s)), 5 ether, "WETH fees stranded by the ETH push");
}
Recommended Mitigation
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!!!");
+ uint256 ethCollection = address(this).balance;
+ if (ethCollection > 0) {
+ (bool collected,) = payable(s_collector).call{value: ethCollection}("");
+ require(collected, "Fee collection failed!!!");
+ }
}
