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

Low (2) / Informational (5) / Gas Findings (1)

guirewire

Low

[L-1] Centralized time management in ThePredicter.sol may affect tournament fairness

Summary:

register and makePrediction functions in ThePredicter.sol uses hardcoded timestamps and block.timestamp for time-based operations, which could potentially be manipulated, affecting the fairness of the tournament. Additionally the use of magic numbers for time calculations would be bad industry practice.

Vulnerability Details:

A hardcoded START_TIME constant determines the tournament's start:

uint256 private constant START_TIME = 1723752000; // Thu Aug 15 2024 20:00:00 GMT+0000

The register() function uses block.timestamp for registration deadline:

if (block.timestamp > START_TIME - 14400) {
revert ThePredicter__RegistrationIsOver();
}

The makePrediction() function also does this similarly:

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

Impact:

While funds are not directly at risk, the use of block.timestamp and hardcoded times could potentially be manipulated by miners to a small degree. This might allow for slightly unfair advantages in registration timing or prediction submissions. The impact is considered low as it doesn't put funds at risk and the manipulation potential is limited.

Tools Used:

Manual Review

Recommended Mitigation:

Consider implementing Chainlink's Time Oracle for more reliable and decentralized time management:

Import Chainlink's Time Oracle.

import "@chainlink/contracts/src/v0.8/interfaces/AggregatorV3Interface.sol";

Update ThePredicter.sol to use Chainlink's Time Oracle (also remove the use of Magic Numbers to follow best practices).

Note: REGISTRATION_BUFFER replaces the use of 14400 seconds with a constant*

contract ThePredicter {
using Address for address payable;
- uint256 private constant START_TIME = 1723752000; // Thu Aug 15 2024 20:00:00 GMT+0000
+ AggregatorV3Interface private timeOracle;
+ uint256 public immutable START_TIME;
+ uint256 private constant REGISTRATION_BUFFER = 14400; //4Hrs in seconds
// ... other contract variables ...
- constructor(address _scoreBoard, uint256 _entranceFee, uint256 _predictionFee) {
+ constructor(address _scoreBoard, uint256 _entranceFee, uint256 _predictionFee, address _timeOracleAddress, uint256 _startTime) {
organizer = msg.sender;
scoreBoard = ScoreBoard(_scoreBoard);
entranceFee = _entranceFee;
predictionFee = _predictionFee;
+ timeOracle = AggregatorV3Interface(_timeOracleAddress);
+ START_TIME = _startTime;
}
+ function getLatestTimestamp() public view returns (uint256) {
+ (, int256 answer, , ,) = timeOracle.latestRoundData();
+ return uint256(answer);
+ }
+
function register() public payable {
if (msg.value != entranceFee) {
revert ThePredicter__IncorrectEntranceFee();
}
- if (block.timestamp > START_TIME - 14400) {
+ if (getLatestTimestamp() > START_TIME - REGISTRATION_BUFFER) {
revert ThePredicter__RegistrationIsOver();
}
// ... rest of the function
}
}
function makePrediction(uint256 _matchNumber, ScoreBoard.Result _result) public payable {
- if (block.timestamp > scoreBoard.getMatchDate(_matchNumber) - 68400) {
+ if (getLatestTimestamp() > scoreBoard.getMatchDate(_matchNumber) - PREDICTION_CUTOFF) {
revert ThePredicter__PredictionsAreClosed();
}
// ... rest of the function
}
​
// ... other functions ...
}
}
}

(Magic Number 14400 was replaced with REGISTRATION_BUFFER constant to denote 4 hours (in seconds). Using named constants is better for readability and maintainability).

These changes would improve the contract's resistance to minor time manipulations and enhance its overall reliability and fairness.

[L-2] ThePredicter::approvePlayer Players can be approved after tournament has started, potentially creating unfair conditions for late entrants

Summary:
The approvePlayer function lacks a check to ensure players are not approved after the tournament has started.

Vulnerability Details:

function approvePlayer(address _player) public onlyOrganizer {
if (playersStatus[_player] != Status.Pending) {
revert ThePredicter__InvalidStatus();
}
playersStatus[_player] = Status.Approved;
}

There is no check against the tournament start time.

Impact:

Players could be approved after the tournament has started, which could lead to unfair conditions such as:

  • Late entrants might be at a disadvantage as they would have missed early prediction opportunities.

  • It could create confusion about the official participant list.

  • It might affect the fairness of reward distribution if late entrants are included after initial calculations.

  • Early participants might feel the competition terms have changed unfairly.

Tools Used:

Manual Review

Recommended Mitigation:

Add a check to ensure the tournament hasn't started:

function approvePlayer(address _player) public onlyOrganizer {
+ require(block.timestamp < tournamentStartTime, "ThePredicter: Tournament has already started");
if (playersStatus[_player] != Status.Pending) {
revert ThePredicter__InvalidStatus();
}
playersStatus[_player] = Status.Approved;
}

Note: This finding doesn't take into the consideration of using Chainlink's Time Oracle which was suggested in another finding to eliminate the centralized time management issue in [L-1].

Informational

[I-1] ScoreBoard::setThePredicter and ThePredicter::withdrawPredictionFees no event emitted, making it harder for someone to track changes

Summary:

ScoreBoard::setThePredicter function is designed to set the role of the Predicter. This function should emit an event when a new predicter is set.

Additionally, ThePredicter::withdrawPredictionFees function does not emit an event when fees are withdrawn, reducing off-chain traceability.

