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

`ThePredicter::withdrawPredictionFees` Reverts if Players Withdraw 50% of Contract Funds, Preventing the `Organizer` from Claiming Fees

EyanIngles

Summary

Once all matches have had their results set, players with a positive score can claim their rewards through the ThePredicter::withdraw function. If players withdraw over 50% of the contract's funds before the organizer can call withdrawPredictionFee, the organizer is unable to claim the required fees due to an arithmetic underflow or overflow revert. This scenario has a higher probability because players are eager to get their rewards, while the organizer might be relaxed, thinking the funds are secure and cannot be taken from them.

Vulnerability Details

Please use and refer to the PoC below that is to be used in ThePredicter.test.solfile.

  1. We have 2 players called player1 and player2, they both register, have their address approved and submit predictions by calling ThePredictor::makePredictionfunction and the organizer then setResult.

  2. player 1 gets a total points of 11 and player 2 get 10 points in total, player1 gains a majority of 52.4% of the winnings.

  3. player1 claims their rewards by calling ThePredicter::withdraw, player2 does not claim their rewards.

  4. The organizer attempts to claim the required fees by calling withdrawPredictionFee but encounters a revert error due to arithmetic underflow or overflow.

This PoC demonstrates how the vulnerability occurs when players withdraw a significant portion of the contract's funds, preventing the organizer from claiming their fees.

function testFailOragnizerUnableToClaimFees() public {
address player1 = makeAddr("player1");
address player2 = makeAddr("player2");
vm.startPrank(player1);
vm.deal(player1, 1 ether);
thePredicter.register{value: 0.04 ether}();
vm.stopPrank();
​
vm.startPrank(player2);
vm.deal(player2, 1 ether);
thePredicter.register{value: 0.04 ether}();
vm.stopPrank();
​
vm.startPrank(organizer);
thePredicter.approvePlayer(player2);
vm.stopPrank();
​
vm.startPrank(player1);
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);
thePredicter.makePrediction{value: 0.0001 ether}(3, ScoreBoard.Result.First);
thePredicter.makePrediction{value: 0.0001 ether}(4, ScoreBoard.Result.First);
thePredicter.makePrediction{value: 0.0001 ether}(5, ScoreBoard.Result.First);
thePredicter.makePrediction{value: 0.0001 ether}(6, ScoreBoard.Result.Draw);
vm.stopPrank();
​
vm.startPrank(player2);
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);
thePredicter.makePrediction{value: 0.0001 ether}(3, ScoreBoard.Result.First);
thePredicter.makePrediction{value: 0.0001 ether}(4, ScoreBoard.Result.First);
vm.stopPrank();
​
vm.startPrank(organizer);
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();
// player 1 will now withdraw their rewards.
vm.startPrank(player1);
thePredicter.withdraw();
vm.stopPrank();
// organiser will now try to withdraw prediction fees.
vm.startPrank(organizer);
thePredicter.withdrawPredictionFees();
}

Impact

causing potential financial losses and disrupting the intended fee collection process. This scenario is likely because players are motivated to withdraw their rewards promptly, potentially leaving the organizer with no access to the funds needed for operational expenses. This causes a high potential financial losses and disrupting the intended fee collection process. This scenario is likely because players are motivated to withdraw their rewards promptly, potentially leaving the organizer with no access to the funds needed for operational expenses.

Tools Used

Manual Review

Recommendations

Recommendations to prevent this bug would be to store a boolean value of if the organizer has claimed the fee's or not, then have a check at ThePredicter::withdraw before any player can withdraw their rewards.

Add a new storage with a boolvalue, pass it through the constructor function and set it to false.

contract ThePredicter {
using Address for address payable;
+bool public playersCanWithdraw;
constructor(address _scoreBoard, uint256 _entranceFee, uint256 _predictionFee) {
organizer = msg.sender; //@explain: makes the msg.sender the owner(organiser)
scoreBoard = ScoreBoard(_scoreBoard); //@explain: gets ScoreBoard contract address.
entranceFee = _entranceFee; //@explain: sets the entrance fee amount.
predictionFee = _predictionFee; //@explain: sets the prediction fee amount.
+ playersCanWithdraw = false;
}

ThePredicter::withdrawPredictionFeeshave the boolean value changed to true once the organizer has withdrawn their fees at the bottom to follow CEI

function withdrawPredictionFees() public
+returns (bool success)
{
if (msg.sender != organizer) {
revert ThePredicter__NotEligibleForWithdraw();
}
​
uint256 fees = address(this).balance - players.length * entranceFee; //@explain: sets a new variable for the fee amount to pay for the hall rent.
(bool success,) = msg.sender.call{value: fees}(""); //@explain: sends the fee amount to the organiser.
require(success, "Failed to withdraw");
+ success = playersCanWithdraw;
}

lastly to have a check on the ThePredicter::withdraw function.

function withdraw() public {
+ require(playersCanWithdraw == true);
if (!scoreBoard.isEligibleForReward(msg.sender)) {
revert ThePredicter__NotEligibleForWithdraw();
}

Updates

Lead Judging Commences

NightHawK Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Wrong computation in withdrawPredictionFees

withdrawPredictionFees incorrectly computes the value to be transferred to the organizer, which leads to pending players not being able to cancel their registration, approved players not being able to claim their rewards and other errors.

Support

FAQs

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