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

[L-01] `collectFee` drags the WETH transfer down when the ETH send fails, potentially locking fees forever

alvarok29

Description

  • The collector should always be able to withdraw accumulated fees, which exist in both ETH and WETH.

  • collectFee sends the WETH and then the ETH in the same transaction, requiring the ETH send to succeed. If the collector is a contract that cannot receive ETH, the require reverts the whole transaction and drags down the WETH transfer that would have succeeded. Since changeCollector is onlyCollector, a collector unable to call it leaves the fees locked forever.

function collectFee() external onlyCollector {
i_weth.transfer(s_collector, i_weth.balanceOf(address(this))); // would succeed
@> (bool collected,) = payable(s_collector).call{value: address(this).balance}("");
@> require(collected, "Fee collection failed!!!"); // reverts everything, incl. the WETH
}

Risk

Likelihood:

  • The collector is configured as a contract that cannot receive ETH (a treasury or mis-set multisig).

Impact:

  • WETH fees are stuck even though their transfer is viable, blocked by a failing ETH send.

  • If the collector also cannot call changeCollector, all fees (ETH and WETH) are locked permanently; no rescue path exists.

Proof of Concept

The test accrues fees in both ETH and WETH, sets a collector contract that cannot receive ETH, and shows collectFee reverts entirely - leaving the transferable WETH stuck - and that with a collector unable to call changeCollector the fees are locked permanently.

Verified with Foundry (test_elWethSeQuedaAtrapadoPorCulpaDelEnvioDeEth, test_siElCollectorNoPuedeCambiarseNoHaySalida), forge test passing:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;
​
import {Test, console2} from "forge-std/Test.sol";
import {Snow} from "../src/Snow.sol";
import {DeploySnow} from "../script/DeploySnow.s.sol";
import {MockWETH} from "../src/mock/MockWETH.sol";
​
/// Un collector que es un contrato SIN `receive`/`fallback` payable: no puede aceptar ETH,
/// pero sí puede ejecutar llamadas (como haría una tesorería o un multisig mal configurado).
contract CollectorSinReceive {
function cobrar(Snow snow) external {
snow.collectFee();
}
​
function cambiar(Snow snow, address nuevo) external {
snow.changeCollector(nuevo);
}
}
​
/// Un collector que además no tiene forma de llamar a `changeCollector`: aquí ya no hay salida.
contract CollectorInerte {
function cobrar(Snow snow) external {
snow.collectFee();
}
}
​
/// @notice PoC: `collectFee()` mezcla dos cobros independientes en una sola transacción - manda
/// el WETH y después el ETH - y exige que el envío de ETH tenga éxito (L106). Si el collector no
/// puede recibir ETH, el `require` revierte **toda** la transacción y arrastra consigo el WETH,
/// que sí se habría podido transferir sin problema.
contract ComisionesBloqueadasTest is Test {
Snow snow;
DeploySnow deployer;
MockWETH weth;
uint256 FEE;
​
address comprador = makeAddr("comprador");
​
function setUp() public {
deployer = new DeploySnow();
snow = deployer.run();
weth = deployer.weth();
FEE = deployer.FEE();
}
​
/// Genera comisiones reales en el contrato: unas en ETH y otras en WETH.
function _generarComisiones() private {
vm.deal(comprador, FEE);
vm.prank(comprador);
snow.buySnow{value: FEE}(1); // fee en ETH
​
weth.mint(comprador, FEE);
vm.startPrank(comprador);
weth.approve(address(snow), FEE);
snow.buySnow(1); // fee en WETH
vm.stopPrank();
​
assertGt(address(snow).balance, 0, "hay ETH acumulado");
assertGt(weth.balanceOf(address(snow)), 0, "y WETH acumulado");
}
​
function test_elWethSeQuedaAtrapadoPorCulpaDelEnvioDeEth() public {
CollectorSinReceive collector = new CollectorSinReceive();
​
// el collector actual traspasa el puesto al contrato que no acepta ETH
address collectorInicial = snow.getCollector();
vm.prank(collectorInicial);
snow.changeCollector(address(collector));
​
_generarComisiones();
uint256 wethAcumulado = weth.balanceOf(address(snow));
​
// cobrar revierte ENTERO, aunque el WETH se podria enviar perfectamente
vm.expectRevert(); // "Fee collection failed!!!"
collector.cobrar(snow);
​
assertEq(weth.balanceOf(address(snow)), wethAcumulado, "el WETH sigue atrapado");
assertEq(weth.balanceOf(address(collector)), 0, "y el collector no ha cobrado nada");
console2.log("WETH bloqueado por un envio de ETH que falla:", wethAcumulado);
}
​
function test_siElCollectorNoPuedeCambiarseNoHaySalida() public {
CollectorInerte collector = new CollectorInerte();
​
address collectorInicial = snow.getCollector();
vm.prank(collectorInicial);
snow.changeCollector(address(collector));
​
_generarComisiones();
​
// no puede cobrar...
vm.expectRevert();
collector.cobrar(snow);
​
// ...y `changeCollector` es onlyCollector, asi que nadie mas puede rescatarlo
vm.prank(collectorInicial);
vm.expectRevert(); // S__NotAllowed: el antiguo collector ya no manda
snow.changeCollector(address(this));
​
assertGt(weth.balanceOf(address(snow)), 0, "comisiones bloqueadas para siempre");
console2.log("ETH bloqueado: ", address(snow).balance);
console2.log("WETH bloqueado:", weth.balanceOf(address(snow)));
}
}

Recommended Mitigation

Decouple the two transfers so a failing ETH send cannot drag down the WETH, and use safeTransfer:

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

Also let the owner reassign the collector (two-step) so a broken collector isn't irreversible; and note i_weth.transfer ignores its return value (use safeTransfer).

Updates

Lead Judging Commences

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