Description
An external call (safeTransferFrom) is made before updating the internal state (s_hasClaimedSnowman), which is generally discouraged. While the contract uses OpenZeppelin's nonReentrant modifier — which effectively prevents reentrancy in the current implementation — **it is **recommended to update critical state before performing any external calls.
function claimSnowman(address receiver, bytes32[] calldata merkleProof, uint8 v, bytes32 r, bytes32 s)
external
nonReentrant
{
if (receiver == address(0)) {
revert SA__ZeroAddress();
}
if (i_snow.balanceOf(receiver) == 0) {
revert SA__ZeroAmount();
}
if (!_isValidSignature(receiver, getMessageHash(receiver), v, r, s)) {
revert SA__InvalidSignature();
}
uint256 amount = i_snow.balanceOf(receiver);
bytes32 leaf = keccak256(bytes.concat(keccak256(abi.encode(receiver, amount))));
if (!MerkleProof.verify(merkleProof, i_merkleRoot, leaf)) {
revert SA__InvalidProof();
}
@> i_snow.safeTransferFrom(receiver, address(this), amount);
@> s_hasClaimedSnowman[receiver] = true;
emit SnowmanClaimedSuccessfully(receiver, amount);
i_snowman.mintSnowman(receiver, amount);
}
Risk
Likelihood:
-
This issue will occur when the nonReentrant modifier is removed (e.g., during refactoring or for testing) and a malicious ERC20 token is used that reenters claimSnowman via the transferFrom call.
-
It can also occur during future upgrades, forks, or contract reuse where developers unintentionally remove or bypass the reentrancy protection, or if they rely solely on the OpenZeppelin SafeERC20 wrapper assuming it's safe against all behaviors (which it is not when used with malicious tokens).
Impact:
-
A malicious actor could claim the Snowman NFT multiple times in a single transaction, bypassing the one-time claim restriction and minting more NFTs than allowed.
-
This would lead to a broken distribution model, economic imbalance in the system (over-minting), and a potential loss of trust from users who cannot claim their rightful NFTs due to exhausted supply or unfair advantage.
Proof of Concept
contract MaliciousSnow is ERC20 {
address public airdrop;
bool public reentered;
constructor(address _airdrop) ERC20("Malicious", "MAL") {
airdrop = _airdrop;
_mint(msg.sender, 100 ether);
}
function transferFrom(address from, address to, uint256 amount) public override returns (bool) {
super._transfer(from, to, amount);
if (!reentered) {
reentered = true;
ISnowmanAirdrop(airdrop).claimSnowman(
from,
validMerkleProof,
v, r, s
);
}
return true;
}
}
function test_ReentrancyClaim() public {
vm.prank(attacker);
snow.approve(airdrop.address, amount);
airdrop.claimSnowman(attacker, proof, v, r, s);
assertEq(snowman.balanceOf(attacker), expectedDoubleMint);
}
Recommended Mitigation
Move the state update before the external call, ensuring that even if reentrancy were somehow possible in the future, it wouldn’t result in duplicate claims
function claimSnowman(address receiver, bytes32[] calldata merkleProof, uint8 v, bytes32 r, bytes32 s)
external
nonReentrant
{
if (receiver == address(0)) {
revert SA__ZeroAddress();
}
if (i_snow.balanceOf(receiver) == 0) {
revert SA__ZeroAmount();
}
if (!_isValidSignature(receiver, getMessageHash(receiver), v, r, s)) {
revert SA__InvalidSignature();
}
uint256 amount = i_snow.balanceOf(receiver);
bytes32 leaf = keccak256(bytes.concat(keccak256(abi.encode(receiver, amount))));
if (!MerkleProof.verify(merkleProof, i_merkleRoot, leaf)) {
revert SA__InvalidProof();
}
- i_snow.safeTransferFrom(receiver, address(this), amount); // send tokens to contract... akin to burning
-
- s_hasClaimedSnowman[receiver] = true;
// Mitigation: Mark as claimed *before* external call
+ s_hasClaimedSnowman[receiver] = true;
+ i_snow.safeTransferFrom(receiver, address(this), amount);
emit SnowmanClaimedSuccessfully(receiver, amount);
i_snowman.mintSnowman(receiver, amount);
}
