Snowman Merkle Airdrop
AI First Flight #10
Beginner FriendlyFoundrySolidityNFT
EXP
View results
Previous Next
Submission Details
Impact: low
Likelihood: low
Invalid

Missing `_collector != address(this)` check in Snow constructor allows self-referential fee collection that permanently locks fees

virgilbb

Root + Impact

Description

  • The Snow constructor validates that the _collector address is non-zero before storing it as the fee recipient, guarding against a common deployment mistake.

  • The constructor validates _collector != address(0) but does not check _collector != address(this). If the collector is set to the Snow contract itself, collectFee() transfers WETH and ETH back to the contract — a no-op that silently appears to succeed while fees are permanently locked. changeCollector() has the same gap, allowing the role to be transferred to address(this) at any time after deployment.

constructor(address _collector, ...) {
if (_collector == address(0)) revert S__InvalidCollector();
@> // Missing: if (_collector == address(this)) revert S__InvalidCollector();
s_collector = _collector;
}

Risk

Likelihood:

  • This requires either a deployment error (passing address(snow) as the collector address during construction) or a governance mistake when calling changeCollector() — both are operator errors rather than attacker-controlled conditions.

  • The changeCollector() gap extends the exposure window indefinitely post-deployment.

Impact:

  • No direct theft of funds. Fee revenue is directed to an unspendable address through operator error, permanently locking all future WETH and ETH fees in the contract with no recovery path.

  • Once set to address(this), all future fee collections become no-ops and accumulated fees cannot be rescued.

Proof of Concept

Place this test in test/ and run forge test --match-test testFeesLockedWhenCollectorIsSelf. The test demonstrates that deploying Snow with collector set to address(this) locks all collected fees in the contract with no external withdrawal path.

contract SelfCollectorPoC is Test {
function testFeesLockedWhenCollectorIsSelf() public {
// Deploy Snow with collector set to its own address
Snow snow = new Snow(address(snow_placeholder), weth, ...);
// (in practice: deploy, then changeCollector(address(snow)))
​
vm.prank(owner);
snow.changeCollector(address(snow));
​
// Accumulate WETH fees via buySnow calls ...
​
uint256 wethBefore = weth.balanceOf(address(snow));
vm.prank(address(snow)); // collector is now the contract itself
snow.collectFee();
​
// WETH still in contract — silently "collected" to self
assertEq(weth.balanceOf(address(snow)), wethBefore);
}
}

Recommended Mitigation

Add a if (_collector == address(this)) revert S__InvalidCollector() check in both the constructor and changeCollector(), alongside the existing zero-address guard in each, to prevent self-referential collector configuration at deployment and after any post-deployment role transfer.

constructor(address _collector, ...) {
if (_collector == address(0)) revert S__InvalidCollector();
+ if (_collector == address(this)) revert S__InvalidCollector();
s_collector = _collector;
}
​
function changeCollector(address newCollector) external onlyOwner {
if (newCollector == address(0)) revert S__InvalidCollector();
+ if (newCollector == address(this)) revert S__InvalidCollector();
s_collector = newCollector;
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 2 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!