Vulnerability Details:

It is important to emit an event for a parameter change. In this case, the role of thePredicter is important as the purpose is to ensure users of this betting system know who exactly is carrying out the role of thePredicter.

Similarly to ThePredicter::withdrawPredictionFees, an event should be emitted after the transfer of fees

Impact:

The only was this harms the protocol is by not providing enough transparency for users of the betting system to know who is the thePredicter. Transparency is key for the security of users of the betting system. Poor transparency will discourage users from using the betting system.

Similar to ThePredicter.sol contract, emitting events is good practice for tracking fee withdrawals.

Tools Used:

Static Analysis (Slither), Manual Review, AI (Phind, ChatGPT)

Recommended Mitigation:

It is recommended to add an event to the setThePredicter function.

Updated ScoreBoard::setThePredicter to Add Event Emission;

Note: Remember to add the event to the ScoreBoard contract

event PredicterSet(address indexed newPredicter);

And then emit the event in the setThePredicter function.

​
function setThePredicter(address _thePredicter) public onlyOwner {
thePredicter = _thePredicter;
+ emit PredicterSet(_thePredicter);
}
​

Similarly, add the event and emit the event for ThePredicter::withdrawPredictionFees:

+ event PredictionFeesWithdrawn(address organizer, uint256 amount);
​
function withdrawPredictionFees() public onlyOrganizer {
uint256 fees = address(this).balance - (entranceFee * playersCount);
payable(msg.sender).transfer(fees);
+ emit PredictionFeesWithdrawn(msg.sender, fees);
}

[I-2] No Param Definitions of enum Result in ScoreBoard.sol

Summary:

enum Result has 4 values which are Pending, First, Draw and Second. It is best practice to define what each of these exactly do.

Vulnerability Details:

N/A - Informational finding.

Impact:

N/A - Informational finding.

Tools Used:

Manual Review

Recommended Mitigation:

Here is an example of what can be done to the ScoreBoard::enum Result to make it more readable.

+ // @param Pending: match in progress
+ // @param First: first team win
+ // @param Second: second team win
+ // @param Draw: match was tied, ie draw
enum Result {
Pending,
First,
Draw,
Second
}
​

[I-3] No Natspec for multiple functions in ThePredicter.sol, which slows down the initial process of getting context of the protocol

Summary:

No Natspec is seen for multiple functions across ThePredicter.sol. There are a few examples within this finding that provide a roadmap for how the developer team can implement Natspecs throughout their contracts.

Vulnerability Details:

N/A - Informational finding. No vulnerability as this is more for readability purposes.

Impact:

No Natspecs slow down the initial review process for users, developers and auditors to get a grasp on the purpose of what a function is supposed to do etc.

Tools Used:

Manual Review

Recommended Mitigation:

Here are a number of examples to mitigate this problem moving forward.

ThePredicter::enum Status Example

+ /// @notice Represents the current status of a player's registration
+ /// @dev Used to track the registration state of players in the prediction game
+ /// @param Unknown The initial state, indicating the player has not interacted with the contract
+ /// @param Pending The player has registered but is awaiting approval
+ /// @param Approved The player's registration has been approved and they can participate
+ /// @param Canceled The player has canceled their registration
enum Status {
Unknown,
Pending,
Approved,
Canceled
}

[I-4] Magic Numbers - Define and use constant variables instead of using literals

Summary:

If the same constant literal value is used multiple times, create a constant state variable and reference it throughout the contract.

Vulnerability Details/Impact:

4 Found Instances

  • Found in src/ScoreBoard.sol Line: 65

    if (block.timestamp <= START_TIME + matchNumber * 68400 - 68400) {

  • Found in src/ThePredicter.sol Line: 93

    if (block.timestamp > START_TIME + matchNumber * 68400 - 68400) {

Tools Used:

Static Analysis - Aderyn

Recommended Mitigation:

Use constant variables instead of literals.

[I-5] Single point of control in protocol, Example seen in withdrawPredictionFees function

Summary:

The withdrawPredictionFees function is designed with a single point of control, allowing only the organizer to withdraw fees.

Vulnerability Details:

function withdrawPredictionFees() public onlyOrganizer {
// ... function implementation
}

The function is restricted to be called only by the organizer address.

Impact:

Informational. While this design creates a centralization point, it appears to be an intentional choice by the developers to give Ivan sole control over a number of functions.

Recommended Mitigation:
No immediate action required if this is the intended design. However, for completeness, we suggest documenting this design choice and its implications:

Clearly document in the contract comments or external documentation that fee withdrawal is intentionally restricted to a single address.

Consider implementing a way to transfer this responsibility to another address in case of emergency:

address public organizer;
​
function transferOrganizer(address newOrganizer) public onlyOrganizer {
require(newOrganizer != address(0), "Invalid new organizer address");
organizer = newOrganizer;
emit OrganizerTransferred(msg.sender, newOrganizer);
}

*Note: This example is pretty loose. Since this is not important as the intent of the protocol is supposed to be somewhat centralized, GuireWire created a short example of what a user could do in a possible worst case scenario.

Ensure there's a robust key management strategy for the organizer's address.

These suggestions aim to maintain the intended centralized control while providing flexibility and transparency in the contract's management

Gas

[G-1] Gas optimization opportunity in ThePredicter::makePrediction by early input validation of _matchNumber

Updates

Lead Judging Commences

NightHawK Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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