Summary
Fee calculation in ThePredicter::withdrawPredicitionFees can cause the organizer to withdraw more fees than intended and leave some users unable to claim their entrance fee or prevent players from claiming their reward
Vulnerability Details
The following formula is used to calculate the amount of prediction fees that should be withdrawn from the contract. The problem is that address(this).balance will include entrance fees of users who are not approved players in the players array. There are instances where a user may want to cancel their registration or a player will want to redeem their rewards but will be unable to do so as their fee has been incorrectly withdrawn.
function withdrawPredictionFees() public {
if (msg.sender != organizer) {
revert ThePredicter__NotEligibleForWithdraw();
}
uint256 fees = address(this).balance - players.length * entranceFee;
(bool success, ) = msg.sender.call{value: fees}("");
require(success, "Failed to withdraw");
}
Impact
A user may want to cancel their registration or a player may want to redeem their rewards but will be unable to do so.
POC
Add the following test to ThePredicter.test.sol
function test_breakPredictionFeesWithdrawal() public {
address stranger2 = makeAddr("stranger2");
address stranger3 = makeAddr("stranger3");
address stranger4 = makeAddr("stranger4");
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);
vm.stopPrank();
vm.startPrank(stranger);
thePredicter.makePrediction{value: 0.0001 ether}(
0,
ScoreBoard.Result.Draw
);
thePredicter.makePrediction{value: 0.0001 ether}(
1,
ScoreBoard.Result.Draw
);
vm.stopPrank();
vm.startPrank(stranger2);
thePredicter.makePrediction{value: 0.0001 ether}(
0,
ScoreBoard.Result.Draw
);
thePredicter.makePrediction{value: 0.0001 ether}(
1,
ScoreBoard.Result.Draw
);
vm.stopPrank();
vm.startPrank(organizer);
scoreBoard.setResult(0, ScoreBoard.Result.Draw);
scoreBoard.setResult(1, ScoreBoard.Result.Draw);
scoreBoard.setResult(8, ScoreBoard.Result.Draw);
vm.stopPrank();
vm.startPrank(organizer);
thePredicter.withdrawPredictionFees();
vm.stopPrank();
vm.startPrank(stranger);
thePredicter.withdraw();
vm.stopPrank();
vm.startPrank(stranger3);
thePredicter.cancelRegistration();
vm.stopPrank();
vm.startPrank(stranger2);
vm.assertEq(address(thePredicter).balance, 0 ether);
vm.expectRevert("Failed to withdraw");
thePredicter.withdraw();
vm.stopPrank();
}
Tools Used
Manual Review
Recommendations
Create a state variable to track the amount of prediction fees collected
address public organizer;
address[] public players;
uint256 public entranceFee;
uint256 public predictionFee;
+ uint256 public predictionFeesCollected;
ScoreBoard public scoreBoard;
mapping(address players => Status) public playersStatus;
Update the state variable each time a prediciton is made
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);
+ predictionFeesCollected += msg.value;
}
Use predictionFeesCollected as withdraw amount
function withdrawPredictionFees() public {
if (msg.sender != organizer) {
revert ThePredicter__NotEligibleForWithdraw();
}
- uint256 fees = address(this).balance - players.length * entranceFee;
+ (bool success, ) = msg.sender.call{value: predictionFeesCollected}("");
require(success, "Failed to withdraw");
}
