Root + Impact
Description
-
A user buying Snow with native ETH is expected to pay exactly s_buyFee * amount and receive their tokens.
-
When msg.value is not exactly the price, execution silently falls into the else branch and pulls the full price in WETH, while the ETH already sent stays trapped in the contract (only collectFee can sweep it, to the collector).
```solidity
function buySnow(uint256 amount) external payable canFarmSnow {
if (msg.value == (s_buyFee * amount)) {
_mint(msg.sender, amount);
} else {
@> i_weth.safeTransferFrom(msg.sender, address(this), (s_buyFee * amount));
_mint(msg.sender, amount);
}
@>
s_earnTimer = block.timestamp;
emit SnowBought(msg.sender, amount);
}
```
Risk
Likelihood:
-
Occurs whenever a user pays in ETH with any value other than the exact price (rounding, fee change, fat-finger), which is normal user behavior.
-
Occurs for any user who approves WETH and accidentally attaches ETH to the call.
Impact:
-
The user is charged twice: full WETH price plus the ETH they sent, which is lost to them.
-
Direct, silent loss of user funds on any imprecise ETH payment.
Proof of Concept
Self-contained Foundry test. Run: `forge test --match-test test_H2_BuySnowOverchargesEthPlusWeth -vvv`
```solidity
pragma solidity ^0.8.24;
import {Test} from "forge-std/Test.sol";
import {Snow} from "../src/Snow.sol";
import {MockWETH} from "../src/mock/MockWETH.sol";
contract H2 is Test {
Snow snow;
MockWETH weth;
address alice = makeAddr("alice");
function setUp() public {
weth = new MockWETH();
snow = new Snow(address(weth), 1, makeAddr("collector"));
}
function test_H2_BuySnowOverchargesEthPlusWeth() public {
uint256 amount = 2;
uint256 cost = 1e18 * amount;
weth.mint(alice, cost);
vm.deal(alice, 1 ether);
vm.startPrank(alice);
weth.approve(address(snow), cost);
snow.buySnow{value: 0.5 ether}(amount);
vm.stopPrank();
assertEq(snow.balanceOf(alice), amount);
assertEq(weth.balanceOf(alice), 0);
assertEq(address(snow).balance, 0.5 ether);
}
}
```
**What it proves:** alice pays the full `2e18` WETH price and additionally loses the `0.5 ETH` she attached — it remains locked in the contract (recoverable only by the collector via `collectFee`). She is charged twice for one purchase.
Recommended Mitigation
Make the payment path explicit: exact ETH or WETH, never a silent fallback that keeps both.
```diff
function buySnow(uint256 amount) external payable canFarmSnow {
- if (msg.value == (s_buyFee * amount)) {
- _mint(msg.sender, amount);
- } else {
- i_weth.safeTransferFrom(msg.sender, address(this), (s_buyFee * amount));
- _mint(msg.sender, amount);
- }
+ uint256 cost = s_buyFee * amount;
+ if (msg.value > 0) {
+ // paying in ETH: must be exact, otherwise revert (no silent WETH charge)
+ require(msg.value == cost, "Snow: incorrect ETH amount");
+ } else {
+ // paying in WETH
+ i_weth.safeTransferFrom(msg.sender, address(this), cost);
+ }
+ _mint(msg.sender, amount);
s_earnTimer = block.timestamp;
emit SnowBought(msg.sender, amount);
}
```
**Why this fixes it:** if any ETH is sent it must equal the price (else the tx reverts and nothing is lost); WETH is only pulled when no ETH is sent. A user can never be charged in both assets.
