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

Incorrect Fee Handling for Prediction Reset in makePrediction Function

Tomas0707

Summary

The makePrediction function in the ThePredicter.sol contract incorrectly requires a prediction fee for resetting predictions, whereas the fee should only be required for setting a new prediction. The function needs correction to handle fee requirements appropriately.

Vulnerability Details

The makePrediction function currently checks the fee every time a prediction is made, including when updating an existing prediction. This is incorrect if the fee is only meant to be charged for initial predictions and not for updates or resets.

https://github.com/Cyfrin/2024-07-the-predicter/blob/839bfa56fe0066e7f5610197a6b670c26a4c0879/src/ThePredicter.sol#L85-L91

Proof of Concept

This is the test code of updating prediction.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
​
import {Test, console} from "forge-std/Test.sol";
import {Strings} from "@openzeppelin/contracts/utils/Strings.sol";
import {ThePredicter} from "../src/ThePredicter.sol";
import {ScoreBoard} from "../src/ScoreBoard.sol";
import {ThePredicterTest} from "./ThePredicter.test.sol";
​
contract CheckPredictionTest is ThePredicterTest {
function testPrediction() public {
vm.startPrank(stranger);
vm.warp(1);
vm.deal(stranger, 1 ether);
thePredicter.register{value: 0.04 ether}();
vm.stopPrank();
​
vm.startPrank(organizer);
vm.warp(2);
thePredicter.approvePlayer(stranger);
vm.stopPrank();
​
vm.startPrank(stranger);
thePredicter.makePrediction{value: 0.0001 ether}(
0,
ScoreBoard.Result.First
);
emit log_named_uint("stranger.balance", address(stranger).balance);
​
vm.expectRevert(
abi.encodeWithSelector(ThePredicter__IncorrectPredictionFee.selector)
);
thePredicter.makePrediction(
0,
ScoreBoard.Result.Second
);
​
thePredicter.makePrediction{value: 0.0001 ether}(
0,
ScoreBoard.Result.Second
);
​
emit log_named_uint("stranger.balance", address(stranger).balance);
vm.stopPrank();
}
}

To test this code:

  • Input this code to new test solidity file: test/CheckPrediction.test.sol.

  • Then run this command:

    forge test --match-path test/CheckPrediction.test.sol --match-test testPrediction -vvvv

  • The result is:

    ├─ [83711] ThePredicter::makePrediction{value: 100000000000000}(0, 1)
    │ ├─ [24806] ScoreBoard::confirmPredictionPayment(stranger: [0x49052147F5D97A723DEBdf07680fFFaDAd29A5dC], 0)
    │ │ └─ ← [Stop]
    │ ├─ [50535] ScoreBoard::setPrediction(stranger: [0x49052147F5D97A723DEBdf07680fFFaDAd29A5dC], 0, 1)
    │ │ └─ ← [Stop]
    │ └─ ← [Stop]
    ├─ emit log_named_uint(key: "stranger.balance", val: 959900000000000000 [9.599e17])
    ├─ [0] VM::expectRevert(ThePredicter__IncorrectPredictionFee())
    │ └─ ← [Return]
    ├─ [507] ThePredicter::makePrediction(0, 3)
    │ └─ ← [Revert] ThePredicter__IncorrectPredictionFee()
    ├─ [29411] ThePredicter::makePrediction{value: 100000000000000}(0, 3)
    │ ├─ [906] ScoreBoard::confirmPredictionPayment(stranger: [0x49052147F5D97A723DEBdf07680fFFaDAd29A5dC], 0)
    │ │ └─ ← [Stop]
    │ ├─ [26635] ScoreBoard::setPrediction(stranger: [0x49052147F5D97A723DEBdf07680fFFaDAd29A5dC], 0, 3)
    │ │ └─ ← [Stop]
    │ └─ ← [Stop]
    ├─ emit log_named_uint(key: "stranger.balance", val: 959800000000000000 [9.598e17])

As you can see, users should repay prediction fee for updating their prediction.

Impact

  • The requirement to repay the prediction fee for resetting predictions can lead to unintended charges and confusion among users.

  • Players may be unfairly penalized or discouraged from updating their predictions due to incorrect fee handling.

Tools Used

Manual code review

Recommendations

Modify the makePrediction function to only require the prediction fee when setting a new prediction. Ensure no fee is required for resetting or updating existing predictions.

function getPrediction(address player, uint256 matchNumber) public view returns (ScoreBoard.Result) {
return playersPredictions[player].predictions[matchNumber];
}

ScoreBoard.sol

function makePrediction(
uint256 matchNumber,
ScoreBoard.Result prediction
) public payable {
bool isNewPrediction = (scoreBoard.getPrediction(msg.sender, matchNumber) == ScoreBoard.Result.Pending);
if (isNewPrediction && 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);
}

ThePredicter.sol

Updates

Lead Judging Commences

NightHawK Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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