[H-4] User Overcharged in buySnow When Sending ETH Incorrectly; collectFee Sweeps Overcharged Funds

Description

• Intended Behavior: The Snow.sol contract allows users to buy Snow tokens. Payment can be made either entirely in native ETH (if msg.value exactly matches the total fee) or entirely in WETH (if msg.value is not the exact ETH fee, implying a WETH payment). The collectFee function allows a designated collector to withdraw accumulated ETH and WETH fees from the contract.

• Specific Vulnerability: The buySnow function has a logical flaw in its payment processing. If a user sends some native ETH (i.e., msg.value > 0) but this amount does not exactly match the total fee calculated as (s_buyFee * amount), the contract enters an else block. In this block:

  1. The native ETH sent by the user (the msg.value) remains with the Snow.sol contract.

  2. The contract then proceeds to execute i_weth.safeTransferFrom(msg.sender, address(this), (s_buyFee * amount)), transferring the full fee amount in WETH from the user.
     This results in the user being charged twice: once with the native ETH they sent, and again with the full WETH fee for the same Snow tokens. The collectFee function subsequently allows the collector to withdraw these overcharged funds (both the erroneously kept ETH and the WETH).

  // Snow.sol

  function buySnow(uint256 amount) external payable canFarmSnow {

  if (msg.value == (s_buyFee * amount)) { // Path A: Correct ETH payment

  _mint(msg.sender, amount);

  } else { // Path B: WETH payment OR incorrect a greater amount

  // If msg.value > 0 here, that ETH is kept by the contract.

  // Then, the full WETH fee is also taken. -> OVERCHARGE

  i_weth.safeTransferFrom(msg.sender, address(this), (s_buyFee * amount));

  _mint(msg.sender, amount);

  }

  // ...

  }

  ​

  function collectFee() external onlyCollector {

  uint256 collection = i_weth.balanceOf(address(this)); // Includes WETH from overcharge

  i_weth.transfer(s_collector, collection);

  ​

  // Includes native ETH from overcharge

  (bool collected,) = payable(s_collector).call{value: address(this).balance}("");

  require(collected, "Fee collection failed!!!");

  }

Risk

Likelihood: Medium

• A user might mistakenly send some ETH while intending a WETH payment, or send an incorrect ETH amount, triggering the overcharge. This depends on user interface and user understanding.

Impact: High

• Direct Loss of User Funds: Users are charged more than the intended fee, losing both the incorrectly sent ETH and the full WETH fee.

• Unjust Enrichment of Collector: The collectFee function transfers these overcharged funds to the collector.

• Damage to Trust: Users who are overcharged will lose trust in the protocol.

Proof of Concept

The testBuySnow_UserOverchargeAndCollectorWithdrawal function in test/Snow.t.sol demonstrates this vulnerability.
(The constructor _buyFee argument is set to 1, which due to s_buyFee = _buyFee * PRECISION makes the actual s_buyFee per Snow token equal to 1 WETH).

1. Setup: Alice has ETH and WETH. The Snow contract (instance named snow) is deployed with _buyFee = 1 (so 1 Snow token costs 1 WETH).

2. Overcharge Transaction: Alice calls snow.buySnow{value: 0.1 ether}(1). She intends to buy 1 Snow token.

   • Since 0.1 ether is not equal to the total fee in ETH (which would be 1 ETH if s_buyFee is 1 WETH), the else block in buySnow is triggered.

   • The Snow contract receives and keeps the 0.1 ether.

   • The Snow contract then transfers 1 WETH (the s_buyFee * 1) from Alice.

   • Alice effectively pays 0.1 ETH + 1 WETH for 1 Snow token.

3. Fee Collection: The collector calls snow.collectFee().

   • The 0.1 ETH and 1 WETH (which includes the overcharged components) held by the Snow contract are transferred to the collector.

The test logs and assertions confirm that Alice is charged both the ETH she sent and the full WETH fee, and these funds are subsequently withdrawable by the collector.

Recommended Mitigation

Modify the buySnow function to handle payments more robustly and prevent overcharging:

1. Clear Payment Paths: Enforce clearer conditions for ETH vs. WETH payments.

   • If msg.value > 0, it should be treated as an attempt for a full ETH payment. If msg.value is not the exact amount, the transaction should revert.

   • If msg.value == 0, it should be treated as a WETH payment, and only WETH should be transferred.

   function buySnow(uint256 amount) external payable canFarmSnow {

   - if (msg.value == (s_buyFee * amount)) {

   + if (msg.value > 0) { // User intends to pay with ETH

   + if (msg.value != (s_buyFee * amount)) {

   + revert S__IncorrectPaymentAmount(); // Define new error

   + }

   _mint(msg.sender, amount);

   - } else {

   + } else { // User intends to pay with WETH (msg.value must be 0)

   i_weth.safeTransferFrom(msg.sender, address(this), (s_buyFee * amount));

   _mint(msg.sender, amount);

   }

   ​

   s_earnTimer = block.timestamp;

   emit SnowBought(msg.sender, amount);

   }

2. Clarify _buyFee and s_buyFee: Review the s_buyFee = _buyFee * PRECISION; logic. If _buyFee is intended to be the price in wei, the multiplication by PRECISION is incorrect and will lead to an extremely inflated s_buyFee. s_buyFee should accurately reflect the intended price per Snow token in wei. For example, if _buyFee from the constructor represents the fee in wei, then it should be s_buyFee = _buyFee;.

By implementing these changes, users will not be accidentally overcharged, and the collectFee function will only collect legitimately earned fees.