Root + Impact
Description
-
Describe the normal behavior in one or more sentences
-
The collectFee() function should transfer accumulated WETH fees to the collector address and revert if the transfer fails, ensuring fees are never lost.
-
Explain the specific issue or problem in one or more sentences
-
The function ignores the return value of the i_weth.transfer() call. If the transfer fails (due to insufficient balance, paused token, or other reasons), the function continues execution without reverting, and the fees remain stuck in the contract.
function collectFee() external {
uint256 collection = i_weth.balanceOf(address(this));
i_weth.transfer(s_collector, collection);
(bool collected,) = address(s_collector).call{value: address(this).balance}();
require(collected, "ETH transfer failed");
}
Risk
Likelihood:
Reason 1: WETH transfers can fail for multiple reasons including paused contract state, transfer restrictions, or implementation bugs. Without checking the return value, these failures go unnoticed.
Reason 2: The function is called by external parties to collect fees. If it appears to succeed but silently fails, fees accumulate indefinitely in the contract with no way to recover them unless a fix is deployed.
Impact:
Impact 1: Permanent loss of protocol fees - Failed transfers are not detected, and accumulated WETH fees become permanently locked in the contract with no recovery mechanism.
Impact 2: False success reporting - The function completes without reverting, giving the caller false confidence that fees were collected when they actually remain in the contract. This breaks accounting and monitoring systems.
Proof of Concept
pragma solidity ^0.8.24;
import {Test, console} from "forge-std/Test.sol";
import {Snow} from "../src/Snow.sol";
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
contract FailingWETH is ERC20 {
bool public shouldFail;
constructor() ERC20("Wrapped Ether", "WETH") {}
function setFailure(bool _fail) external {
shouldFail = _fail;
}
function transfer(address to, uint256 amount) public override returns (bool) {
if (shouldFail) {
return false;
}
return super.transfer(to, amount);
}
function mint(address to, uint256 amount) external {
_mint(to, amount);
}
}
contract UncheckedTransferTest is Test {
Snow snow;
FailingWETH weth;
address collector = makeAddr("collector");
function setUp() public {
weth = new FailingWETH();
}
function testSilentFailure() public {
uint256 feeAmount = 10 ether;
weth.mint(address(snow), feeAmount);
uint256 collectorBalanceBefore = weth.balanceOf(collector);
uint256 snowBalanceBefore = weth.balanceOf(address(snow));
console.log("Collector WETH before:", collectorBalanceBefore);
console.log("Snow contract WETH before:", snowBalanceBefore);
weth.setFailure(true);
snow.collectFee();
uint256 collectorBalanceAfter = weth.balanceOf(collector);
uint256 snowBalanceAfter = weth.balanceOf(address(snow));
console.log("Collector WETH after:", collectorBalanceAfter);
console.log("Snow contract WETH after:", snowBalanceAfter);
assertEq(collectorBalanceAfter, collectorBalanceBefore, "Collector received fees when they shouldn't");
assertEq(snowBalanceAfter, feeAmount, "Fees still stuck in contract");
console.log("FEES LOCKED: ", snowBalanceAfter);
}
}
Recommended Mitigation
- remove this code
+ add this codefunction collectFee() external {
uint256 collection = i_weth.balanceOf(address(this));
- // Remove this code - unchecked transfer
- i_weth.transfer(s_collector, collection);
+ // Add this code - check return value
+ bool success = i_weth.transfer(s_collector, collection);
+ require(success, "WETH transfer failed");
(bool collected,) = address(s_collector).call{value: address(this).balance}();
require(collected, "ETH transfer failed");
}
