Summary
Users are able to register and be approved more than once, which can lead to the same player taking more than one place. Having in mind that places are limited, this can lead to honest players not being able to participate.
Vulnerability Details
In terms of player statuses, the two possible ways are:
But nothing stops already approved players to call register() and change their state to Pending again. When a player is approved, the players array is updated and the address of the user is added to it. This opens a possibility to have the same address being populated multiple times in the array. This leads to a problem as the array is limited to only 30 users.
function register() public payable {
if (msg.value != entranceFee) {
revert ThePredicter__IncorrectEntranceFee();
}
if (block.timestamp > START_TIME - 14400) {
revert ThePredicter__RegistrationIsOver();
}
if (playersStatus[msg.sender] == Status.Pending) {
revert ThePredicter__CannotParticipateTwice();
}
playersStatus[msg.sender] = Status.Pending;
}
pragma solidity ^0.8.13;
import {Test, console} from "forge-std/Test.sol";
import {ThePredicter} from "../src/ThePredicter.sol";
import {ScoreBoard} from "../src/ScoreBoard.sol";
contract ThePredicterPocTest is Test {
ThePredicter public thePredicter;
ScoreBoard public scoreBoard;
address public organizer = makeAddr("organizer");
address public friend = makeAddr("friend");
address public stranger = makeAddr("stranger");
uint256 constant ENTRANCE_FEE = 0.04 ether;
uint256 constant PREDICTION_FEE = 0.0001 ether;
function setUp() public {
vm.startPrank(organizer);
scoreBoard = new ScoreBoard();
thePredicter = new ThePredicter(address(scoreBoard), ENTRANCE_FEE, PREDICTION_FEE);
scoreBoard.setThePredicter(address(thePredicter));
vm.stopPrank();
vm.deal(friend, ENTRANCE_FEE);
vm.deal(stranger, ENTRANCE_FEE * 2);
}
function testDoubleRegisterIssue() public {
vm.prank(stranger);
thePredicter.register{value: ENTRANCE_FEE}();
vm.prank(organizer);
thePredicter.approvePlayer(stranger);
vm.prank(stranger);
thePredicter.register{value: ENTRANCE_FEE}();
vm.prank(organizer);
thePredicter.approvePlayer(stranger);
vm.assertEq(thePredicter.players(0), stranger);
vm.assertEq(thePredicter.players(1), stranger);
}
}
Impact
Honest users might not be able to participate because of one player which has taken more than one place.
Tools Used
Manual review.
Recommendations
Consider adding a check if the user is already approved and do not allow to change his status.
function register() public payable {
if (msg.value != entranceFee) {
revert ThePredicter__IncorrectEntranceFee();
}
if (block.timestamp > START_TIME - 14400) {
revert ThePredicter__RegistrationIsOver();
}
+ if (playersStatus[msg.sender] == Status.Pending || playersStatus[msg.sender] == Status.Approved) {
revert ThePredicter__CannotParticipateTwice();
}
playersStatus[msg.sender] = Status.Pending;
}
