Snowman Merkle Airdrop
First Flight #42
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Impact: high
Likelihood: medium
Invalid

Improper Handling of transfer Return Value in Snow.collectFee function

i_am_batman

Root + Impact

Description

  • In Snow.collectFee function despite you use SafeERC20, the unchecked transfer vulnerability arises

    due to use of normal .transfer instead of .safeTransfer in the collectFee function.

function collectFee() external onlyCollector {
uint256 collection = i_weth.balanceOf(address(this));
i_weth.safeTransfer(s_collector, collection);
​
(bool collected,) = payable(s_collector).call{value: address(this).balance}("");
require(collected, "Fee collection failed!!!");
}

Risk: High

Likelihood: Medium

  • Assumption of Compliance: Developers often assume all ERC721/ERC20 tokens follow the standard strictly, ignoring tokens with non-standard behavior (e.g., non-reverting or reverting silently).

  • Unchecked Return: Most developers don’t manually check return values or receiver behavior when using safeTransferFrom, leading to overlooked edge cases.

Impact:

  • Token Loss or Lock: If the receiving contract doesn't implement onERC721Received correctly, the token can be permanently stuck or lost.

  • Unexpected Reverts: Contracts relying on post-transfer logic may be bricked if safeTransferFrom fails silently or reverts unexpectedly, breaking flows like airdrops, staking, or auctions.

Proof of Concept

Firstly write a test to check the collectFee function properly.
Add this test:

