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

[H-2] Not accounting for a possible score of 0 in `ThePredictor::withdraw` will lead to stuck funds

mayhem

Summary

The protocol makes the assumption that players will either have a positive or negative score when performing calculations in ThePredicter::withdraw. Specifically, this if check assigns a value to withdraw::reward :

reward = maxScore < 0
? entranceFee
: (shares * players.length * entranceFee) / totalShares;

The check assumes that maxScore will either be less than or greater than 0. In a situation where all players end up with a score of 0, withdraw will return a [FAIL. Reason: panic: division or modulo by zero (0x12)] error due to totalShares == 0.
Considering users should have their entranceFee returned if maxScore < 0, we can assume the intentions would be the same if maxScrore == 0. Not accounting for maxScore == 0 will result in funds being stuck in the contract.

Vulnerability Details

In order for a player to be eligible for withdraw, they must pass this check:

if (!scoreBoard.isEligibleForReward(msg.sender))

ScoreBoard::isEligibleForReward requires that a player make at least 1 prediction:

function isEligibleForReward(address player) public view returns (bool) {
return
results[NUM_MATCHES - 1] != Result.Pending &&
playersPredictions[player].predictionsCount > 1;
}

In Predicter::withdraw, a user's payout is determined by uint256 totalShares = uint256(totalPositivePoints);. The calculation is made in a for loop checking each players score

for (uint256 i = 0; i < players.length; ++i) {
int8 cScore = scoreBoard.getPlayerScore(players[i]);
if (cScore > maxScore) maxScore = cScore;
if (cScore > 0) totalPositivePoints += cScore;
}

ScoreBoard::getPlayerScore will either return 2 or -1:

score += playersPredictions[player].predictions[i] == results[i]
? int8(2)
: -1;

This allows for withdraw::totalPositivePoints to equal 0. In those situations, withdraw will return a panic error.

Proof of Concept (foundry test)

  1. A max number of participants enter the protocol

  2. Each participant predicts one match correctly and two incorrectly leading to a score of 0 for all players

  3. test_NormalWithdraw will attempt to withdraw but result in a panic error

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
​
import {Test, console} from "forge-std/Test.sol";
import {ThePredicter} from "../src/ThePredicter.sol";
import {ScoreBoard} from "../src/ScoreBoard.sol";
​
contract ThePredicterTest is Test {
ThePredicter public thePredicter;
ScoreBoard public scoreBoard;
address public organizer = makeAddr("organizer");
address public stranger = makeAddr("stranger");
​
uint256 public constant PICK_A_NUMBER = 30;
address[] players = new address[](PICK_A_NUMBER);
​
function setUp() public {
vm.startPrank(organizer);
scoreBoard = new ScoreBoard();
thePredicter = new ThePredicter(
address(scoreBoard),
0.04 ether,
0.0001 ether
);
​
scoreBoard.setThePredicter(address(thePredicter));
vm.stopPrank();
}
​
modifier participants() {
for (uint256 i =0; i < PICK_A_NUMBER; i++) {
string memory stringNumber = vm.toString(i);
players[i] = makeAddr(stringNumber);
vm.deal(players[i], 1 ether);
vm.startPrank(players[i]);
thePredicter.register{value: 0.04 ether}();
vm.stopPrank();
​
vm.startPrank(organizer);
thePredicter.approvePlayer(players[i]);
vm.stopPrank();
​
vm.startPrank(players[i]);
thePredicter.makePrediction{value: 0.0001 ether}(
1,
ScoreBoard.Result.First
);
thePredicter.makePrediction{value: 0.0001 ether}(
2,
ScoreBoard.Result.Second
);
thePredicter.makePrediction{value: 0.0001 ether}(
3,
ScoreBoard.Result.Second
);
}
​
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();
_;
}
​
function test_NormalWithdraw() public participants{
vm.startPrank(address(players[1]));
thePredicter.withdraw();
vm.stopPrank();
assertEq(players[1].balance, 0.9997 ether);
}
}

Impact

This is a high impact vulnerability. It can occur if no one gets any predictions correct or if the sum of predictions scores equals zero. The odds of this occurring will depend on how many players enter the protocol, how frequently the players will make predictions (as they are not obligated to predict every match), and the precision of their predictions. The vulnerability will lead to a loss of all funds except for the prediction fees.

Tools Used

Manual Review
Foundry

Recommendations

We need to adjust the logic in Predicter::withdraw
One fix would be to include 0 for maxScore:

+ reward = maxScrore <= 0
- reward = maxScore < 0
? entranceFee
: (shares * players.length * entranceFee) / totalShares;

Or account for totalShares being 0:

+ if (totalShares == 0) {
+ reward = entranceFee;
+ } else {
+ reward = maxScore < 0
+ ? entranceFee
+ : (shares * players.length * entranceFee) / totalShares;
+ }
- reward = maxScore < 0
- ? entranceFee
- : (shares * players.length * entranceFee) / totalShares;
Updates

Lead Judging Commences

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

Possible maxScore of zero is not accounted

The checks related to maxScore do not account possible maxScore of zero leading to stuck funds or a division by zero error.

Support

FAQs

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