Submission Details
Impact:
Likelihood:
`Snow::changeCollector` has no owner override, allowing the collector role to permanently and irrecoverably brick fee collection
`Snow::changeCollector` has no owner override, allowing the collector role to permanently and irrecoverably brick fee collection
Description
`Snow::changeCollector` is gated by `onlyCollector` with no `onlyOwner` override or recovery path, and validates the new collector only against `address(0)`. If the current collector sets `s_collector` to a contract address that cannot receive ETH (no `receive` or `fallback` function, or one that deliberately reverts), `collectFee`'s unconditional ETH sweep will permanently revert on every future call, and since only the collector itself can call `changeCollector` again. There is no way for anyone, including the contract owner, to recover or reassign the role if this happens.
function changeCollector(address _newCollector) external onlyCollector {
if (_newCollector == address(0)) {
revert S__ZeroAddress();
}
s_collector = _newCollector;
emit NewCollector(_newCollector);
}
No validation that `_newCollector` can receive ETH. Only a zero-address check is performed. Nothing prevents setting `s_collector` to a contract with no payable `fallback`, or one that reverts on receipt of ETH. No recovery path once misconfigured. `changeCollector` is `onlyCollector`, not `onlyOwner`.
```solidity
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!!!");
}
```
Once `s_collector` is stuck this way, `collectFee`'s `require` statement will revert on every call, since `.call` to the broken collector always fails. Because both the WETH sweep and ETH sweep happen in the same function, this also blocks recovery of accumulated WETH fees, even though WETH itself transferred successfully. The function reverts as a whole because of the require statement.
Risk
Likelihood:
Low-to-Medium. Not exploitable by an arbitrary third party. Requires the current collector (a privileged role) to make an error or be compromised. However, the consequence is total and permanent, with no operational safeguard or recovery path once triggered.
Impact:
High impact (total, permanent loss of fee-collection function) combined with a non-default but realistic precondition (privileged-role error or compromise) places this at Medium per the standard impact/likelihood matrix
Proof of Concept
Add the following contract to TestSnow.t.sol:
/// @notice A contract with no receive() or payable fallback(),
contract BadCollector {
// Intentionally no receive() or fallback() — any plain ETH transfer
// sent to this contract via .call{value: ...}("") will fail.
}
Add the contract as a variable to the TestSnow contract.
BadCollector badCollector;
Initialize it in the setUp function:
badCollector = new BadCollector();
Add the following function to the test suite:
function test_CollectorCanPermanentlyBrickFeeCollection() public {
// collectFee works normally
vm.prank(collector);
snow.collectFee(); // should succeed
// Re-fund the contract to test collection
vm.deal(address(snow), 5 ether);
// current collector changes s_collector to a contract with no receive or fallback
vm.prank(collector);
snow.changeCollector(address(badCollector));
assertEq(snow.getCollector(), address(badCollector), "Collector should now be the bad contract");
// collectFee() now permanently reverts
vm.prank(address(badCollector));
vm.expectRevert("Fee collection failed!!!");
snow.collectFee();
// owner cannot call changeCollector, since it's onlyCollector, not onlyOwner.
vm.prank(address(deployer));
vm.expectRevert(); // reverts
snow.changeCollector(collector);
}
Run the test in the terminal:
forge test --mt test_CollectorCanPermanentlyBrickFeeCollection -vvvv
If the test passes the exploit is possible.
Recommended Mitigation
Give the `owner` an override. Allow `onlyOwner` to also call `changeCollector` (or add a separate `onlyOwner` emergency-reset function), so a bad collector role isn't unrecoverable:
+ error S__NotAuthorized();
+ function changeCollector(address _newCollector) external {
+ if (msg.sender != s_collector && msg.sender != owner()) {
+ revert S__NotAuthorized();
+ }
+ if (_newCollector == address(0)) {
+ revert S__ZeroAddress();
+ }
+ s_collector = _newCollector;
+ emit NewCollector(_newCollector);
+ }
// Another possible mitigation is switching to a push over pull style withdrawal. Additionally, decoupling WETH and ETH withdrawals would prevent failed ETH transfers from reverting WETH transfers.
Rewards breakdown
This contest has a flat reward structure with the following payouts:
Live
The contest is live. Earn rewards by submitting a finding.
Judging
Submissions are being reviewed by our AI judge. Results will be available in a few minutes.
View all submissionsRewards Distribution
The contest is complete and the rewards are being distributed.