First Flight #20: The Predicter

Summary

When the organizer calls ThePredicter::withdrawPredictionFees, the balance of the contract subtracted by all the players entrance fees is supposed to be returned. Which more or less is assumed to be the sum of the predictionFees. However, the way this is done now is by using:
address(this).balance - players.length * entranceFee

But players.length is only increased after a player is approved, not when a player is only registered. So if many players register but aren't approved, it will ruin the formula above, making the organizer withdraw a lot more Ether than intended.

One might think that this is not a problem because the organizer is the one getting extra money, and one might think this is not a problem because the organizer can just approve all the extra players that register. However, as soon as the number of players reach 30, the organizer cannot approve more of them, but nothing is preventing more users from registering using ThePredicter::register. And while the organizer is the one getting extra funds, it ruins the functionality of the contract so the players who want to withdraw their winnings later don't have anything to withdraw anymore.

Vulnerability Details

Example (PoC):
Assume that the entranceFee = 1 ether and predictionFee = 0.1 ether

1. ThePredicter gets 30 players who register
   Contract balance now = 30 ether

2. The 30 players gets approved and make 60 predictions for 0.1 ether each
   Contract balance now = 30 ether + 0.1 * 60 ether = 36 ether

3. 20 new malicious users register so the contract gains 20 * entranceFee, and the organizer cannot approve these players.
   Contract balance now = 36 ether + 20 ether = 56 ether

4. If the organizer now calls ThePredicter::withdrawPredictionFees, it will withdraw using the formula:
   address(this).balance - players.length * entranceFee
   Which will result in a withdrawal of:
   56 ether - 30 ether = 26 ether

   Contract balance now = 30 ether

5. If the 20 malicious users now call ThePredicter:cancelRegistration to get back their entranceFee, the contract balance will not have enough to pay the winners later. The balance of the contract will be:
   30 - 20 ether = 10 ether

Since the reward pool for predictions is taken from entranceFees for the players, the reward pool should be more than 30 ether since the number of players making predictions are 30 players.

Impact

Medium.
Might require a lot of funds to pull off, but there is basically no risk since the entranceFee can be paid back again.

Tools Used

Code inspection

Recommendations

Have real bookkeeping of how many predictionFees are added by adding some storage variable that keeps track of it, instead of relying on balance that's unpredictable.