SNARKeling Treasure Hunt

First Flight #59
Beginner FriendlyGameFiFoundry
100 EXP
View results
Submission Details
Impact: medium
Likelihood: low
Invalid

Missing nonReentrant modifier on emergencyWithdraw allowing reentrancy attacks

Root + Impact

Description

  • The normal behavior of emergencyWithdraw is to let the contract owner safely withdraw any amount of ETH to a chosen recipient address when the contract is paused (for emergency fund recovery).

  • The specific issue is that the function performs an external low-level ETH transfer (.call{value}) without the nonReentrant modifier that already exists in the contract (and is used on claim()). This violates the Checks-Effects-Interactions pattern and leaves the function exposed to reentrancy.

// Root cause in the codebase with @> marks to highlight the relevant section
/// @notice In case of an emergency, allow the owner to send ETH to a specified address.
@> function emergencyWithdraw(address payable recipient, uint256 amount) external { // missing nonReentrant modifier
require(paused, "THE_CONTRACT_MUST_BE_PAUSED");
require(msg.sender == owner, "ONLY_OWNER_CAN_EMERGENCY_WITHDRAW");
require(recipient != address(0) && recipient != address(this) && recipient != owner, "INVALID_RECIPIENT");
require(amount > 0 && amount <= address(this).balance, "INVALID_AMOUNT");
(bool sent, ) = recipient.call{value: amount}(""); // external call with no reentrancy protection
require(sent, "ETH_TRANSFER_FAILED");
emit EmergencyWithdraw(recipient, amount);
}

Risk

Likelihood:

  • Owner calls emergencyWithdraw while the contract is paused and passes a malicious contract as the recipient

  • Owner account is compromised or tricked into interacting with a malicious recipient (social engineering / phishing)

Impact:

  • Malicious recipient can re-enter emergencyWithdraw (or trigger other state-changing logic) before the first call completes

  • Funds can be drained beyond the intended amount (or cause unexpected behavior during an emergency)

Proof of Concept

// Malicious recipient contract (deployed by attacker)
contract MaliciousRecipient {
TreasureHunt public hunt;
uint256 public attackAmount;
constructor(address _hunt) {
hunt = TreasureHunt(_hunt);
}
// Owner (or compromised owner) will call this to start the attack
function attack(uint256 _amount) external {
attackAmount = _amount;
hunt.emergencyWithdraw(payable(address(this)), _amount);
}
receive() external payable {
// Re-enter the function while funds are still available
if (address(hunt).balance >= attackAmount) {
hunt.emergencyWithdraw(payable(address(this)), attackAmount);
}
}
}

Attack flow (for demo):

  1. Deploy MaliciousRecipient pointing to TreasureHunt.

  2. Pause the contract.

  3. Owner calls MaliciousRecipient.attack(amount).

  4. The fallback/receive re-enters emergencyWithdraw before the first transfer finishes.

Recommended Mitigation

/// @notice In case of an emergency, allow the owner to send ETH to a specified address.
- function emergencyWithdraw(address payable recipient, uint256 amount) external {
+ function emergencyWithdraw(address payable recipient, uint256 amount) external nonReentrant {
require(paused, "THE_CONTRACT_MUST_BE_PAUSED");
require(msg.sender == owner, "ONLY_OWNER_CAN_EMERGENCY_WITHDRAW");
require(recipient != address(0) && recipient != address(this) && recipient != owner, "INVALID_RECIPIENT");
require(amount > 0 && amount <= address(this).balance, "INVALID_AMOUNT");
(bool sent, ) = recipient.call{value: amount}("");
require(sent, "ETH_TRANSFER_FAILED");
emit EmergencyWithdraw(recipient, amount);
}

The contract already implements a clean nonReentrant modifier (and locked state variable) and applies it to claim(). Adding it here is a one-line, zero-risk fix. For consistency you should also consider adding it to the withdraw() function.

Updates

Lead Judging Commences

s3mvl4d Lead Judge 18 days ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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

Give us feedback!