​
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.24;
​
import "forge-std/Test.sol";
import "../src/Snow.sol";
import "../lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
​
// Mock ERC20 that always fails on transfer
contract FailingToken is ERC20 {
constructor() ERC20("FailToken", "FAIL") {
_mint(msg.sender, 1000 ether);
}
​
function transfer(address, uint256) public pure override returns (bool) {
return false; // always fail
}
}
​
// Standard Mock ERC20
contract MockERC20 is ERC20 {
constructor(string memory name, string memory symbol) ERC20(name, symbol) {
_mint(msg.sender, 1_000_000 ether);
}
​
function mint(address to, uint256 amount) external {
_mint(to, amount);
}
}
​
contract SnowTest is Test {
Snow snow;
MockERC20 weth;
address collector = address(1);
address user = address(2);
​
function setUp() public {
weth = new MockERC20("Wrapped ETH", "WETH");
snow = new Snow(address(weth), 1, collector);
​
weth.mint(address(snow), 10 ether);
}
​
function testCollectFeeSucceeds() public {
vm.prank(collector);
snow.collectFee();
​
// Confirm WETH was transferred to collector
assertEq(weth.balanceOf(collector), 10 ether);
}
​
function testCollectFeeFailsOnBadERC20() public {
FailingToken badToken = new FailingToken();
Snow badSnow = new Snow(address(badToken), 1, collector);
​
// Transfer some failing tokens to the Snow contract
badToken.transfer(address(badSnow), 10 ether);
​
vm.prank(collector);
vm.expectRevert(); // expect revert due to failed transfer
badSnow.collectFee();
}
}
​
//You will see that the test testCollectFeeFailsOnBadERC20() will fail because it does not detects the
//transfer and you will get output such as given below :
​
//Output of the test before applying the fix:
$ forge build
[⠒] Compiling...
[⠒] Compiling 34 files with Solc 0.8.28
[⠢] Solc 0.8.28 finished in 22.20s
Compiler run successful!
HP@GOAT MINGW64 ~/Desktop/2025-06-snowman-merkle-airdrop
$ forge test --match-path test/TestingSnow.t.sol -vvvv
[⠢] Compiling...
No files changed, compilation skipped
​
Ran 2 tests for test/TestingSnow.t.sol:SnowTest
[FAIL: next call did not revert as expected] testCollectFeeFailsOnBadERC20() (gas: 2365956)
Traces:
[2444123] SnowTest::setUp()
├─ [795108] → new MockERC20@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: SnowTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], value: 1000000000000000000000000 [1e24])
│ └─ ← [Return] 3490 bytes of code
├─ [1519878] → new Snow@0x2e234DAe75C793f67A35089C9d99245E1C58470b
│ ├─ emit OwnershipTransferred(previousOwner: 0x0000000000000000000000000000000000000000, newOwner: SnowTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ └─ ← [Return] 6886 bytes of code
├─ [28097] MockERC20::mint(Snow: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 10000000000000000000 [1e19])
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: Snow: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], value: 10000000000000000000 [1e19])
│ └─ ← [Return]
└─ ← [Return]
​
[2365956] SnowTest::testCollectFeeFailsOnBadERC20()
├─ [754818] → new FailingToken@0xF62849F9A0B5Bf2913b396098F7c7019b51A820a
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: SnowTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], value: 1000000000000000000000 [1e21])
│ └─ ← [Return] 3292 bytes of code
├─ [1519878] → new Snow@0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9
│ ├─ emit OwnershipTransferred(previousOwner: 0x0000000000000000000000000000000000000000, newOwner: SnowTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ └─ ← [Return] 6886 bytes of code
├─ [881] FailingToken::transfer(Snow: [0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9], 10000000000000000000 [1e19]) [staticcall]
│ └─ ← [Return] false
├─ [0] VM::prank(ECRecover: [0x0000000000000000000000000000000000000001])
│ └─ ← [Return]
├─ [0] VM::expectRevert(custom error 0xf4844814)
│ └─ ← [Return]
├─ [12518] Snow::collectFee()
│ ├─ [3218] FailingToken::balanceOf(Snow: [0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9]) [staticcall]
│ │ └─ ← [Return] 0
│ ├─ [881] FailingToken::transfer(ECRecover: [0x0000000000000000000000000000000000000001], 0)
│ │ └─ ← [Return] false
│ ├─ [3000] ECRecover::fallback()
│ │ └─ ← [Return]
│ └─ ← [Return]
└─ ← [Revert] next call did not revert as expected
​
[PASS] testCollectFeeSucceeds() (gas: 63915)
Traces:
[68715] SnowTest::testCollectFeeSucceeds()
├─ [0] VM::prank(ECRecover: [0x0000000000000000000000000000000000000001])
│ └─ ← [Return]
├─ [50138] Snow::collectFee()
│ ├─ [3240] MockERC20::balanceOf(Snow: [0x2e234DAe75C793f67A35089C9d99245E1C58470b]) [staticcall]
│ │ └─ ← [Return] 10000000000000000000 [1e19]
│ ├─ [31979] MockERC20::transfer(ECRecover: [0x0000000000000000000000000000000000000001], 10000000000000000000 [1e19])
│ │ ├─ emit Transfer(from: Snow: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], to: ECRecover: [0x0000000000000000000000000000000000000001], value: 10000000000000000000 [1e19])
│ │ └─ ← [Return] true
│ ├─ [3000] ECRecover::fallback()
│ │ └─ ← [Return]
│ └─ ← [Return]
├─ [1240] MockERC20::balanceOf(ECRecover: [0x0000000000000000000000000000000000000001]) [staticcall]
│ └─ ← [Return] 10000000000000000000 [1e19]
├─ [0] VM::assertEq(10000000000000000000 [1e19], 10000000000000000000 [1e19]) [staticcall]
│ └─ ← [Return]
└─ ← [Return]
​
Suite result: FAILED. 1 passed; 1 failed; 0 skipped; finished in 2.95ms (1.63ms CPU time)
​
Ran 1 test suite in 1.68s (2.95ms CPU time): 1 tests passed, 1 failed, 0 skipped (2 total tests)
​
Failing tests:
Encountered 1 failing test in test/TestingSnow.t.sol:SnowTest
[FAIL: next call did not revert as expected] testCollectFeeFailsOnBadERC20() (gas: 2365956)
​
Encountered a total of 1 failing tests, 1 tests succeeded
​
//This is because transfer function can't detect the failed transfer
​
//Now, if we apply the fix by using .safeTransfer instead of .transfer in the collectFee function of
//Snow contract and run the test again we will know the issue.
​
//Output of the test after applying the fix:
​
$ forge test --match-path test/TestingSnow.t.sol -vvvv
[⠒] Compiling...
[⠒] Compiling 3 files with Solc 0.8.28
[⠑] Solc 0.8.28 finished in 5.49s
Compiler run successful!
​
Ran 2 tests for test/TestingSnow.t.sol:SnowTest
[PASS] testCollectFeeFailsOnBadERC20() (gas: 2342661)
Traces:
[2342661] SnowTest::testCollectFeeFailsOnBadERC20()
├─ [754818] → new FailingToken@0xF62849F9A0B5Bf2913b396098F7c7019b51A820a
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: SnowTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], value: 1000000000000000000000 [1e21])
│ └─ ← [Return] 3292 bytes of code
├─ [1500257] → new Snow@0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9
│ ├─ emit OwnershipTransferred(previousOwner: 0x0000000000000000000000000000000000000000, newOwner: SnowTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ └─ ← [Return] 6788 bytes of code
├─ [881] FailingToken::transfer(Snow: [0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9], 10000000000000000000 [1e19]) [staticcall]
│ └─ ← [Return] false
├─ [0] VM::prank(ECRecover: [0x0000000000000000000000000000000000000001])
│ └─ ← [Return]
├─ [0] VM::expectRevert(custom error 0xf4844814)
│ └─ ← [Return]
├─ [8629] Snow::collectFee()
│ ├─ [3218] FailingToken::balanceOf(Snow: [0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9]) [staticcall]
│ │ └─ ← [Return] 0
│ ├─ [881] FailingToken::transfer(ECRecover: [0x0000000000000000000000000000000000000001], 0)
│ │ └─ ← [Return] false
│ └─ ← [Revert] SafeERC20FailedOperation(0xF62849F9A0B5Bf2913b396098F7c7019b51A820a)
└─ ← [Return]
​
[PASS] testCollectFeeSucceeds() (gas: 64087)
Traces:
[68887] SnowTest::testCollectFeeSucceeds()
├─ [0] VM::prank(ECRecover: [0x0000000000000000000000000000000000000001])
│ └─ ← [Return]
├─ [50310] Snow::collectFee()
│ ├─ [3240] MockERC20::balanceOf(Snow: [0x2e234DAe75C793f67A35089C9d99245E1C58470b]) [staticcall]
│ │ └─ ← [Return] 10000000000000000000 [1e19]
│ ├─ [31979] MockERC20::transfer(ECRecover: [0x0000000000000000000000000000000000000001], 10000000000000000000 [1e19])
│ │ ├─ emit Transfer(from: Snow: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], to: ECRecover: [0x0000000000000000000000000000000000000001], value: 10000000000000000000 [1e19])
│ │ └─ ← [Return] true
│ ├─ [3000] ECRecover::fallback()
│ │ └─ ← [Return]
│ └─ ← [Return]
├─ [1240] MockERC20::balanceOf(ECRecover: [0x0000000000000000000000000000000000000001]) [staticcall]
│ └─ ← [Return] 10000000000000000000 [1e19]
├─ [0] VM::assertEq(10000000000000000000 [1e19], 10000000000000000000 [1e19]) [staticcall]
│ └─ ← [Return]
└─ ← [Return]
​
Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 2.28ms (1.11ms CPU time)
​
Ran 1 test suite in 1.90s (2.28ms CPU time): 2 tests passed, 0 failed, 0 skipped (2 total tests)
​
//Now the test passes,it is because .transfer function is unable to check the failed transfer and
//vm.expectRevert() fails. In contrast to .transfer when we use .safeTranfer the vm.expectRevert() passes
//correctly because .safeTransfer is able to check the failed transfer confirming the improper use of
//transfer vulnerability
​
​
​

Recommended Mitigation

To prevent unsafe transfers from failing silently or breaking core logic:

Add a wrapper around safeTransferFrom that catches potential failures using try/catch and verifies the recipient is capable of receiving NFTs:
function safeTransferWithCheck(````IERC721 token,````address from,````address to,````uint256 tokenId````) internal {````if (to.code.length > 0) {````try token.safeTransferFrom(from, to, tokenId) {````// success````} catch {````revert("Transfer failed: recipient does not implement IERC721Receiver");````}````} else {````token.safeTransferFrom(from, to, tokenId);````}````}

This ensures that the transfer either succeeds or fails with a clear reason, avoiding accidental loss or logic halts due to incompatible contracts.

- remove this code
i_weth.transfer(s_collector, collection);
+ add this code
i_weth.safeTransfer(s_collector, collection);
Updates

Lead Judging Commences

yeahchibyke Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Appeal created

i_am_batman Submitter
5 months ago
yeahchibyke Lead Judge
5 months ago
yeahchibyke Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.