Snowman Merkle Airdrop

[High] Unchecked Return Value from ERC20.transfer() in Snow.sol::collectFee function

Description

In the Snow.sol::collectFee function, an external ERC20 transfer() or transferFrom() function is called without checking its boolean return value. Standard ERC20 tokens (e.g., USDT) typically return false on failure (e.g., insufficient balance, allowance, or other internal errors) rather than reverting. If the transfer() or transferFrom() call fails silently, the contract will continue execution as if the transfer was successful, leading to an incorrect state.

Risk

Funds may not be successfully transferred to s_collector, and the function proceeds without errors, leading to loss of expected fees or mismanagement. This creates a discrepancy between the contract's perceived state and the actual state of token transfers.

Proof of Concept

To demonstrate this vulnerability, we will modify MockWETH.sol to simulate a failed transfer() that returns false (as some non-standard ERC20 tokens do), and then write a Foundry test that calls collectFee() under this condition.

1. Modify MockWETH.sol:
   Add the following to your MockWETH.sol contract:

   // In contracts/MockWETH.sol

   bool public shouldFailTransfer;

   ​

   function setShouldFailTransfer(bool _fail) external {

   shouldFailTransfer = _fail;

   }

   ​

   function transfer(address to, uint256 amount) public override returns (bool) {

   if (shouldFailTransfer) {

   return false; // Simulate a silent failure (e.g., insufficient allowance in a non-standard ERC20)

   }

   return super.transfer(to, amount);

   }

2. Add Test Case to TestSnow.t.sol:
   Create a new test function in your TestSnow.t.sol file:

   // In test/TestSnow.t.sol

   function testTransferFailsButNoRevertInCollectFee() public {

   // Setup: Jerry buys 1 Snow, depositing FEE in WETH to Snow contract

   vm.startPrank(jerry);

   weth.approve(address(snow), FEE);

   snow.buySnow(1);

   vm.stopPrank();

   ​

   // Ensure Snow contract holds the fee (FEE = 1e18 in MockWETH.sol constructor)

   assertEq(weth.balanceOf(address(snow)), FEE, "Snow contract should hold initial fee");

   assertEq(weth.balanceOf(collector), 0, "Collector should initially have 0 WETH");

   ​

   // Force MockWETH's transfer function to return false

   weth.setShouldFailTransfer(true);

   ​

   // Action: Collector tries to collect the fee. The call should not revert.

   vm.prank(collector);

   snow.collectFee();

   ​

   // Verification: The WETH should still be in the Snow contract, not transferred to the collector.

   // This demonstrates the silent failure where the contract proceeds as if successful.

   assertEq(weth.balanceOf(address(snow)), FEE, "WETH should remain trapped in Snow contract");

   assertEq(weth.balanceOf(collector), 0, "Collector's balance should still be 0");

   }

3. Reproduction Steps:

   • Ensure you have MockWETH.sol, Snow.sol, and TestSnow.t.sol in your Foundry project.

   • Deploy MockWETH and set its address as i_weth in the Snow constructor.

   • Run the test TestSnow.t.sol::testTransferFailsButNoRevertInCollectFee().

   • Expected Result: The collectFee() function should not revert.

   • Actual Result: The collectFee() function executes successfully, but the WETH remains in the Snow contract (weth.balanceOf(address(snow)) is still FEE) and is not transferred to the collector (weth.balanceOf(collector) is still 0), confirming the silent failure.

Recommended Mitigation

Always check the boolean return value of ERC20.transfer() and transferFrom() to ensure the operation was successful. The most robust and recommended approach is to use OpenZeppelin's SafeERC20 library, which automatically handles these checks by reverting on false returns or insufficient allowances.

Proposed Fix in Snow.sol: