Root + Impact
Description
-
Describe the normal behavior in one or more sentences
The `buySnow()` function has flawed conditional logic that can result in loss of ETH, improper payment handling, and allows users to pay with both ETH and WETH simultaneously without proper validation.
-
Explain the specific issue or problem in one or more sentence
The function uses an `if-else` structure that doesn't properly handle edge cases. If `msg.value` doesn't exactly equal the required fee, it attempts to transfer WETH, even if ETH was sent. Additionally, excess ETH is not refunded.
```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);
}
```
**Issues:**
1. If user sends ETH but `msg.value != (s_buyFee * amount)`, the function still tries to transfer WETH and keeps the ETH
2. If user sends excess ETH (`msg.value > (s_buyFee * amount)`), excess is not refunded
3. If user sends both ETH and approves WETH, both payments could be taken
4. No validation that payment was actually received
Risk
Likelihood:
-
* Users may accidentally send incorrect amounts of ETH
* Front-end or integration bugs could send wrong values
* Users might send ETH when they intended to pay with WETH or vice versa
* Occurs whenever `msg.value` doesn't exactly match the required fee
Impact:
-
* Permanent loss of user funds (ETH sent but not used)
* Users paying twice (both ETH and WETH)
* Contract accumulating ETH that should have been refunded
* Poor user experience and potential financial losses
Proof of Concept
```solidity
function testBuySnowEthLoss() public {
Snow snow = deploySnow();
address user = makeAddr("user");
uint256 fee = snow.s_buyFee();
vm.deal(user, fee * 2);
vm.prank(user);
snow.buySnow{value: fee * 2}(1);
assert(snow.balanceOf(user) == 1);
assert(address(snow).balance == fee * 2);
vm.deal(user, fee / 2);
weth.mint(user, fee);
vm.startPrank(user);
weth.approve(address(snow), fee);
snow.buySnow{value: fee / 2}(1);
vm.stopPrank();
assert(address(snow).balance > 0);
assert(weth.balanceOf(address(snow)) == fee);
}
```
Recommended Mitigation
```diff
// Snow.sol
function buySnow(uint256 amount) external payable canFarmSnow {
uint256 requiredFee = s_buyFee * amount;
+ if (msg.value > 0) {
+ // Paying with ETH
+ if (msg.value != requiredFee) {
+ revert S__InvalidPaymentAmount();
+ }
+ _mint(msg.sender, amount);
+ } else {
+ // Paying with WETH
+ i_weth.safeTransferFrom(msg.sender, address(this), requiredFee);
+ _mint(msg.sender, amount);
+ }
- 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);
}
```
