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

Players can withdraw winnings multiple times

vinica_boy

Summary

After matches are over and results are set players can withdraw their winnings based on the correctness of their prediction. It is intended to withdraw all winning at once.
It is possible to withdraw winnings multiple times thus taking more than the user should receive and in the same time other users wont be able to take out their winnings.

Vulnerability Details

Withdrawal of winnings depends on scoreBoard.isEligibleForReward() function which checks if the result for the last match is set and if the player has more than one predictions. Predictions count is set to 0 in order to stop users to make multiple withdrawals, but this check can be passed by calling the setPrediction() function in ScoreBoard.sol.

function withdraw() public {
if (!scoreBoard.isEligibleForReward(msg.sender)) {
revert ThePredicter__NotEligibleForWithdraw();
}
...
if (reward > 0) {
scoreBoard.clearPredictionsCount(msg.sender);
(bool success, ) = msg.sender.call{value: reward}("");
require(success, "Failed to withdraw");
}
}
function isEligibleForReward(address player) public view returns (bool) {
return results[NUM_MATCHES - 1] != Result.Pending && playersPredictions[player].predictionsCount > 1;
}
function clearPredictionsCount(address player) public onlyThePredicter {
playersPredictions[player].predictionsCount = 0;
}
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; // predictionsCount can be updated after being cleared
}
}
}

Impact

This can lead to a malicious player draining the funds of the contract and making other players unable to withdraw anything.

Tools Used

Manual review

Recommendations

Consider refactoring the whole function to:

  • revert if predictions are closed

  • update the predictions count only if there is a new prediction

  • for already predicted matches leave the count untouched

function setPrediction(address player, uint256 matchNumber, Result result) public {
if (block.timestamp > START_TIME + matchNumber * 68400 - 68400) {
revert("Predictions closed");
}
if(result == Result.Pending) {
revert("Incorrect match outcome");
}
bool isFirstPrediction = playersPredictions[player].predictions[matchNumber] == Result.Pending;
if(isFirstPrediction) {
++playersPredictions[player].predictionsCount;
}
playersPredictions[player].predictions[matchNumber] = result;
}
Updates

Lead Judging Commences

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

Multiple withdrawals are possible

`ThePredicter.withdraw` combined with `ScoreBoard.setPrediction` allows a player to withdraw rewards multiple times leading to a drain of funds in the contract.

Support

FAQs

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