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

makePrediction() should have an onlyPlayer modifier and setPrediction() should be internal

0xziin

Summary

makePrediction() from ThePredicter.sol contract should have an onlyPlayer modifier allowing only an authorized player to make a prediction.

https://github.com/Cyfrin/2024-07-the-predicter/blob/main/src/ThePredicter.sol#L85-L99

makePrediction() calls setPrediction() but setPrediction() is a public function but should be internal otherwise what's the point of using makePrediction() which is payable and need to pay a fee to make a prediction while setPrediction() is accessible to simple users without paying any fee.

Vulnerability Details

A simple user (not a Player, meaning a non authorized user) is able to make a prediction using makePrediction(). But more than that, a user can overpass the fee he normally has to pay for a prediction, by using directly the setPrediction() function which is accessible (public function).

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);
}
function setPrediction(
address player,
uint256 matchNumber,
Result result
) public {
if (block.timestamp <= START_TIME + matchNumber * 68400 - 68400)
playersPredictions[player].predictions[matchNumber] = result;
playersPredictions[player].predictionsCount = 0;
for (uint256 i = 0; i < NUM_MATCHES; ++i) {
if (
playersPredictions[player].predictions[i] != Result.Pending &&
playersPredictions[player].isPaid[i]
) ++playersPredictions[player].predictionsCount;
}
}

Impact

A non authorized user can bypass the "Player restrictions" AND make a prediction at no cost (no prediction fee payed) !
It is a pretty huge vulnerability. More than bypassing the rules, it also breaks the economy of the prediction game.

Tools Used

VisualCode.

Recommendations

1) Should add an onlyPlayer modifier in makePrediction() function

modifier onlyPlayer() {
bool present;
//Check if the user is present in the list of authorized players
for(uint256 i; i < players.length; i++) {
if (msg.sender == players[i]) {
present = true;
}
}
//If not -> revert
if (!present) {
revert ScoreBoard__UnauthorizedAccess();
}
_;
}

2) Should change setPrediction() from public to internal function

Updates

Lead Judging Commences

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

setPrediction lacks access control

setPrediction has no access control and allows manipulation to Players' predictions.

Support

FAQs

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